RFR: 8200224: Multiple press event when JFXPanel gains focus

[Kevin Rushforth]

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.

PR: https://git.openjdk.java.net/jfx/pull/25