On Wed, 13 Nov 2019 22:11:14 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> 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`. I've updated the pull request and merged it with master. Later, I will retest it with a Windows VM. I've done now the requested changes. Now only retesting it with Windows is left. I've now tested on Windows. I can confirm, that the test doesn't reproduce the error on Windows. PR: https://git.openjdk.java.net/jfx/pull/25
