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

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8200224: Multiple press event when JFXPanel gains focus
From:       Kevin Rushforth <kcr () openjdk ! java ! net>
Date:       2019-11-26 13:14:46
Message-ID: 0WfZwq0WJvyFs8azelFiuwBKmxj222FRepOKWGlOgB0=.d9f659bf-aeb2-42d8-9ab1-8f452e5fe44c () github ! com
[Download RAW message or body]

On Tue, 26 Nov 2019 13:06:38 GMT, Florian Kirmaier <fkirmaier@openjdk.org> wrote:

> On Tue, 26 Nov 2019 09:24:01 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:
> 
> > On Tue, 29 Oct 2019 11:27:35 GMT, Florian Kirmaier \
> > <github.com+6547435+FlorianKirmaier@openjdk.org> 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


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