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

[Florian Kirmaier]

On Wed, 13 Nov 2019 21:53:13 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

> On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier <fkirmaier at openjdk.org> 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).

It's now removed

done

I've kept the logic of the other JFXPanelTest. Should i change it?

removed

I've removed it

PR: https://git.openjdk.java.net/jfx/pull/25