On Tue, 26 Nov 2019 09:23:59 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
> 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. Yes, I confirmed this last Friday, but ran out of time to reply. You also need to restore the test that you moved from `test.robot.*` to its original state (so we preserve the previous Mac-only test). PR: https://git.openjdk.java.net/jfx/pull/25
