Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8200224: Multiple press event when JFXPanel gains focus #25

Closed
wants to merge 14 commits into from

Conversation

@FlorianKirmaier
Copy link
Member

FlorianKirmaier commented Oct 28, 2019

Original PR: javafxports/openjdk-jfx#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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JDK-8200224: First mouse press each time JFXPanel gains focus is triggered twice

Approvers

  • Kevin Rushforth (kcr - Reviewer) Note! Review applies to 24385eb
  • Prasanta Sadhukhan (psadhukhan - Reviewer) Note! Review applies to 24385eb
This pull request introduces a new fix for the problem JDK-8200224.
Instead of simulating new input-events, we set JavaFX Scene/Window to focused.
We do so, by using the metho 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.
Added a unit-test
@bridgekeeper bridgekeeper bot added the oca label Oct 28, 2019
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2019

Hi FlorianKirmaier, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user FlorianKirmaier" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Oct 28, 2019

/signed

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 28, 2019

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@bridgekeeper bridgekeeper bot added oca-verify and removed oca oca-verify labels Oct 28, 2019
@openjdk openjdk bot added the rfr label Oct 29, 2019
@mlbridge
Copy link

mlbridge bot commented Oct 29, 2019

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 29, 2019

@FlorianKirmaier you still need to file a JBS issue to associate your GitHub username with your OpenJDK user ID as noted above, and per the instructions in this message sent to openjfx-dev.

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 29, 2019

@prsadhuk please review this. I will also review it (this will need two reviewers).

@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Oct 29, 2019

@kevinrushforth
If created the ticket a moment ago. https://bugs.openjdk.java.net/browse/JDK-8233121
Is this okay, or is any information missing?

@kevinrushforth
Copy link
Member

kevinrushforth commented Oct 29, 2019

https://bugs.openjdk.java.net/browse/JDK-8233121

It was created in the JDK Project rather than the SKARA project (odd...the link should have filled in the right project and component). I fixed it.

@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Oct 29, 2019

Great, thanks.

Some more notes on the importance of this Change.
A project which uses a lot of JFXPanels for small components was basically unusable because the majority of clicks were registered as a double click. This made the software basically unusable with JavaFX11.

Copy link
Member

kevinrushforth left a comment

The fix seems fine. Have you tested it on all three platforms?

I have several comments on the test.

cleaned up code
the unit tests are now stable.
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Nov 12, 2019

You can run the AWT_TESTS with the following statement:

./gradlew -PFULL_TEST=true swing:clean swing:test

For some reason, it's hidden behind this flag.
Maybe that's the reason, why you couldn't reproduce the bug?
As fas as I can thee, the swing-tests are stable, so there is no real reason to hide it behind a flag.
Maybe that's something which should be changed?

The tests are now stable. The previous version had some problems because the other test for Swing was calling System.exit.

The fix is well tested under Windows and Mac but not on Linux. The tests are based on a backport to an older JavaFX-version.

@FlorianKirmaier FlorianKirmaier requested a review from kevinrushforth Nov 12, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 13, 2019

I'll try it again with the updated test.

You can run the AWT_TESTS with the following statement:

./gradlew -PFULL_TEST=true swing:clean swing:test

Yes, I know that it needs -PFULL_TEST=true, but the earlier test wasn't failing for me. And yes, it is intentional; for reasons I can't recall, the swing tests don't work in headless mode. Anyway, I don't want to revisit this right now.

The tests are now stable. The previous version had some problems because the other test for Swing was calling System.exit.

Tests should not call System.exit. If they do, then that's almost certainly a bug.

Copy link
Member

kevinrushforth left a comment

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.

@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 13, 2019

FYI, I filed JDK-8234110 to move the existing :swing test to :systemTests.

Reverted the changes to SwingFXUtilsTest
Moved JFXPanelTest to the system tests. The new test is now part of the already existing test class.
Removed requestFocus from the unittest.
Copy link
Member

kevinrushforth left a comment

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)
Moved JFXPanel from test/robot to test/javafx
Some minor cleanups
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Nov 21, 2019

I've done now the requested changes.
Now only retesting it with Windows is left.

@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Nov 25, 2019

I've now tested on Windows.
I can confirm, that the test doesn't reproduce the error on Windows.

@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 25, 2019

Yes, I confirmed this last Friday, but ran out of time to reply. You also need to restore the test that you moved from test.robot.* to its original state (so we preserve the previous Mac-only test).

Copy link
Member

