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). You can run the AWT_TESTS with the following statement: ./gradlew -PFULL_TEST=true swing:clean swing:test For some reason, it's hidden behind this flag. Maybe that's the reason, why you couldn't reproduce the bug? As fas as I can thee, the swing-tests are stable, so there is no real reason to hide it behind a flag. Maybe that's something which should be changed? The tests are now stable. The previous version had some problems because the other test for Swing was calling System.exit. The fix is well tested under Windows and Mac but not on Linux. The tests are based on a backport to an older JavaFX-version. PR: https://git.openjdk.java.net/jfx/pull/25
