RFR: 8200224: Multiple press event when JFXPanel gains focus
fkirmaier at openjdk.java.net
Tue Nov 26 13:06:38 UTC 2019
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.
>> - 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;
> 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. : )
More information about the openjfx-dev