On Tue, 18 Nov 2025 17:03:49 GMT, Kevin Rushforth <[email protected]> wrote:
>> Prasanta Sadhukhan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> temp-var handling in few other methods
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 156:
>
>> 154: private transient volatile EmbeddedWindow stage;
>> 155:
>> 156: private transient volatile Scene scene;
>
> Minor: I would remove the blank line between these two declarations to make
> it clear that the comment applies to both.
I intentionally added the blank line to separate it out as `stage` is set in
`setSceneImpl `in FX thread and accessed in `paintComponent `in EDT thread
whereas `scene` is set in `setSceneImpl ` in FX thread and never accessed in
EDT thread
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 974:
>
>> 972: getScene().getFocusOwner() != null &&
>> 973: getScene().getFocusOwner().isFocused()) {
>> 974: hStagePeer.focusUngrab();
>
> This can be reverted. `stagePeer` is only accessed on the FX thread.
`stagePeer` is accessed in `processMouseEvent`, `sendResizeEventToFX`,
`sendMoveEventToFX`, `sendFocusEventToFX `in EDT so I did that.
But it seems in this particular method, it is only accessed in FX thread as is
mentioned so reverted..
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1023:
>
>> 1021: }
>> 1022: });
>> 1023: sendMoveEventToFX();
>
> This is not an equivalent change, since `sendMoveEventToFX()` can now happen
> before `stage.show()` and will also happen unconditionally even if the stage
> is null or already showing. Rather than moving it out of the runOnFxThread
> block, leave it where it is and call `runOnEDT` like this:
>
>
> SwingNodeHelper.runOnFxThread(() -> {
> if ((stage != null) && !stage.isShowing()) {
> stage.show();
> SwingNodeHelper.runOnEDT(() -> sendMoveEventToFX());
> }
> });
ok
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1078:
>
>> 1076: stagePeer = embeddedStage;
>> 1077: var hStagePeer = stagePeer;
>> 1078: if (hStagePeer == null) {
>
> If this is always invoked from the FX thread, you don't need to locally
> capture the peers.
but down below it is accessed in EDT so I did the local capture
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line
> 105:
>
>> 103: Thread.sleep(100);
>> 104: }
>> 105: System.out.println("failure = " + failure.get());
>
> This print can be removed.
ok
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2541819249
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2541821731
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2541822284
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2541822543
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2541822916
