RFR: 8200224: Multiple press event when JFXPanel gains focus

Tue Nov 26 09:23:59 UTC 2019

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.

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