[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window [v3]
From:       Kevin Rushforth <kcr () openjdk ! org>
Date:       2024-04-30 23:24:00
Message-ID: 16tHtx97hXBO0ouOygAIlrJvp12xtFkiuk4yWpTAfDw=.81eaabda-6c47-47f6-950b-464b35a61ee7 () github ! com
[Download RAW message or body]

On Sat, 9 Mar 2024 18:41:10 GMT, Marius Hanl <mhanl@openjdk.org> wrote:

> > This PR fixes the problem that maximizing/fullscreen a `Stage` or `Dialog` is \
> > broken when `sizeToScene()` was called before or after. 
> > The approach here is to ignore the `sizeToScene()` request when the `Stage` is \
> > maximized or set to fullscreen.  Otherwise the Window Manager of the OS will be \
> > confused and you will get weird flickering or wrong Window buttons (e.g. on \
> > Windows, the 'Maximize' button still shows the 'Restore' Icon, while we already \
> > resized the `Stage` to not be maximized).
> 
> Marius Hanl has updated the pull request incrementally with one additional commit \
> since the last revision: 
> improve tests

The fix looks good. The spec changes (updated javadocs) look good. Can you create the \
CSR for the spec change?

I have a couple overall comments:

* I wanted to verify different orders of operation, so I wrote a (manual) test \
                program and attached it to the JBS bug. It covers the following \
                cases:
    * set ; sizeToScene ; show
    * sizeToScene ; set ; show
    * show ; set ; sizeToScene
    * show ; sizeToScene ; set

I verified that the first 3 are broken today. All cases work with your fix. I think \
it might be a good idea to add automated tests for the different orderings.

* Please merge the latest master. Note that the calls to Util.shutdown in the tests \
must be fixed after this is done or they will no longer compile.

tests/system/src/test/java/test/javafx/stage/SizeToSceneFullscreenTest.java line 69:

> 67:     @AfterAll
> 68:     public static void teardown() {
> 69:         Util.shutdown(stage);

You will need to remove the `stage` argument after you merge the latest master.

tests/system/src/test/java/test/javafx/stage/SizeToSceneMaximizeTest.java line 69:

> 67:     @AfterAll
> 68:     public static void teardown() {
> 69:         Util.shutdown(stage);

You will need to remove the `stage` argument after you merge the latest master.

-------------

PR Review: https://git.openjdk.org/jfx/pull/1382#pullrequestreview-2032228167
PR Review Comment: https://git.openjdk.org/jfx/pull/1382#discussion_r1585330069
PR Review Comment: https://git.openjdk.org/jfx/pull/1382#discussion_r1585330320


[prev in list] [next in list] [prev in thread] [next in thread]