On Wed, 30 Aug 2023 06:49:52 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> Due to transient datatype of scenePeer, it can become null which can result
>> in NPE in scenarios where scene is continuously been reset and set, which
>> warrants a null check, as is done in other places for the same variable.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Add nullcheck for sceneState
overall looks good - the new LOCK'ing logic appears I have some minor comments
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 444:
> 442: return pHeight;
> 443: }
> 444: }
perhaps we could add setCompSize(int, int) to set both pHeight and pWidth at
the same time, in order to avoid multiple LOCK'ed blocks down below (L638,
L1068)?
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 450:
> 448: return scaleFactorX;
> 449: }
> 450: }
same idea, possibly add setScaleFactor(double, double)?
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 885:
> 883: double lScaleFactorX = getScaleFactorX();
> 884: double lScaleFactorY = getScaleFactorY();
> 885: if (lScenePeer == null) {
minor: move check on L885 right after L880, no need to lock 4 times if
lScenePeer is null
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 1147:
> 1145: invokeOnClientEDT(() -> {
> 1146: SwingDnD lDnD = getDnD();
> 1147: lDnD = new SwingDnD(JFXPanel.this, embeddedScene);
is there a need for getting getDnd() on L1146 and immediately overwriting it on
L1147?
should it be
`SwingDnD lDnD = new SwingDnD(JFXPanel.this, embeddedScene);`
?
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelNPETest.java line 44:
> 42: import org.junit.Assert;
> 43: import org.junit.BeforeClass;
> 44: import org.junit.Test;
should we be using junit5?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1178#pullrequestreview-1605556486
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312121220
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312122175
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312128303
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312132172
PR Review Comment: https://git.openjdk.org/jfx/pull/1178#discussion_r1312132992
