On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier <fkirma...@openjdk.org> > wrote: > >> The pull request has been updated with a complete new set of changes >> (possibly due to a rebase). >> >> ---------------- >> >> Commits: >> - 44774dfb: JDK-8200224 >> - d1309fb6: JDK-8200224 >> - 53a66b8f: JDK-8200224 >> - 0be3cb5b: JDK-8200224 >> - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into >> JDK-8200224 >> - 5cd96a56: JDK-8200224 >> - 85c87628: JDK-8200224 >> - 314e423c: JDK-8200224 >> - 725e5fef: JDK-8200224 >> >> Changes: https://git.openjdk.java.net/jfx/pull/25/files >> Webrev: https://webrevs.openjdk.java.net/jfx/25/webrev.03 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >> Stats: 272 lines in 3 files changed: 147 ins; 120 del; 5 mod >> Patch: https://git.openjdk.java.net/jfx/pull/25.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/25/head:pull/25 > > To follow-up on my previous comment here are the requested changes: > > 1. Restore ` > tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`, > which was removed by your last commit, so that it doesn't show up in the PR. > 2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test > `JFXPanel` is the second one added (see my inline comments). This will allow > the test to catch the error on Windows by failing without your fix. > 3. A few other inline comments on the new test. > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved. >> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > That should be `2019` > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35: > >> 34: >> 35: >> 36: import javax.swing.JFrame; > > You only need one blank line here. > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94: > >> 93: >> 94: class TestFXPanel extends JFXPanel { >> 95: protected void processMouseEventPublic(MouseEvent e) { > > This class can be `static` > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108: > >> 107: >> 108: SwingUtilities.invokeLater(() -> { >> 109: TestFXPanel fxPnl = new TestFXPanel(); > > Add the following here: > JFXPanel dummyFXPanel = new JFXPanel(); > dummyFXPanel.setPreferredSize(new Dimension(100, 100)); > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112: > >> 111: JFrame jframe = new JFrame(); >> 112: JPanel jpanel = new JPanel(); >> 113: jpanel.add(fxPnl); > > Add the following here: > jpanel.add(dummyFXPanel); > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118: > >> 117: >> 118: Platform.runLater(() -> { >> 119: Group grp = new Group(); > > Add the following here: > Scene dummyScene = new Scene(new Group()); > dummyFXPanel.setScene(dummyScene); > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138: > >> 137: >> 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) { >> 139: throw new Exception(); > > This `if` test can be written more succinctly as: > > assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)); > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72: > >> 71: @BeforeClass >> 72: public static void doSetupOnce() { >> 73: // Start the Application > > If you add `throws Exception` then you don't need the try/catch around the > `launchLatch.await`. > > tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119: > >> 118: Platform.runLater(() -> { >> 119: Group grp = new Group(); >> 120: Scene scene = new Scene(new Group()); > > This variable is unused. > > ---------------- > > Changes requested by kcr (Lead). The if is triggered, when the time runs out. remvoed done done It's now simplified. done PR: https://git.openjdk.java.net/jfx/pull/25
