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).

You can run the AWT_TESTS with the following statement:
./gradlew -PFULL_TEST=true swing:clean swing:test
For some reason, it's hidden behind this flag.
Maybe that's the reason, why you couldn't reproduce the bug?
As fas as I can thee, the swing-tests are stable, so there is no real reason to hide it behind a flag.
Maybe that's something which should be changed?

The tests are now stable. The previous version had some problems because the other test for Swing was calling System.exit.

The fix is well tested under Windows and Mac but not on Linux. The tests are based on a backport to an older JavaFX-version.

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