RFR: 8200224: Multiple press event when JFXPanel gains focus

[Kevin Rushforth]

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

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