On Mon, 30 Mar 2020 13:37:51 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
> Closed focused Stages are not collected with Monocle > > This commit adds a unit-test and a fix for collecting focused closed stages. > > ticket: https://bugs.openjdk.java.net/browse/JDK-8241840 Suggested some changes and query. I still need to verify the fix in detail. tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 77: > 76: Platform.runLater(() -> stage.close()); > 77: } > 78: Looks like the primary `stage` is not required for actual test, and this block is only used to initialize JavaFX runtime. Please check if it can be replaced by below block ``` @BeforeClass public static void initFX() throws Exception { startupLatch = new CountDownLatch(1); Platform.startup(startupLatch::countDown); Assert.assertTrue("Timeout waiting for FX runtime to start", startupLatch.await(15, TimeUnit.MILLISECONDS)); } tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 133: > 132: > 133: Assert.assertNull(weakReference.get()); > 134: } This assert check call should be added to the @Test method. I would recommend to refer [this method](https://github.com/openjdk/jfx/blob/364c64a2e55b561df4ca1afc85c91054b339eea8/modules/javafx.controls/src/test/java/test/javafx/scene/control/ListViewTest.java#L1998) from ListViewTest. tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 113: > 112: }); > 113: > 114: Assert.assertTrue("Timeout, waiting for runLater. ", > leakLatch.await(15, TimeUnit.SECONDS)); Is it really required to nest the `Platform.runLater()` calls ? Can you please check if this code can be simplified by making the calls sequential, Consider using `Util.runAndWait()` tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 84: > 83: counter += 1; > 84: testLeakOnce(); > 85: } If the test has constant behavior on every run, then only one run should be done. modules/javafx.graphics/src/main/java/com/sun/glass/ui/Window.java line 354: > 353: } > 354: if(Window.focusedWindow == this) { > 355: Window.focusedWindow = null; Please correct format by adding space after `if`. tests/system/src/test/java/test/javafx/stage/FocusedWindowTest.java line 53: > 52: System.setProperty("monocle.platform","Headless"); > 53: } > 54: The test will always run with Monocle. I see that if this static block is removed then the test fails on Windows 10. Can you please verify all the platforms and change the test such that it runs on all platforms/ combinations. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/153
