On Tue, 26 Nov 2019 13:06:38 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
> On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth <k...@openjdk.org> wrote: > >> 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 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
