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. PR: https://git.openjdk.java.net/jfx/pull/25
