RFR: 8200224: Multiple press event when JFXPanel gains focus

[Florian Kirmaier]

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.

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