RFR: 8200224: Multiple press event when JFXPanel gains focus
Kevin Rushforth
kcr at openjdk.org
Wed Nov 13 22:11:14 UTC 2019
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`.
PR: https://git.openjdk.java.net/jfx/pull/25
More information about the openjfx-dev
mailing list