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