8364547: Window size may be incorrect when constrained to min or max

[beldenfox]

When changing the width and height of a window the platform code is responsible for enforcing the min and max size constraints. If the final width and height don't match the width and height passed into setBounds the platform needs to call notifyResize to correct the window's properties. This happens naturally if the window size actually changes since that will trigger the OS to send size change notifications. If the platform window size doesn't change the OS notifications won't trigger. We need to catch that case and send notifyResize anyway.

This PR does this for Mac and Windows. Linux is being handled in PR #1789 which also includes the system tests for these bugs.

---

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)

Issue

• JDK-8364547: Window size may be incorrect when constrained to min or max (Bug - P4)

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1870

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mfox! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (468fd354)
• 00: Full (1c452782)

[kevinrushforth]

This looks reasonable although I haven't tested it. I left one question inline. I presume you have run all systems tests?

[kevinrushforth]

Could this lead to two resize commands in some cases? I can see why this new logic is needed for the case where the window was already at max (or min) width and height, but if it wasn't already constrained, wouldn't the resize event have already happened?

[tsayao]

The issue is that the Java side updates the property first, and only afterward requests the Glass native side to apply the change. I've found this to cause many problems and I fixed the same way in #1789 - when the change can't be applied, it notifies back.

[beldenfox]

Could this lead to two resize commands in some cases?

Yes, I was trying not to be too clever with my checks. I figured if the second notification wasn't necessary it would be benign. At the very least it won't trigger invalidation of the window's width and height properties.

I will tighten this up since I have to tweak the code a bit anyway. I just verified that on Windows you can alter the size of a maximized window and the OS will keep it in the MAXIMIZED state (as it resizes glass calls notifyResize with WindowEvent.MAXIMIZED). This PR can kick the window out of the MAXIMIZED or MINIMIZED state incorrectly. Unfortunately it puts the window in the wrong internal state without updating the maximized or iconified properties so it's not easy to write a test to detect this.

[kevinrushforth]

This is pending work from you, right? I'll put it back on my review queue once you make the changes you mentioned above.

[beldenfox]

I've made the changes. It should be ready for review.

[kevinrushforth]

I completely missed that you had already done this. I'll review it then.

[kevinrushforth]

/reviewers 2

[openjdk]

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

[bridgekeeper]

@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[beldenfox]

/touch

[openjdk]

@beldenfox The pull request is being re-evaluated and the inactivity timeout has been reset.

[kevinrushforth]

The fix looks good.

Windows: I ran SizingTest from PR #1789 (removing the assumeTrue that skips the test on Windows), and it fails for me without this fix and passes with this fix.

macOS: I ran SizingTest from PR #1789, and I get 8 test failures without this fix and 4 failures with this fix. The 4 failures are all using StageStyle.EXTENDED, which might be a different bug.

@beldenfox Can you merge in the latest master (I did when testing and there were no issues)? And can you coment on the StageStyle.EXTENDED failures on macOS? I think it would be fine to treat them as a separate bug unless it is easy to fix.

I'll reapprove if you make changes.

[kevinrushforth]

@lukostyra or @andy-goryachev-oracle Can one of you be the second reviewer?

[beldenfox]

Thanks for the review. I'm traveling so won't be able to pick this up again until November.