-
Notifications
You must be signed in to change notification settings - Fork 478
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
8324239: JFXPanelHiDPITest fails on Windows 11 #1344
Conversation
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
Webrevs
|
Guess better will be to use Platform.runAndWait for scene to be created |
is there a reason this test is skipped on macOS? |
It's unrelated to this PR, but to answer your question, the test is Windows-specific by it's use of setting |
I wish the test class had a description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
Are there any other places where we invokeLater() instead of invokeAndWait()?
And runLater() instead of runAndWait() ?
@prsadhuk This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 22 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Good question. There might be, but each would need to be looked into to see whether they are causing problems. Also, when doing anything with Swing interop you need to be careful to not introduce a deadlock. It's fine in this specific case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test runs correctly now. Tests should generally use Util.runAndWait
instead of PlatformImpl.runAndWait
.
@@ -149,7 +151,7 @@ public MyApp() { | |||
} | |||
|
|||
private void createScene(final JFXPanel fxPanel) { | |||
Platform.runLater(() -> { | |||
PlatformImpl.runAndWait(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we prefer test.util.Util::runAndWait
so that exceptions are propagated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
/integrate |
Going to push as commit 876d535.
Your commit was automatically rebased without conflicts. |
Test fails with
because scenePeer is not yet created as the test is run with invokeLater.
FIx is to make it run with invokeAndWait so that it waits for the scene to be created..
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1344/head:pull/1344
$ git checkout pull/1344
Update a local copy of the PR:
$ git checkout pull/1344
$ git pull https://git.openjdk.org/jfx.git pull/1344/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1344
View PR using the GUI difftool:
$ git pr show -t 1344
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1344.diff
Webrev
Link to Webrev Comment