[Rev 03] RFR: 8200224: Multiple press event when JFXPanel gains focus

[Florian Kirmaier]

On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

> On Tue, 26 Nov 2019 09:23:58 GMT, Florian Kirmaier <fkirmaier at openjdk.org> 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