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: [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 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
