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

8316419: [macos] Setting X/Y makes Stage maximization not work before show #1258

Closed
wants to merge 2 commits into from

Conversation

beldenfox
Copy link
Contributor

@beldenfox beldenfox commented Oct 12, 2023

When a window is visible the maximized, iconified, and fullscreen properties are two-way streets; changes made in Java are sent on to the platform window and changes in the platform window are sent back into Java.

When a window is hidden these properties (and others, like location and sizing information) are not sent on to the platform window until the window is made visible. In other words, the properties don't reflect the actual state of the window but the intended state after it's shown.

There's a period when the window is transitioning from hidden to shown where the core Stage code is using the stored properties to configure the platform window and the platform window is simultaneously calling back in to update the properties. This was causing the intended state to be wiped out before it could be sent on to the platform window.

The problem is specific to Mac because on that platform any change to the size or location of a window can cause it to enter or leave the maximized (zoomed) state. So just setting the location can cause the maximized flag to be altered.

The proposed solution is to copy the intended state bits to Stage local variables to be used later in the configuration.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

  • JDK-8316419: [macos] Setting X/Y makes Stage maximization not work before show (Bug - P4)
  • JDK-8255835: [macOS] Undecorated stage cannot be maximized (Bug - P4)
  • JDK-8305675: [macos] Stage set to iconified before being shown is displayed on screen (Bug - P3)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1258/head:pull/1258
$ git checkout pull/1258

Update a local copy of the PR:
$ git checkout pull/1258
$ git pull https://git.openjdk.org/jfx.git pull/1258/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1258

View PR using the GUI difftool:
$ git pr show -t 1258

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1258.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 12, 2023

👋 Welcome back beldenfox! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Ready for review label Oct 12, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 12, 2023

Webrevs

Copy link
Contributor

@lukostyra lukostyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on first sight. I'll verify it soon on all platforms.

Copy link
Contributor

@lukostyra lukostyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change causes a regression on Windows, test MaximizeUndecorated.testMaximized starts failing:

MaximizeUndecorated > testMaximize FAILED
    java.lang.AssertionError: Stage has moved to desktop top corner
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at test.javafx.stage.MaximizeUndecorated.testMaximize(MaximizeUndecorated.java:85)

@beldenfox
Copy link
Contributor Author

If the iconified, maximized, and fullscreen flags are set on a hidden window they aren't sent on to the platform until the window is shown. There are two stages:

a) before the window becomes visible the size and position is sent

b) after the window becomes visible the iconified, maximized, and fullscreen flags are sent

In the original bug the maximized and iconified properties were being altered by the platform code at the end of stage (a).

In this test case the maximized property is being set by user code between stages (a) and (b). That's when the setOnShown() action is executing.

Fun fact: on Mac the OS will report that an undecorated window is maximized (zoomed) even when it very definitely is not.

@beldenfox
Copy link
Contributor Author

Fix is now localized to the Mac. When an NSView is being initialized the isZoomed call returned bogus results. New code checks to see if the view's screen is set. For undecorated windows the OS assumed the zoomed size should be the same as the unzoomed size, not the size of the screen. This could also lead isZoomed to return surprising results.

Copy link
Contributor

@lukostyra lukostyra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lukostyra
Copy link
Contributor

I actually wanted to take a second look at Linux too, since JDK-8255835 also mentioned the bug happening on Linux. Will get back to it tomorrow morning.

@lukostyra
Copy link
Contributor

lukostyra commented Oct 26, 2023

/issue JDK-8255835

I can confirm this also solves JDK-8255835 - both MaximizeUndecorated test and the test program attached to the issue work as they are supposed to after these changes on both Mac and Linux.

EDIT: Right, I forgot only authors can do that. @beldenfox could you add JDK-8255835?

@openjdk
Copy link

openjdk bot commented Oct 26, 2023

@lukostyra Only the author (@beldenfox) is allowed to issue the /issue command.

@beldenfox
Copy link
Contributor Author

/issue JDK-8255835
/issue JDK-8305675

I'm also adding JDK-8305675 which covers setIconified. The bogus isZoomed results were causing the core to think that the window had been maximized and then restored and that last step wiped out both the maximized and iconified properties.

JDK-8305675 should be tested using the StartIconified test in tests/manual/stage/StartIconified.java. Despite what the test says the window should pop up briefly before iconifying. I have no idea why that isn't happening on Linux given the way the core code works.

If you de-iconify the window it might be blank. That also happens on Linux and is related to a timing issue. I'll make sure a bug is entered against it.

JDK-8305675 mentions exceptions raised by SceneChangeShouldNotFocusStageTest but that's not Mac-specific and has been spun off to another bug.

@openjdk
Copy link

openjdk bot commented Oct 26, 2023

@beldenfox
Adding additional issue to issue list: 8255835: [macOS] Undecorated stage cannot be maximized.

@openjdk
Copy link

openjdk bot commented Oct 26, 2023

@beldenfox
Adding additional issue to issue list: 8305675: [macos] Stage set to iconified before being shown is displayed on screen.

@kevinrushforth
Copy link
Member

@andy-goryachev-oracle Can you be the second reviewer?

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Oct 27, 2023

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@andy-goryachev-oracle
Copy link
Contributor

