On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier 
<github.com+6547435+floriankirma...@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 

> 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 

> 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 

> 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 

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

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

Reply via email to