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