On Wed, 19 Nov 2025 17:14:51 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> NPE is seen while accessing transient "scenePeer" variable between reads..
>> Fix is made to store it in a temp variable rather than reading it twice
>> since the value can change between successive reads in many places it is
>> accessed.
>> Also some debug logs added to be enabled via `jfxpanel.debug` property
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review comment + OutputRedirect
Looked at how `stage`, `scene`, `stagePeer`, and `scenePeer` fields are used,
looks good. Left some minor comments for the follow-up/redesign.
Only the test needs to be changed, otherwise looks good. Thank you @prsadhuk
for persistence!
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassScene.java
line 253:
> 251: final void updateSceneState() {
> 252: // should only be called on the event thread
> 253: if (sceneState != null) {
L253, this is the original code, but I found the "event thread" words
misleading - too similar to "event dispatcher" thread. Should it be "fx
application thread"?
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 527:
> 525: if (hStagePeer != null) {
> 526: int focusCause = AbstractEvents.FOCUSEVENT_ACTIVATED;
> 527: hStagePeer.setFocused(true, focusCause);
suggestion (for a follow up): document which methods in
`EmbeddedStageInterface` can be called from which thread.
`setFocused()` method seems to be thread-safe.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1021:
> 1019: if ((stage != null) && !stage.isShowing()) {
> 1020: stage.show();
> 1021: SwingNodeHelper.runOnEDT(() -> sendMoveEventToFX());
for possible followup: L1032 the field returned in `getInputMethodRequests()`
in `EmbeddedScene` is not `volatile`.
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1116:
> 1114: dnd = new SwingDnD(JFXPanel.this, hScenePeer);
> 1115: dnd.addNotify();
> 1116: if (hScenePeer != null) {
this null check can be removed, since the code will bail out on L1106 if
`hScenePeer == null`
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line 98:
> 96: Thread.sleep(1);
> 97: Platform.runLater(() ->
> contentPane.setScene(webView.getScene()));
> 98: Thread.sleep(1);
discussed offline:
we'll should add some code (`invokeAndWait` + `Util.runAndWait`) here to make
sure all the prior events are processed before leaving the try block and doing
`OutputRedirect.checkAndRestoreStderr();`
Also, maybe add a `@Timeout` to make sure the test does not hang.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1968#pullrequestreview-3484167341
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2543126404
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2543329198
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2543356982
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2543363338
PR Review Comment: https://git.openjdk.org/jfx/pull/1968#discussion_r2543115097
