On Mon, 7 Oct 2024 16:50:59 GMT, Thiago Milczarek Sayao <tsa...@openjdk.org>
wrote:
>> When creating a Scene, a `DrawableInfo` is allocated with `malloc`. When
>> scene changes, this is called on `WindowStage.java`:
>>
>> `QuantumRenderer.getInstance().disposePresentable(painter.presentable); //
>> latched on RT`
>>
>> But the underlying `DrawableInfo` is never freed.
>>
>> I also think this should be done when the Stage is closed.
>>
>> To test:
>>
>> import javafx.animation.Animation;
>> import javafx.animation.KeyFrame;
>> import javafx.animation.KeyValue;
>> import javafx.animation.Timeline;
>> import javafx.application.Application;
>> import javafx.scene.Scene;
>> import javafx.scene.control.TextField;
>> import javafx.scene.control.Label;
>> import javafx.scene.layout.Pane;
>> import javafx.scene.layout.StackPane;
>> import javafx.scene.layout.VBox;
>> import javafx.scene.paint.Color;
>> import javafx.stage.Stage;
>> import javafx.util.Duration;
>>
>> public class TestScenes extends Application {
>>
>> @Override
>> public void start(Stage stage) {
>> Timeline timeline = new Timeline(
>> new KeyFrame(Duration.millis(100), e ->
>> stage.setScene(createScene("Scene 1", Color.RED))),
>> new KeyFrame(Duration.millis(200), e ->
>> stage.setScene(createScene("Scene 2", Color.BLUE))),
>> new KeyFrame(Duration.millis(300), e ->
>> stage.setScene(createScene("Scene 3", Color.GREEN)))
>> );
>>
>> timeline.setCycleCount(Animation.INDEFINITE);
>> timeline.play();
>>
>> stage.show();
>> }
>>
>> private Scene createScene(String text, Color color) {
>> return new Scene(new StackPane(), 400, 300, color);
>> }
>>
>> public static void main(String[] args) {
>> launch(TestScenes.class, args);
>> }
>> }
>
> Thiago Milczarek Sayao has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Let `createGraphics` fail if called after dispose
The fix itself looks good. Providing a few minor comments.
modules/javafx.graphics/src/main/java/com/sun/prism/es2/IOSGLDrawable.java line
32:
> 30:
> 31: private static native long nCreateDrawable(long nativeWindow, long
> nativeCtxInfo);
> 32: private static native void nDestroyDrawable(long nativeCtxInfo);
I would recommend to rename these method as `nReleaseDrawable`.
I see we use names as ReleaseXxxx and DeleteXxxx. The DestroyXxxx name would
be a new addition, I think we could avoid the variation.
modules/javafx.graphics/src/main/native-prism-es2/ios/IOSGLDrawable.c line 55:
> 53:
> 54: /* initialize the structure */
> 55: memset(dInfo, 0, sizeof(DrawableInfo));
Minor: The declaration of `initializeDrawableInfo` from line#35 should be
removed.
modules/javafx.graphics/src/main/native-prism-es2/ios/IOSGLDrawable.c line 126:
> 124:
> 125: free(dInfo);
> 126: }
Minor: Please add a new line here.
modules/javafx.graphics/src/main/native-prism-es2/macosx/MacGLDrawable.c line
120:
> 118:
> 119: free(dInfo);
> 120: }
Minor: Please add a new line here.
modules/javafx.graphics/src/main/native-prism-es2/windows/WinGLDrawable.c line
76:
> 74:
> 75: /* initialize the structure */
> 76: memset(dInfo, 0, sizeof(DrawableInfo));
Minor: The declaration of `initializeDrawableInfo` from line#358 should be
removed.
modules/javafx.graphics/src/main/native-prism-es2/windows/WinGLDrawable.c line
148:
> 146:
> 147: free(dInfo);
> 148: }
Minor: Please add a new line here.
modules/javafx.graphics/src/main/native-prism-es2/x11/X11GLDrawable.c line 36:
> 34: #include "com_sun_prism_es2_X11GLDrawable.h"
> 35:
> 36: extern void initializeDrawableInfo(DrawableInfo *dInfo);
Minor: The declaration of function `initializeDrawableInfo` should be removed
as well.
-------------
Changes requested by arapte (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1586#pullrequestreview-2354297814
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791869606
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791777057
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791756735
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791771174
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791778528
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791771378
PR Review Comment: https://git.openjdk.org/jfx/pull/1586#discussion_r1791775168
