RFR: 8200224: Multiple press event when JFXPanel gains focus

[Kevin Rushforth]

On Tue, 26 Nov 2019 13:06:38 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

> On Tue, 26 Nov 2019 09:24:01 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
>> 
>> I left a couple additional comments about the test changes. Namely:
>> 
>> 1. You didn't fully revert the changes to `SwingFXUtilsTest`
>> 2. Your new test should be put in its own class in `test.javafx.embed.swing` (and not in the exist mac-only Robot test)
>> 
>> tests/system/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java line 87:
>> 
>>> 86:         testFromFXImg("opaque.gif");
>>> 87:         testFromFXImg("opaque.jpg");
>>> 88:         testFromFXImg("opaque.png");
>> 
>> You didn't fully revert your change to this file. The above needs to be restored such that when you are done, this file is unchanged versus master, and not part of the PR.
>> 
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line 25:
>> 
>>> 24:  */
>>> 25: package test.robot.javafx.embed.swing;
>>> 26: 
>> 
>> Since you aren't using `Robot` I'll repeat my earlier recommendation to put your new test in `test.javafx.embed.swing` (as a new test class) rather than modifying this existing test class. This class isn't suitable anyway, since it is only run on Mac.
>> 
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line 40:
>> 
>>> 39: import javax.swing.*;
>>> 40: import java.awt.*;
>>> 41: import java.awt.event.InputEvent;
>> 
>> The use of wild card imports is discouraged (except for static imports).
>> 
>> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java line 129:
>> 
>>> 128:             JPanel jpanel = new JPanel();
>>> 129:             jpanel.add(fxPnl);
>>> 130:             jframe.setContentPane(jpanel);
>> 
>> You didn't add an initial dummy `JFXPanel` so I suspect it will still not catch the bug (meaning it will still pass even without your patch).
>> 
>> ----------------
>> 
>> Changes requested by kcr (Lead).
> 
> 1. I've restored the test. But the git history is now very chaotic. Especially the moves might cause problems. Do the commits get squashed after merging? Otherwise, it might make sense, to redo the changes on a fresh branch and create a new pull request.
> 2. The test works now on windows. : )

Yes, the commits are squashed, so don't worry about the history (in worst case I would ask you to do a squash-rebase / force push rather than create a new PR, but that isn't needed here).

At first glance the changes you made look good. I'll take a closer look later.

@prsadhuk please also review this.

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