Re: [Integrated] RFR: 8200224: Multiple press event when JFXPanel gains focus
Changeset: 1d670f18 Author:Florian Kirmaier Committer: Kevin Rushforth Date: 2019-11-27 16:20:12 + URL: https://git.openjdk.java.net/jfx/commit/1d670f18 8200224: Multiple press event when JFXPanel gains focus Reviewed-by: kcr, psadhukhan ! modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java + tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
Re: [Rev 05] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - fa465c55: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/24385eb8..fa465c55 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.05 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.04-05 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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 PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 27 Nov 2019 15:13:33 GMT, Kevin Rushforth wrote: > On Wed, 27 Nov 2019 05:31:27 GMT, Prasanta Sadhukhan > wrote: > >> On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier >> wrote: >> >>> The pull request has been updated with additional changes. >>> >>> >>> >>> Added commits: >>> - 24385eb8: JDK-8200224 >>> - e0829ad3: JDK-8200224 >>> - c190384f: JDK-8200224 >>> - 17b458b1: JDK-8200224 >>> >>> Changes: >>> - all: https://git.openjdk.java.net/jfx/pull/25/files >>> - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 >>> >>> Webrevs: >>> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 >>> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 >>> >>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >>> Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 >> >> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: >> >>> 448: requestFocus(); >>> 449: // This fixes JDK-8087914 without causing JDK-8200224 >>> 450: // It is safe, because in JavaFX only the method >>> "setFocused(true)" is called, >> >> We actually do not clutter source code with bugids, it will be good if it >> can be removed with proper comments maybe something like "extra simulated >> mouse pressed event is removed by making the JavaFX scene focused" > > In general, we no longer reference bug IDs in source code, so I agree with > the recommendation to reword the comment. > > @FlorianKirmaier - can you reword as suggested? Once you are done, you can > integrate it (using the `/integrate` command), and I will sponsor it. There > will likely be a delay due to the US Thanksgiving holiday. Done PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 27 Nov 2019 05:31:27 GMT, Prasanta Sadhukhan wrote: > On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier > wrote: > >> The pull request has been updated with additional changes. >> >> >> >> Added commits: >> - 24385eb8: JDK-8200224 >> - e0829ad3: JDK-8200224 >> - c190384f: JDK-8200224 >> - 17b458b1: JDK-8200224 >> >> Changes: >> - all: https://git.openjdk.java.net/jfx/pull/25/files >> - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 >> >> Webrevs: >> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 >> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 >> >> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 >> Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 > > modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: > >> 448: requestFocus(); >> 449: // This fixes JDK-8087914 without causing JDK-8200224 >> 450: // It is safe, because in JavaFX only the method >> "setFocused(true)" is called, > > We actually do not clutter source code with bugids, it will be good if it can > be removed with proper comments maybe something like "extra simulated mouse > pressed event is removed by making the JavaFX scene focused" In general, we no longer reference bug IDs in source code, so I agree with the recommendation to reword the comment. @FlorianKirmaier - can you reword as suggested? Once you are done, you can integrate it (using the `/integrate` command), and I will sponsor it. There will likely be a delay due to the US Thanksgiving holiday. PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Approved] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 Looks good albeit with minor suggestion regarding presence of bugid in comment Approved by psadhukhan (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 449: > 448: requestFocus(); > 449: // This fixes JDK-8087914 without causing JDK-8200224 > 450: // It is safe, because in JavaFX only the method > "setFocused(true)" is called, We actually do not clutter source code with bugids, it will be good if it can be removed with proper comments maybe something like "extra simulated mouse pressed event is removed by making the JavaFX scene focused" PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Approved] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:36 GMT, Florian Kirmaier wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - 24385eb8: JDK-8200224 > - e0829ad3: JDK-8200224 > - c190384f: JDK-8200224 > - 17b458b1: JDK-8200224 > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/25/files > - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 > - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 > Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 All looks good now. As a reminder, this will need a second reviewer. Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 13:06:38 GMT, Florian Kirmaier
wrote:
> On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
>
>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
>> 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
Re: [Rev 04] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - 24385eb8: JDK-8200224 - e0829ad3: JDK-8200224 - c190384f: JDK-8200224 - 17b458b1: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/44774dfb..24385eb8 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.04 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.03-04 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 141 lines in 2 files changed: 123 ins; 11 del; 7 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 PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> 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
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier
> wrote:
>
>> The pull request has been updated with a complete new set of changes
>> (possibly due to a rebase).
>>
>>
>>
>> Commits:
>> - 44774dfb: JDK-8200224
>> - d1309fb6: JDK-8200224
>> - 53a66b8f: JDK-8200224
>> - 0be3cb5b: JDK-8200224
>> - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into
>> JDK-8200224
>> - 5cd96a56: JDK-8200224
>> - 85c87628: JDK-8200224
>> - 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.03
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 272 lines in 3 files changed: 147 ins; 120 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
>
> To follow-up on my previous comment here are the requested changes:
>
> 1. Restore `
> tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`,
> which was removed by your last commit, so that it doesn't show up in the PR.
> 2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test
> `JFXPanel` is the second one added (see my inline comments). This will allow
> the test to catch the error on Windows by failing without your fix.
> 3. A few other inline comments on the new test.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> That should be `2019`
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35:
>
>> 34:
>> 35:
>> 36: import javax.swing.JFrame;
>
> You only need one blank line here.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94:
>
>> 93:
>> 94: class TestFXPanel extends JFXPanel {
>> 95: protected void processMouseEventPublic(MouseEvent e) {
>
> This class can be `static`
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108:
>
>> 107:
>> 108: SwingUtilities.invokeLater(() -> {
>> 109: TestFXPanel fxPnl = new TestFXPanel();
>
> Add the following here:
> JFXPanel dummyFXPanel = new JFXPanel();
> dummyFXPanel.setPreferredSize(new Dimension(100, 100));
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112:
>
>> 111: JFrame jframe = new JFrame();
>> 112: JPanel jpanel = new JPanel();
>> 113: jpanel.add(fxPnl);
>
> Add the following here:
> jpanel.add(dummyFXPanel);
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118:
>
>> 117:
>> 118: Platform.runLater(() -> {
>> 119: Group grp = new Group();
>
> Add the following here:
> Scene dummyScene = new Scene(new Group());
> dummyFXPanel.setScene(dummyScene);
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138:
>
>> 137:
>> 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
>> 139: throw new Exception();
>
> This `if` test can be written more succinctly as:
>
> assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS));
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72:
>
>> 71: @BeforeClass
>> 72: public static void doSetupOnce() {
>> 73: // Start the Application
>
> If you add `throws Exception` then you don't need the try/catch around the
> `launchLatch.await`.
>
> tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119:
>
>> 118: Platform.runLater(() -> {
>> 119: Group grp = new Group();
>> 120: Scene scene = new Scene(new Group());
>
> This variable is unused.
>
>
>
> Changes requested by kcr (Lead).
The if is triggered, when the time runs out.
remvoed
done
done
It's now simplified.
done
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
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).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:23:59 GMT, Florian Kirmaier
wrote:
> On Wed, 13 Nov 2019 22:11:14 GMT, Kevin Rushforth wrote:
>
>> On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth wrote:
>>
>>> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier
>>> wrote:
>>>
On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> 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
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line
> 457:
>
>> 456:
>> 457: sendMouseEventToFX(e);
>> 458: super.processMouseEvent(e);
>
> typo: save --> safe
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
>> reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> replace `2014, 2016` with `2019,`
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 28:
>
>> 27:
>> 28: import javafx.application.Application;
>> 29: import javafx.application.Platform;
>
> Remove unused import (several of the below imports are similarly unused).
> Also, since this is a new test, please sort the imports.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 48:
>
>> 47:
>> 48: import static junit.framework.Assert.assertEquals;
>> 49: import static org.junit.Assert.assertNotNull;
>
> This method should be imported from `org.junit` package.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 116:
>
>> 115: MouseEvent e = new MouseEvent(fxPnl,
>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
>> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
>> 117:
>
> This doesn't appear to trigger the bug -- at least on Windows. The test
> passes for me with or without your fix. You might consider moving it to
> the system tests project, under `tests/system/src/test/java/test/robot`
> and using `Robot` to effect the mouse press.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 57:
>
>> 56:
>> 57:
>> 58: @BeforeClass
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 65:
>
>> 64:
>> 65:
>> 66: try {
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 79:
>
>> 78:
>> 79: class TestFXPanel extends JFXPanel {
>> 80: protected void processMouseEventPublic(MouseEvent e) {
>
> I think this can be a static class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 88:
>
>> 87:
>> 88: CountDownLatch firstPressedEventLatch = new
>> CountDownLatch(1);
>> 89:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 91:
>
>> 90: // It's an array, so we can mutate it inside of lambda
>> statement
>> 91: int[] pressedEventCounter = {0};
>> 92:
>
>
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier
wrote:
> The pull request has been updated with a complete new set of changes
> (possibly due to a rebase).
>
>
>
> Commits:
> - 44774dfb: JDK-8200224
> - d1309fb6: JDK-8200224
> - 53a66b8f: JDK-8200224
> - 0be3cb5b: JDK-8200224
> - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into
> JDK-8200224
> - 5cd96a56: JDK-8200224
> - 85c87628: JDK-8200224
> - 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.03
> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
> Stats: 272 lines in 3 files changed: 147 ins; 120 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
To follow-up on my previous comment here are the requested changes:
1. Restore `
tests/system/src/test/java/test/robot/javafx/embed/swing/JFXPanelTest.java`,
which was removed by your last commit, so that it doesn't show up in the PR.
2. In the new test, add a dummy `JFXPanel` to the `JPanel` so that the test
`JFXPanel` is the second one added (see my inline comments). This will allow
the test to catch the error on Windows by failing without your fix.
3. A few other inline comments on the new test.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 2:
> 1: /*
> 2: * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
That should be `2019`
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 35:
> 34:
> 35:
> 36: import javax.swing.JFrame;
You only need one blank line here.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 94:
> 93:
> 94: class TestFXPanel extends JFXPanel {
> 95: protected void processMouseEventPublic(MouseEvent e) {
This class can be `static`
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 108:
> 107:
> 108: SwingUtilities.invokeLater(() -> {
> 109: TestFXPanel fxPnl = new TestFXPanel();
Add the following here:
JFXPanel dummyFXPanel = new JFXPanel();
dummyFXPanel.setPreferredSize(new Dimension(100, 100));
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 112:
> 111: JFrame jframe = new JFrame();
> 112: JPanel jpanel = new JPanel();
> 113: jpanel.add(fxPnl);
Add the following here:
jpanel.add(dummyFXPanel);
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 118:
> 117:
> 118: Platform.runLater(() -> {
> 119: Group grp = new Group();
Add the following here:
Scene dummyScene = new Scene(new Group());
dummyFXPanel.setScene(dummyScene);
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 138:
> 137:
> 138: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
> 139: throw new Exception();
This `if` test can be written more succinctly as:
assertTrue(firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS));
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 72:
> 71: @BeforeClass
> 72: public static void doSetupOnce() {
> 73: // Start the Application
If you add `throws Exception` then you don't need the try/catch around the
`launchLatch.await`.
tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java line 119:
> 118: Platform.runLater(() -> {
> 119: Group grp = new Group();
> 120: Scene scene = new Scene(new Group());
This variable is unused.
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 02] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 13 Nov 2019 21:53:13 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier
> wrote:
>
>> The pull request has been updated with additional changes.
>>
>>
>>
>> Added commits:
>> - 5cd96a56: JDK-8200224
>>
>> Changes:
>> - all: https://git.openjdk.java.net/jfx/pull/25/files
>> - new: https://git.openjdk.java.net/jfx/pull/25/files/85c87628..5cd96a56
>>
>> Webrevs:
>> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.02
>> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.01-02
>>
>> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
>> Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 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 see that when you made your earlier comment regarding `System.exit`, you
> really meant that the existing swing test was calling `Platform.exit`, which
> isn't the same thing; it does shut down the JavaFX runtime, which is intended.
>
> The problem you are running into is that gradle runs multiple tests in the
> same VM and in an undefined, and unpredicable, order. This means that tests
> need to take care not to alter or rely on global state such as threads,
> static fields, global JavaFX state, and the running (or lack thereof) of the
> JavaFX runtime. The swing tests violate this rule and are therefore
> inherently unstable. The only reason this hasn't been a problem up to now is
> that the javafx.swing module contains a single test class. I will file a new
> test bug to address it -- probably by moving that test to the `systemTests`
> project.
>
> You will need to move your test into the `systemTests` project under the
> `tests/system/` directory. Such tests are valid in the system tests project
> because we run each test in that project in its own VM. Once your proposed
> test is robust (meaning consistently catches the bug without your fix and
> consistently passes with your fix) on all platforms without using Robot, then
> `tests/system/src/test/java/test/javafx/embed/swing/` would be the best place
> to put your new test.
>
> Regarding the test itself, it still doesn't fail for me on Windows without
> your fix. I ran the test program attached to the bug and I see something that
> might help explain this. That test program creates a pair of JFXPanel objects
> and adds both to the JPanel. If I first click on the first one, then it only
> shows a single click. Every time after that, if I click on a new JFXPanel,
> then I get 2 clicks. If, instead, I click on the second JFXPanel right after
> starting the program, I get 2 clicks the first time. With that in mind, I
> modified your test to add a dummy JFXPanel to the JPanel before the one you
> are sending the mouse pressed event to, and then it then does what I expect:
> it catches the bug without your fix and passes with your fix. That might help
> you come up with a more robust test. I added some specific comments on the
> test as well.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java
> line 46:
>
>> 45:
>> 46:
>> 47: public class SwingFXUtilsTest {
>
> Your proposed fix for this test class is not the right fix, and needs to be
> reverted. It highlights an existing bug with the swing tests, which I will
> address in general comments.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 100:
>
>> 99: jpanel.add(fxPnl);
>> 100: jframe.setContentPane(jpanel);
>> 101: jframe.setVisible(true);
>
> You should call `jframe.pack()` here.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 128:
>
>> 127:
>> 128: Thread.sleep(100); // there should be no pressed event after
>> the initial one. Let's wait for 0.1s and check again.
>> 129: assertEquals(1, pressedEventCounter[0]);
>
> I recommend using 500 msec so we don't risk missing a failing click on slower
> systems.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 60:
>
>> 59: public static void doSetupOnce() {
>> 60: if (setupIsDone) return;
>> 61: Platform.startup(() -> {
>
> This is not needed if you revert the changes to the other test class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 130:
>
>> 129: assertEquals(1, pressedEventCounter[0]);
>> 130: }
>> 131: }
>
> You will want to add a cleanup method, annotated with `@AfterClass` to call
> `Platform.exit` and dispose of the JFrame.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 106:
>
>> 105: Scene scene = new Scene(new Group());
>> 106: scene.getRoot().requestFocus();
>> 107:
>
> I don't think requesting focus is necessary (or desired).
>
> -
Re: [Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with a complete new set of changes (possibly due to a rebase). Commits: - 44774dfb: JDK-8200224 - d1309fb6: JDK-8200224 - 53a66b8f: JDK-8200224 - 0be3cb5b: JDK-8200224 - 03b59288: Merge branch 'master' of https://github.com/openjdk/jfx into JDK-8200224 - 5cd96a56: JDK-8200224 - 85c87628: JDK-8200224 - 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.03 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 272 lines in 3 files changed: 147 ins; 120 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 PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 13 Nov 2019 22:11:14 GMT, Kevin Rushforth wrote:
> On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth wrote:
>
>> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier
>> wrote:
>>
>>> On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
>>>
On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
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
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line
457:
> 456:
> 457: sendMouseEventToFX(e);
> 458: super.processMouseEvent(e);
typo: save --> safe
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 2:
> 1: /*
> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
replace `2014, 2016` with `2019,`
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 28:
> 27:
> 28: import javafx.application.Application;
> 29: import javafx.application.Platform;
Remove unused import (several of the below imports are similarly unused).
Also, since this is a new test, please sort the imports.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 48:
> 47:
> 48: import static junit.framework.Assert.assertEquals;
> 49: import static org.junit.Assert.assertNotNull;
This method should be imported from `org.junit` package.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 116:
> 115: MouseEvent e = new MouseEvent(fxPnl,
> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
> 117:
This doesn't appear to trigger the bug -- at least on Windows. The test
passes for me with or without your fix. You might consider moving it to
the system tests project, under `tests/system/src/test/java/test/robot`
and using `Robot` to effect the mouse press.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 57:
> 56:
> 57:
> 58: @BeforeClass
You can remove the extra blank line.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 65:
> 64:
> 65:
> 66: try {
You can remove the extra blank line.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 79:
> 78:
> 79: class TestFXPanel extends JFXPanel {
> 80: protected void processMouseEventPublic(MouseEvent e) {
I think this can be a static class.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 88:
> 87:
> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
> 89:
This can be final.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 91:
> 90: // It's an array, so we can mutate it inside of lambda
> statement
> 91: int[] pressedEventCounter = {0};
> 92:
This can be final.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 132:
> 131:
> 132:
> 133: }
You can remove the extra blan
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 13 Nov 2019 01:17:44 GMT, Kevin Rushforth wrote:
> On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier
> wrote:
>
>> On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
>>
>>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
>>> 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
>>>
>>> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line
>>> 457:
>>>
456:
457: sendMouseEventToFX(e);
458: super.processMouseEvent(e);
>>>
>>> typo: save --> safe
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 2:
>>>
1: /*
2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
reserved.
3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>
>>> replace `2014, 2016` with `2019,`
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 28:
>>>
27:
28: import javafx.application.Application;
29: import javafx.application.Platform;
>>>
>>> Remove unused import (several of the below imports are similarly unused).
>>> Also, since this is a new test, please sort the imports.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 48:
>>>
47:
48: import static junit.framework.Assert.assertEquals;
49: import static org.junit.Assert.assertNotNull;
>>>
>>> This method should be imported from `org.junit` package.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 116:
>>>
115: MouseEvent e = new MouseEvent(fxPnl,
MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
116: 5, 5, 1, false, MouseEvent.BUTTON1);
117:
>>>
>>> This doesn't appear to trigger the bug -- at least on Windows. The test
>>> passes for me with or without your fix. You might consider moving it to the
>>> system tests project, under `tests/system/src/test/java/test/robot` and
>>> using `Robot` to effect the mouse press.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 57:
>>>
56:
57:
58: @BeforeClass
>>>
>>> You can remove the extra blank line.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 65:
>>>
64:
65:
66: try {
>>>
>>> You can remove the extra blank line.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 79:
>>>
78:
79: class TestFXPanel extends JFXPanel {
80: protected void processMouseEventPublic(MouseEvent e) {
>>>
>>> I think this can be a static class.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 88:
>>>
87:
88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
89:
>>>
>>> This can be final.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 91:
>>>
90: // It's an array, so we can mutate it inside of lambda
statement
91: int[] pressedEventCounter = {0};
92:
>>>
>>> This can be final.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 132:
>>>
131:
132:
133: }
>>>
>>> You can remove the extra blank line.
>>>
>>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>>> line 123:
>>>
122:
123: if(!firstPressedEventLatch.await(5000,
TimeUnit.MILLISECOND
Re: [Rev 02] RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier
wrote:
> The pull request has been updated with additional changes.
>
>
>
> Added commits:
> - 5cd96a56: JDK-8200224
>
> Changes:
> - all: https://git.openjdk.java.net/jfx/pull/25/files
> - new: https://git.openjdk.java.net/jfx/pull/25/files/85c87628..5cd96a56
>
> Webrevs:
> - full: https://webrevs.openjdk.java.net/jfx/25/webrev.02
> - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.01-02
>
> Issue: https://bugs.openjdk.java.net/browse/JDK-8200224
> Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 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 see that when you made your earlier comment regarding `System.exit`, you
really meant that the existing swing test was calling `Platform.exit`, which
isn't the same thing; it does shut down the JavaFX runtime, which is intended.
The problem you are running into is that gradle runs multiple tests in the same
VM and in an undefined, and unpredicable, order. This means that tests need to
take care not to alter or rely on global state such as threads, static fields,
global JavaFX state, and the running (or lack thereof) of the JavaFX runtime.
The swing tests violate this rule and are therefore inherently unstable. The
only reason this hasn't been a problem up to now is that the javafx.swing
module contains a single test class. I will file a new test bug to address it
-- probably by moving that test to the `systemTests` project.
You will need to move your test into the `systemTests` project under the
`tests/system/` directory. Such tests are valid in the system tests project
because we run each test in that project in its own VM. Once your proposed test
is robust (meaning consistently catches the bug without your fix and
consistently passes with your fix) on all platforms without using Robot, then
`tests/system/src/test/java/test/javafx/embed/swing/` would be the best place
to put your new test.
Regarding the test itself, it still doesn't fail for me on Windows without your
fix. I ran the test program attached to the bug and I see something that might
help explain this. That test program creates a pair of JFXPanel objects and
adds both to the JPanel. If I first click on the first one, then it only shows
a single click. Every time after that, if I click on a new JFXPanel, then I get
2 clicks. If, instead, I click on the second JFXPanel right after starting the
program, I get 2 clicks the first time. With that in mind, I modified your test
to add a dummy JFXPanel to the JPanel before the one you are sending the mouse
pressed event to, and then it then does what I expect: it catches the bug
without your fix and passes with your fix. That might help you come up with a
more robust test. I added some specific comments on the test as well.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/SwingFXUtilsTest.java
line 46:
> 45:
> 46:
> 47: public class SwingFXUtilsTest {
Your proposed fix for this test class is not the right fix, and needs to be
reverted. It highlights an existing bug with the swing tests, which I will
address in general comments.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 100:
> 99: jpanel.add(fxPnl);
> 100: jframe.setContentPane(jpanel);
> 101: jframe.setVisible(true);
You should call `jframe.pack()` here.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 128:
> 127:
> 128: Thread.sleep(100); // there should be no pressed event after the
> initial one. Let's wait for 0.1s and check again.
> 129: assertEquals(1, pressedEventCounter[0]);
I recommend using 500 msec so we don't risk missing a failing click on slower
systems.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 60:
> 59: public static void doSetupOnce() {
> 60: if (setupIsDone) return;
> 61: Platform.startup(() -> {
This is not needed if you revert the changes to the other test class.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 130:
> 129: assertEquals(1, pressedEventCounter[0]);
> 130: }
> 131: }
You will want to add a cleanup method, annotated with `@AfterClass` to call
`Platform.exit` and dispose of the JFrame.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 106:
> 105: Scene scene = new Scene(new Group());
> 106: scene.getRoot().requestFocus();
> 107:
I don't think requesting focus is necessary (or desired).
Changes requested by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 12 Nov 2019 10:35:36 GMT, Florian Kirmaier
wrote:
> On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
>
>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
>> 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
>>
>> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 457:
>>
>>> 456:
>>> 457: sendMouseEventToFX(e);
>>> 458: super.processMouseEvent(e);
>>
>> typo: save --> safe
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 2:
>>
>>> 1: /*
>>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
>>> reserved.
>>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>
>> replace `2014, 2016` with `2019,`
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 28:
>>
>>> 27:
>>> 28: import javafx.application.Application;
>>> 29: import javafx.application.Platform;
>>
>> Remove unused import (several of the below imports are similarly unused).
>> Also, since this is a new test, please sort the imports.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 48:
>>
>>> 47:
>>> 48: import static junit.framework.Assert.assertEquals;
>>> 49: import static org.junit.Assert.assertNotNull;
>>
>> This method should be imported from `org.junit` package.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 116:
>>
>>> 115: MouseEvent e = new MouseEvent(fxPnl,
>>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
>>> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
>>> 117:
>>
>> This doesn't appear to trigger the bug -- at least on Windows. The test
>> passes for me with or without your fix. You might consider moving it to the
>> system tests project, under `tests/system/src/test/java/test/robot` and
>> using `Robot` to effect the mouse press.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 57:
>>
>>> 56:
>>> 57:
>>> 58: @BeforeClass
>>
>> You can remove the extra blank line.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 65:
>>
>>> 64:
>>> 65:
>>> 66: try {
>>
>> You can remove the extra blank line.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 79:
>>
>>> 78:
>>> 79: class TestFXPanel extends JFXPanel {
>>> 80: protected void processMouseEventPublic(MouseEvent e) {
>>
>> I think this can be a static class.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 88:
>>
>>> 87:
>>> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
>>> 89:
>>
>> This can be final.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 91:
>>
>>> 90: // It's an array, so we can mutate it inside of lambda statement
>>> 91: int[] pressedEventCounter = {0};
>>> 92:
>>
>> This can be final.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 132:
>>
>>> 131:
>>> 132:
>>> 133: }
>>
>> You can remove the extra blank line.
>>
>> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
>> line 123:
>>
>>> 122:
>>> 123: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS))
>>> {
>>> 124: throw new Exception();
>>
>> Add a space after the `if`.
>>
>> The fix seems fine. Have you tested it on all three platforms?
>>
>> I have several comments on the test.
>>
>>
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> 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
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 457:
>
>> 456:
>> 457: sendMouseEventToFX(e);
>> 458: super.processMouseEvent(e);
>
> typo: save --> safe
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
>> reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> replace `2014, 2016` with `2019,`
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 28:
>
>> 27:
>> 28: import javafx.application.Application;
>> 29: import javafx.application.Platform;
>
> Remove unused import (several of the below imports are similarly unused).
> Also, since this is a new test, please sort the imports.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 48:
>
>> 47:
>> 48: import static junit.framework.Assert.assertEquals;
>> 49: import static org.junit.Assert.assertNotNull;
>
> This method should be imported from `org.junit` package.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 116:
>
>> 115: MouseEvent e = new MouseEvent(fxPnl,
>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
>> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
>> 117:
>
> This doesn't appear to trigger the bug -- at least on Windows. The test
> passes for me with or without your fix. You might consider moving it to the
> system tests project, under `tests/system/src/test/java/test/robot` and using
> `Robot` to effect the mouse press.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 57:
>
>> 56:
>> 57:
>> 58: @BeforeClass
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 65:
>
>> 64:
>> 65:
>> 66: try {
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 79:
>
>> 78:
>> 79: class TestFXPanel extends JFXPanel {
>> 80: protected void processMouseEventPublic(MouseEvent e) {
>
> I think this can be a static class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 88:
>
>> 87:
>> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
>> 89:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 91:
>
>> 90: // It's an array, so we can mutate it inside of lambda statement
>> 91: int[] pressedEventCounter = {0};
>> 92:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 132:
>
>> 131:
>> 132:
>> 133: }
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 123:
>
>> 122:
>> 123: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
>> 124: throw new Exception();
>
> Add a space after the `if`.
>
> The fix seems fine. Have you tested it on all three platforms?
>
> I have several comments on the test.
>
>
>
> Disapproved by kcr (Lead).
Maybe try `./gradlew -PFULL_TEST=true swing:clean swing:test`.
I'm sure, it can be reproduced on windows.
If you still can not reproduce it, then I will add a test for the Robot.
PR: https:
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Wed, 6 Nov 2019 08:30:47 GMT, Kevin Rushforth wrote:
> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
> 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
>
> modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 457:
>
>> 456:
>> 457: sendMouseEventToFX(e);
>> 458: super.processMouseEvent(e);
>
> typo: save --> safe
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
>> reserved.
>> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>
> replace `2014, 2016` with `2019,`
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 28:
>
>> 27:
>> 28: import javafx.application.Application;
>> 29: import javafx.application.Platform;
>
> Remove unused import (several of the below imports are similarly unused).
> Also, since this is a new test, please sort the imports.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 48:
>
>> 47:
>> 48: import static junit.framework.Assert.assertEquals;
>> 49: import static org.junit.Assert.assertNotNull;
>
> This method should be imported from `org.junit` package.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 116:
>
>> 115: MouseEvent e = new MouseEvent(fxPnl,
>> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
>> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
>> 117:
>
> This doesn't appear to trigger the bug -- at least on Windows. The test
> passes for me with or without your fix. You might consider moving it to the
> system tests project, under `tests/system/src/test/java/test/robot` and using
> `Robot` to effect the mouse press.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 57:
>
>> 56:
>> 57:
>> 58: @BeforeClass
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 65:
>
>> 64:
>> 65:
>> 66: try {
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 79:
>
>> 78:
>> 79: class TestFXPanel extends JFXPanel {
>> 80: protected void processMouseEventPublic(MouseEvent e) {
>
> I think this can be a static class.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 88:
>
>> 87:
>> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
>> 89:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 91:
>
>> 90: // It's an array, so we can mutate it inside of lambda statement
>> 91: int[] pressedEventCounter = {0};
>> 92:
>
> This can be final.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 132:
>
>> 131:
>> 132:
>> 133: }
>
> You can remove the extra blank line.
>
> modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
> line 123:
>
>> 122:
>> 123: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
>> 124: throw new Exception();
>
> Add a space after the `if`.
>
> The fix seems fine. Have you tested it on all three platforms?
>
> I have several comments on the test.
>
>
>
> Disapproved by kcr (Lead).
You can run the AWT_TESTS with the following statement:
./gradlew -PFULL_TEST=true swing:clean swing:test
For some reason, it's hidden behind this flag.
Maybe that's the reason, why you coul
Re: [Rev 02] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - 5cd96a56: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/85c87628..5cd96a56 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.01-02 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 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 PR: https://git.openjdk.java.net/jfx/pull/25
Re: [Rev 01] RFR: 8200224: Multiple press event when JFXPanel gains focus
The pull request has been updated with additional changes. Added commits: - 85c87628: JDK-8200224 Changes: - all: https://git.openjdk.java.net/jfx/pull/25/files - new: https://git.openjdk.java.net/jfx/pull/25/files/314e423c..85c87628 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/25/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/25/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8200224 Stats: 52 lines in 3 files changed: 8 ins; 34 del; 10 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 PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier
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
modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java line 457:
> 456:
> 457: sendMouseEventToFX(e);
> 458: super.processMouseEvent(e);
typo: save --> safe
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 2:
> 1: /*
> 2: * Copyright (c) 2014, 2016 Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
replace `2014, 2016` with `2019,`
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 28:
> 27:
> 28: import javafx.application.Application;
> 29: import javafx.application.Platform;
Remove unused import (several of the below imports are similarly unused). Also,
since this is a new test, please sort the imports.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 48:
> 47:
> 48: import static junit.framework.Assert.assertEquals;
> 49: import static org.junit.Assert.assertNotNull;
This method should be imported from `org.junit` package.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 116:
> 115: MouseEvent e = new MouseEvent(fxPnl,
> MouseEvent.MOUSE_PRESSED, 0, MouseEvent.BUTTON1_DOWN_MASK,
> 116: 5, 5, 1, false, MouseEvent.BUTTON1);
> 117:
This doesn't appear to trigger the bug -- at least on Windows. The test passes
for me with or without your fix. You might consider moving it to the system
tests project, under `tests/system/src/test/java/test/robot` and using `Robot`
to effect the mouse press.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 57:
> 56:
> 57:
> 58: @BeforeClass
You can remove the extra blank line.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 65:
> 64:
> 65:
> 66: try {
You can remove the extra blank line.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 79:
> 78:
> 79: class TestFXPanel extends JFXPanel {
> 80: protected void processMouseEventPublic(MouseEvent e) {
I think this can be a static class.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 88:
> 87:
> 88: CountDownLatch firstPressedEventLatch = new CountDownLatch(1);
> 89:
This can be final.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 91:
> 90: // It's an array, so we can mutate it inside of lambda statement
> 91: int[] pressedEventCounter = {0};
> 92:
This can be final.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 132:
> 131:
> 132:
> 133: }
You can remove the extra blank line.
modules/javafx.swing/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
line 123:
> 122:
> 123: if(!firstPressedEventLatch.await(5000, TimeUnit.MILLISECONDS)) {
> 124: throw new Exception();
Add a space after the `if`.
The fix seems fine. Have you tested it on all three platforms?
I have several comments on the test.
Disapproved by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 15:59:58 GMT, Kevin Rushforth wrote: > On Tue, 29 Oct 2019 15:48:46 GMT, Florian Kirmaier > wrote: > >> On Tue, 29 Oct 2019 14:47:19 GMT, Kevin Rushforth wrote: >> >>> On Tue, 29 Oct 2019 14:45:46 GMT, Kevin Rushforth wrote: >>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier 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 @FlorianKirmaier you still need to file a JBS issue to associate your GitHub username with your OpenJDK user ID as noted [above](#issuecomment-546883472), and per the instructions in [this message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-September/023558.html) sent to openjfx-dev. >>> >>> @prsadhuk please review this. I will also review it (this will need two >>> reviewers). >> >> @kevinrushforth >> If created the ticket a moment ago. >> https://bugs.openjdk.java.net/browse/JDK-8233121 >> Is this okay, or is any information missing? > >> https://bugs.openjdk.java.net/browse/JDK-8233121 > > It was created in the JDK Project rather than the SKARA project (odd...the > link should have filled in the right project and component). I fixed it. Great, thanks. Some more notes on the importance of this Change. A project which uses a lot of JFXPanels for small components was basically unusable because the majority of clicks were registered as a double click. This made the software basically unusable with JavaFX11. PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 15:48:46 GMT, Florian Kirmaier wrote: > On Tue, 29 Oct 2019 14:47:19 GMT, Kevin Rushforth wrote: > >> On Tue, 29 Oct 2019 14:45:46 GMT, Kevin Rushforth wrote: >> >>> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier >>> 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 >>> >>> @FlorianKirmaier you still need to file a JBS issue to associate your >>> GitHub username with your OpenJDK user ID as noted >>> [above](#issuecomment-546883472), and per the instructions in [this >>> message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-September/023558.html) >>> sent to openjfx-dev. >> >> @prsadhuk please review this. I will also review it (this will need two >> reviewers). > > @kevinrushforth > If created the ticket a moment ago. > https://bugs.openjdk.java.net/browse/JDK-8233121 > Is this okay, or is any information missing? > https://bugs.openjdk.java.net/browse/JDK-8233121 It was created in the JDK Project rather than the SKARA project (odd...the link should have filled in the right project and component). I fixed it. PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 14:47:19 GMT, Kevin Rushforth wrote: > On Tue, 29 Oct 2019 14:45:46 GMT, Kevin Rushforth wrote: > >> On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier >> 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 >> >> @FlorianKirmaier you still need to file a JBS issue to associate your GitHub >> username with your OpenJDK user ID as noted >> [above](#issuecomment-546883472), and per the instructions in [this >> message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-September/023558.html) >> sent to openjfx-dev. > > @prsadhuk please review this. I will also review it (this will need two > reviewers). @kevinrushforth If created the ticket a moment ago. https://bugs.openjdk.java.net/browse/JDK-8233121 Is this okay, or is any information missing? PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 14:45:46 GMT, Kevin Rushforth wrote: > On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier > 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 > > @FlorianKirmaier you still need to file a JBS issue to associate your GitHub > username with your OpenJDK user ID as noted [above](#issuecomment-546883472), > and per the instructions in [this > message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-September/023558.html) > sent to openjfx-dev. @prsadhuk please review this. I will also review it (this will need two reviewers). PR: https://git.openjdk.java.net/jfx/pull/25
Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier 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 @FlorianKirmaier you still need to file a JBS issue to associate your GitHub username with your OpenJDK user ID as noted [above](#issuecomment-546883472), and per the instructions in [this message](https://mail.openjdk.java.net/pipermail/openjfx-dev/2019-September/023558.html) sent to openjfx-dev. PR: https://git.openjdk.java.net/jfx/pull/25
RFR: 8200224: Multiple press event when JFXPanel gains focus
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 PR: https://git.openjdk.java.net/jfx/pull/25
