RFR: 8200224: Multiple press event when JFXPanel gains focus
Kevin Rushforth
kcr at openjdk.java.net
Tue Nov 26 09:24:00 UTC 2019
On Tue, 26 Nov 2019 09:23:59 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:
> On Wed, 13 Nov 2019 22:11:14 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:
>
>> On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:
>>
>>> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:
>>>
>>>> On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:
>>>>
>>>>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier <github.com+6547435+FlorianKirmaier at 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
More information about the openjfx-dev
mailing list