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. : ) PR: https://git.openjdk.java.net/jfx/pull/25
