RFR: 8269639: [macos] Calling stage.setY(0) twice causes wrong popups location

Kevin Rushforth kcr at openjdk.java.net
Sat Jul 17 13:30:57 UTC 2021


On Sat, 17 Jul 2021 00:15:48 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

> As described in the issue, when the stage is moved to the screen top position, it is moved below the system menu bar. However, doing it twice doesn't trigger a native callback to the Java layer, and the stage yPosition doesn't get updated with the actual position of the application. This has several side effects, like the wrong popup control position.
> 
> This PR adds a callback in case set position doesn’t match actual position. 
> 
> It is only going to be called when the final position of the stage doesn't match the one that was set, which could happen in rare occasions, mainly due to constrains applied by the native layer, so it doesn't add any overhead.
> 
> A system test for MacOS is included.

The fix and the test looks good. I left one suggestion and two minor formatting comments on the test.

tests/system/src/test/java/test/javafx/stage/StageAtTopPositionTest.java line 25:

> 23:  * questions.
> 24:  */
> 25: package test.javafx.stage;

Minor: can you add a blank line before the `package` statement? (I realize we have a few existing classes that don't follow this)

tests/system/src/test/java/test/javafx/stage/StageAtTopPositionTest.java line 72:

> 70:             stage.setY(400);
> 71:             stage.addEventHandler(WindowEvent.WINDOW_SHOWN, e ->
> 72:                                     Platform.runLater(startupLatch::countDown));

Minor: indentation is a bit off if you intended to line it up.

tests/system/src/test/java/test/javafx/stage/StageAtTopPositionTest.java line 86:

> 84:             }
> 85:         } catch (InterruptedException ex) {
> 86:             fail("Unexpected exception: " + ex);

If you add `throws Exception` to the method, you can simplify this and replace the try/catch block with:


    assertTrue("Timeout waiting for FX runtime to start",
               startupLatch.await(15, TimeUnit.SECONDS));


(most of our newer tests do this)

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

PR: https://git.openjdk.java.net/jfx/pull/580


More information about the openjfx-dev mailing list