[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [Rev 02] RFR: 8200224: Multiple press event when JFXPanel gains focus
From:       Florian Kirmaier <fkirmaier () openjdk ! java ! net>
Date:       2019-11-26 9:24:04
Message-ID: OOtWudeQGGucLlhtveVhV3ucZtAiR5OlcxIrunAbWFc=.873ee335-d4a8-4d74-a7f6-c33a8e15d95d () github ! com
[Download RAW message or body]

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

> On Tue, 12 Nov 2019 10:23:16 GMT, Florian Kirmaier <fkirmaier@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


[prev in list] [next in list] [prev in thread] [next in thread]