On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier <fkirma...@openjdk.org> > wrote: > >> On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth <k...@openjdk.org> wrote: >> >>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier >>> <github.com+6547435+floriankirma...@openjdk.org> wrote: >>> >>>> Original PR: https://github.com/javafxports/openjdk-jfx/pull/591 >>>> >>>> A side effect of JDK-8200224 is, that the first click on a JFXPanel is >>>> always a double click. >>>> The cause for JDK-8200224 is the fix for JDK-8087914 (Clicking on Menu in >>>> JFXPanel ignored if another swing component has focus). >>>> This fix introduced synthesized press-events, which is an unsafe fix for >>>> the problem. >>>> >>>> The pull request introduces a new fix for the problem. >>>> Instead of simulating new input-events, we set JavaFX Scene/Window to >>>> focused. >>>> We do so, by using setFocused. >>>> When the original Swing-Focus event is called slightly later, it won't >>>> have any side-effects, >>>> because calling setFocused just sets the value of a boolean property. >>>> >>>> I've now also added a unit-test for the problem. >>>> >>>> ---------------- >>>> >>>> Commits: >>>> - 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.00 >>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >>>> Stats: 140 lines in 2 files changed: 133 ins; 2 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 >>> >>> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line >>> 457: >>> >>>> 456: >>>> 457: sendMouseEventToFX(e); >>>> 458: super.processMouseEvent(e); >>> >>> typo: save --> safe >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 2: >>> >>>> 1: /* >>>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights >>>> reserved. >>>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. >>> >>> replace `2014, 2016` with `2019,` >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 28: >>> >>>> 27: >>>> 28: import javafx.application.Application; >>>> 29: import javafx.application.Platform; >>> >>> Remove unused import (several of the below imports are similarly unused). >>> Also, since this is a new test, please sort the imports. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 48: >>> >>>> 47: >>>> 48: import static junit.framework.Assert.assertEquals; >>>> 49: import static org.junit.Assert.assertNotNull; >>> >>> This method should be imported from `org.junit` package. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 116: >>> >>>> 115: MouseEvent e = new MouseEvent(fxPnl, >>>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK, >>>> 116: 5, 5, 1, false, MouseEvent.BUTTON1); >>>> 117: >>> >>> This doesn't appear to trigger the bug -- at least on Windows. The test >>> passes for me with or without your fix. You might consider moving it to the >>> system tests project, under `tests/system/src/test/java/test/robot` and >>> using `Robot` to effect the mouse press. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 57: >>> >>>> 56: >>>> 57: >>>> 58: @BeforeClass >>> >>> You can remove the extra blank line. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 65: >>> >>>> 64: >>>> 65: >>>> 66: try { >>> >>> You can remove the extra blank line. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 79: >>> >>>> 78: >>>> 79: class TestFXPanel extends JFXPanel { >>>> 80: protected void processMouseEventPublic(MouseEvent e) { >>> >>> I think this can be a static class. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 88: >>> >>>> 87: >>>> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1); >>>> 89: >>> >>> This can be final. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 91: >>> >>>> 90: // It's an array, so we can mutate it inside of lambda >>>> statement >>>> 91: int[] pressedEventCounter = {0}; >>>> 92: >>> >>> This can be final. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 132: >>> >>>> 131: >>>> 132: >>>> 133: } >>> >>> You can remove the extra blank line. >>> >>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java >>> line 123: >>> >>>> 122: >>>> 123: if(!firstPressedEventLatch.await(5000, >>>> TimeUnit.MILLISECONDS)) { >>>> 124: throw new Exception(); >>> >>> Add a space after the `if`. >>> >>> The fix seems fine. Have you tested it on all three platforms? >>> >>> I have several comments on the test. >>> >>> ---------------- >>> >>> Disapproved by kcr (Lead). >> >> Maybe try `./gradlew -PFULL_TEST=true swing:clean swing:test`. >> I'm sure, it can be reproduced on windows. >> If you still can not reproduce it, then I will add a test for the Robot. > > I'll try it again with the updated test. > >> You can run the AWT_TESTS with the following statement: >> >> ``` >> ./gradlew -PFULL_TEST=true swing:clean swing:test >> ``` > > Yes, I know that it needs `-PFULL_TEST=true`, but the earlier test wasn't > failing for me. And yes, it is intentional; for reasons I can't recall, the > swing tests don't work in headless mode. Anyway, I don't want to revisit this > right now. > >> The tests are now stable. The previous version had some problems because the >> other test for Swing was calling System.exit. > > Tests should not call `System.exit`. If they do, then that's almost certainly > a bug. FYI, I filed [JDK-8234110](https://bugs.openjdk.java.net/browse/JDK-8234110) to move the existing `:swing` test to `:systemTests`. PR: https://git.openjdk.java.net/jfx/pull/25