The test program StartIconified JDK-8305675 works the same regardless of the fix: the window flashes, then immediately gets minimized. Is this expected?
macOS 13.5.2

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Oct 31, 2023

The test program MaximizedStage JDK-8255835 : very strange - the window appears to be maximized beyond the visible area, the OS tool bar is on top and the dock is visible. It is impossible to minimize or normalize or quit the app via its toolbar.

Is this is acceptable?

Screenshot 2023-10-31 at 13 58 57

@andy-goryachev-oracle
Copy link
Contributor

SetXYAndMaximize JDK-8316419 works the same with or without the fix, the window appears maximized to fill the visible area (excl. the dock and the tool bar):

Screenshot 2023-10-31 at 14 02 37

Is it expected?

@beldenfox
Copy link
Contributor Author

The test program MaximizedStage JDK-8255835 : very strange - the window appears to be maximized beyond the visible area, the OS tool bar is on top and the dock is visible. It is impossible to minimize or normalize or quit the app via its toolbar.

The window is borderless. It's difficult to see what's going on with the original test file so I added a new one to the JBS. MaximizedBorderStage adds a thin red border to the edge of the scene so you can verify that the JavaFX window doesn't go under the toolbar, the top edge of the window is flush with the bottom edge of the toolbar.

SetXYAndMaximize JDK-8316419 works the same with or without the fix, the window appears maximized to fill the visible area (excl. the dock and the tool bar):

The test program StartIconified JDK-8305675 works the same regardless of the fix: the window flashes, then immediately gets minimized. Is this expected? macOS 13.5.2

When I run these two test programs using the current JavaFX master they both fail (the windows don't iconify or maximize) but they work correctly with this fix. This is on my Mac running 13.6 but the OS version shouldn't matter.

@andy-goryachev-oracle
Copy link
Contributor

MaximizedStage with a border is much cleaner, works as expected, thank you!

StartIconified_8305675 and SetXYAndMaximize_8316419 work the same with our without fix, I just double checked.
May be because I am running 13.5.2 on M1 silicon?

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless, the code works correctly now, so 🕶 LGTM

@openjdk
Copy link

openjdk bot commented Nov 1, 2023

@beldenfox 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:

8316419: [macos] Setting X/Y makes Stage maximization not work before show
8255835: [macOS] Undecorated stage cannot be maximized
8305675: [macos] Stage set to iconified before being shown is displayed on screen

Reviewed-by: lkostyra, angorya

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 18 new commits pushed to the master branch:

  • 72c052e: 8319231: Unrecognized "minimum" key in .jcheck/conf causes /reviewers command to be ignored
  • 6104113: 8317836: FX nodes embedded in JFXPanel need to track component orientation
  • a11faa9: 8319066: Application window not always activated in macOS 14 Sonoma
  • f0246b8: 8311216: DataURI can lose information in some charset environments
  • 9b93c96: 8205067: Resizing window with TextField hides text value
  • 1672960: 8318630: TextAreaBehaviorRobotTest.testNonMacBindings fails on Linux
  • 2aa69e0: 8318708: FX: Update copyright year in docs, readme files to 2024
  • bce15aa: 8318714: Update copyright header for files modified in 2023
  • 929035f: 8306083: Text.hitTest is incorrect when more than one Text node in TextFlow
  • de456da: 8317508: Provide media support for libavcodec version 60
  • ... and 8 more: https://git.openjdk.org/jfx/compare/73e690fc118c55d0c59fb567e884434a03243fff...master

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.

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 (@lukostyra, @andy-goryachev-oracle) 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 Ready to be integrated label Nov 1, 2023
@beldenfox
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Nov 1, 2023
@openjdk
Copy link

openjdk bot commented Nov 1, 2023

@beldenfox
Your change (at version 94fe8dc) is now ready to be sponsored by a Committer.

@andy-goryachev-oracle
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Nov 1, 2023

Going to push as commit f41e3ec.
Since your change was applied there have been 18 commits pushed to the master branch:

  • 72c052e: 8319231: Unrecognized "minimum" key in .jcheck/conf causes /reviewers command to be ignored
  • 6104113: 8317836: FX nodes embedded in JFXPanel need to track component orientation
  • a11faa9: 8319066: Application window not always activated in macOS 14 Sonoma
  • f0246b8: 8311216: DataURI can lose information in some charset environments
  • 9b93c96: 8205067: Resizing window with TextField hides text value
  • 1672960: 8318630: TextAreaBehaviorRobotTest.testNonMacBindings fails on Linux
  • 2aa69e0: 8318708: FX: Update copyright year in docs, readme files to 2024
  • bce15aa: 8318714: Update copyright header for files modified in 2023
  • 929035f: 8306083: Text.hitTest is incorrect when more than one Text node in TextFlow
  • de456da: 8317508: Provide media support for libavcodec version 60
  • ... and 8 more: https://git.openjdk.org/jfx/compare/73e690fc118c55d0c59fb567e884434a03243fff...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 1, 2023
@openjdk openjdk bot closed this Nov 1, 2023
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Nov 1, 2023
@openjdk
Copy link

openjdk bot commented Nov 1, 2023

@andy-goryachev-oracle @beldenfox Pushed as commit f41e3ec.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
4 participants