8251241: macOS: iconify property doesn't change after minimize when resizable is false

[FlorianKirmaier]

ticket: https://bugs.openjdk.java.net/browse/JDK-8251241

This small change fixing the minimize for mac.
Changes are property registered.
Calling setIconize(true) no longer resets to false.

---

Progress

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

Issue

• JDK-8251241: macOS: iconify property doesn't change after minimize when resizable is false

Reviewers

• Kevin Rushforth (kcr - Reviewer)
• Ambarish Rapte (arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/280/head:pull/280
$ git checkout pull/280

[bridgekeeper]

👋 Welcome back fkirmaier! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

[mlbridge]

Webrevs

• 02: Full - Incremental (7c9584f)
• 01: Full - Incremental (1f42ceb)
• 00: Full (f89e43f)

[kevinrushforth]

/reviewers 2

[openjdk]

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[kevinrushforth]

JDK-8251241: macOS: iconify property doesn't change after minimize when resizable is false ⚠️ Title mismatch between PR and JBS.

Can you change the title to match the JBS title?

[FlorianKirmaier]

Done! I've changed the title.

[dlemmermann]

Thank you Florian!!! :-)

[kevinrushforth]

This looks good based on my initial testing, including a modified version of the test program that doesn't temporarily set resizable to true. Nice.

I want to do some additional testing.

I left one minor comment on the code formatting inline.

[FlorianKirmaier]

Thank you for the very quick feedback!
In the application, the fix was developed for, it also seems to fix all issues related to iconified.

[kevinrushforth]

You may have seen my comment in JBS: This PR also fixes JDK-8249202, which I discovered while testing JDK-8248490. This means that I already have an automated test that can be modified to test this. I just need to add a couple things to it (an assertion check for the iconified property, a check to ensure that we can deiconify the stage, and then add a mode to run it with a non-resizable stage). Once I verify it, I can send you the patch for the test, if that's OK with you?

[kevinrushforth]

Here is the patch for IconifyTest.java :

icontest.patch.txt

I verified that it catches the bugs, in that the modified test fails without the fix and passes with the fix.

Can you apply the patch to your repo and push a new commit with the updated test?

[FlorianKirmaier]

I've added your patch for the unit-test!
Nice to see it fixes more bugs. : )
Don't want to know how many hours were spent working around this bug by all the people around the world.
Compared to this, the fix was developed really quickly.

[kevinrushforth]

Looks good. Thanks for fixing this long-standing bug.

I ran a full set of unit test, including the newly modified one. All looks good.

Can we get one more reviewer on this?

[kevinrushforth]

@arapte can you be a second reviewer?

[openjdk]

@FlorianKirmaier This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8251241: macOS: iconify property doesn't change after minimize when resizable is false

Reviewed-by: kcr, arapte

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

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

• b25ffc7: 8251352: Many javafx.base classes have implicit no-arg constructors
• 2aed5ad: 8240969: WebView does not allow to load style sheet in modularized applications
• 1c54e61: 8241840: Memoryleak: Closed focused Stages are not collected with Monocle.
• f50d218: Merge
• 208d828: 8228570: Add various documentation clarifications
• 7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate b25ffc7ada679683dca74ba420dd77ade5ef8372.

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, @arapte) 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).

[FlorianKirmaier]

/integrate

[openjdk]

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

[kevinrushforth]

/sponsor

[openjdk]

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

• b25ffc7: 8251352: Many javafx.base classes have implicit no-arg constructors
• 2aed5ad: 8240969: WebView does not allow to load style sheet in modularized applications
• 1c54e61: 8241840: Memoryleak: Closed focused Stages are not collected with Monocle.
• f50d218: Merge
• 208d828: 8228570: Add various documentation clarifications
• 7a8708b: 8250799: NumberStringConverter and its subclasses are missing documentation for all their constructors

Your commit was automatically rebased without conflicts.

Pushed as commit 85821ca.