kevinrushforth left a comment

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.
simplified code based on code-review
simplified code based on code-review
Readded JFXPanelTest
readded removed line
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Nov 26, 2019

  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. : )
@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 26, 2019

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.

Copy link
Member

kevinrushforth left a comment

All looks good now. As a reminder, this will need a second reviewer.

@openjdk openjdk bot removed the rfr label Nov 27, 2019
@openjdk
Copy link

openjdk bot commented Nov 27, 2019

@FlorianKirmaier This change can now be integrated. The commit message will be:

8200224: Multiple press event when JFXPanel gains focus

Reviewed-by: kcr, psadhukhan
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 11 commits pushed to the master branch:

  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors
  • aad1720: 8233420: Upgrade to gcc 8.3 on Linux
  • 42040c4: 8232063: Upgrade gradle to version 6.0
  • aab07a4: 8234239: [TEST_BUG] Reenable few ignored web tests
  • 95ad601: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
  • 3e0557a: 8234303: [TEST_BUG] Correct ignore tag in graphics unit tests

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @prsadhuk) but any other Committer may sponsor as well.

  • To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
@openjdk openjdk bot added the ready label Nov 27, 2019
// requestFocus() so that 2nd mouse press will be honoured
// since now fx have focus
jfxPanelIOP.postEvent(this, e);
// This fixes JDK-8087914 without causing JDK-8200224

This comment has been minimized.

@prsadhuk

prsadhuk Nov 27, 2019 Collaborator

We actually do not clutter source code with bugids, it will be good if it can be removed with proper comments maybe something like "extra simulated mouse pressed event is removed by making the JavaFX scene focused"

This comment has been minimized.

@kevinrushforth

kevinrushforth Nov 27, 2019 Member

In general, we no longer reference bug IDs in source code, so I agree with the recommendation to reword the comment.

@FlorianKirmaier - can you reword as suggested? Once you are done, you can integrate it (using the /integrate command), and I will sponsor it. There will likely be a delay due to the US Thanksgiving holiday.

This comment has been minimized.

@FlorianKirmaier

FlorianKirmaier Nov 27, 2019 Author Member

Done

Copy link
Collaborator

prsadhuk left a comment

Looks good albeit with minor suggestion regarding presence of bugid in comment

updated comment based on codereview
@FlorianKirmaier
Copy link
Member Author

FlorianKirmaier commented Nov 27, 2019

/integrate

@openjdk
Copy link

openjdk bot commented Nov 27, 2019

@FlorianKirmaier
Your change (at version fa465c5) is now ready to be sponsored by a Committer.

@openjdk openjdk bot added the sponsor label Nov 27, 2019
@kevinrushforth
Copy link
Member

kevinrushforth commented Nov 27, 2019

/sponsor

@openjdk openjdk bot closed this Nov 27, 2019
@openjdk openjdk bot added integrated and removed sponsor ready labels Nov 27, 2019
@openjdk
Copy link

openjdk bot commented Nov 27, 2019

@kevinrushforth @FlorianKirmaier The following commits have been pushed to master since your change was applied:

  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md
  • 4d3c723: 8234593: Mark LeakTest.testGarbageCollectability as unstable
  • 5a39824: 8234056: Upgrade to libxslt 1.1.34
  • 8bea7b7: 8229472: Deprecate for removal JavaBeanXxxPropertyBuilders constructors
  • aad1720: 8233420: Upgrade to gcc 8.3 on Linux
  • 42040c4: 8232063: Upgrade gradle to version 6.0
  • aab07a4: 8234239: [TEST_BUG] Reenable few ignored web tests
  • 95ad601: 8233421: Upgrade to Visual Studio 2017 version 15.9.16
  • 3e0557a: 8234303: [TEST_BUG] Correct ignore tag in graphics unit tests

Your commit was automatically rebased without conflicts.

Pushed as commit 1d670f1.

@mlbridge
Copy link

mlbridge bot commented Nov 27, 2019

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: 1d670f1
Author: Florian Kirmaier
Committer: Kevin Rushforth
Date: 2019-11-27 16:20:12 +0000
URL: https://git.openjdk.java.net/jfx/commit/1d670f18

8200224: Multiple press event when JFXPanel gains focus

Reviewed-by: kcr, psadhukhan

! modules/javafx.swing/src/main/java/javafx/embed/swing/JFXPanel.java

  • tests/system/src/test/java/test/javafx/embed/swing/JFXPanelTest.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.