On Wed, 23 Apr 2025 09:46:06 GMT, Thiago Milczarek Sayao <tsa...@openjdk.org> wrote:
>> This is a continuation to >> [JDK-8236651](https://bugs.openjdk.org/browse/JDK-8236651) and it aims to >> stabilize the linux glass gtk backend. >> >> >> Overall, it has been made more robust within its scope, particularly in >> terms of sizing, positioning, and state management. >> >> List of changes: >> 1. It embraces the asynchronous nature of X11 by reporting geometry changes >> only upon receiving a configure event, rather than immediately as before. >> This is because it merely requests changes from the window manager, which >> may or may not honor them. However, it still reports changes immediately in >> certain special cases, such as when the window has not yet been realized >> (i.e., when the window has not actually been created yet). One scenario >> where this behavior is evident is when we request the window to move to >> position (0, 0), but the window manager instead places it in the top-right >> corner where panels converge. >> 2. FullScreen now keeps track of geometry changes and apply them on restore >> as documented on Stage.java. No geometry changes affects the FullScreen >> state; >> 3. States (fullscreen, maximized and iconify) are now reported back to Java >> when it actually happens rather than immediately (except when not realized); >> 4. When a window is maximized, it will ignore geometry changes and restore >> to the geometry it had prior to being maximized. After some testing, I >> believe this is the best behavior for platform compatibility; >> 5. Unifies the WindowContext class: previously, there were three separate >> classes—two of which (for applets and Java Web Start) were removed, leaving >> only one. However, the supporting infrastructure was still there partially. >> [Unify WindowContext in >> glass-gtk](https://bugs.openjdk.org/browse/JDK-8305768) >> 6. `setAlpha` and `setBackground` were removed because they no longer >> function correctly due to the lack of shared rendering. It's not possible to >> paint a background using GTK and then render custom content on top of it, >> unless the rendering is done by Gtk (it is not). It is possible to keep the >> background by painting with cairo, and make it work with software rendering, >> but that's a very remote scenario; >> 7. Tests were added and re-enabled to ensure everything works correctly. The >> stage tests now cover various StageStyle configurations, as I found that >> `DECORATED` stages often behave differently from `UNDECORATED` or `UTILITY` >> stages; >> 8. Added Logs for debugging. Enable it with ` -PCONF=DebugNative`; >> 9. It no longer keeps track of ge... > > Thiago Milczarek Sayao has updated the pull request incrementally with one > additional commit since the last revision: > > Reenable RestoreSceneSizeTest (JDK-8353556) I did a rough pass on macOS and Windows. There's some failures in the StageLocation and StageAttributes tests that I haven't had time to look into. You might want to consider adding a few delay constants and using them instead of the 500's you've scattered through these tests. There's the delay needed for big state changes (like entering and exiting fullscreen) and that needs to be 500 or more. Then there's the delay for waiting for a layout pulse and I'm assuming that could be significantly shorter. There seem to be places where you compare an attribute like Stage.getWidth() against a constant using strict equality and other places where you provide a tolerance delta. I suspect you want a delta in more locations but someone else will have to chime in on that. I think this mostly affects Windows machines using fractional scaling. There was a discussion a while back about naming conventions and I think the consensus was that new tests should not have "test" in the name (so testMinSize should be minSize or minStageSize). But I might be wrong on that and in the end it's a matter of style. tests/system/src/test/java/test/javafx/stage/FullScreenTest.java line 123: > 121: mode = EnumSource.Mode.INCLUDE, > 122: names = {"DECORATED", "UNDECORATED", "TRANSPARENT"}) > 123: void testUnFullScreenChangedSize(StageStyle stageStyle) { According to the spec changes to the size or position of a window while it's in fullscreen mode will be ignored and applied after the window leaves fullscreen mode. That's not how it currently works on macOS or Windows 11. Actually implementing that part of the spec would be complicated and probably not worth the development cycles. I would rather remove that wording and drop the testUnFullScreenChangeSize and Position tests. I imagine this was easy to implement back when fullscreen was implemented as a separate window. It's not clear this is useful behavior, the spec might just have captured an implementation detail and elevated it to a feature. tests/system/src/test/java/test/javafx/stage/SizingTest.java line 136: > 134: mode = EnumSource.Mode.INCLUDE, > 135: names = {"DECORATED", "UNDECORATED", "TRANSPARENT"}) > 136: void testMaximizeMaxSize(StageStyle stageStyle) { This test assumes that maximizing a window will ignore the max size settings. That's not how it works on macOS and it would be a bear to try to implement this. I think we should remove this test. tests/system/src/test/java/test/javafx/stage/SizingTest.java line 164: > 162: mode = EnumSource.Mode.INCLUDE, > 163: names = {"DECORATED", "UNDECORATED", "TRANSPARENT"}) > 164: void testFullScreenMaxSize(StageStyle stageStyle) { You're assuming that fullscreen mode should ignore the max size properties. That's not how Windows 11 and macOS work. On macOS the window expands to the max size and is then centered on a black screen. On Windows 11 it ends up in the upper left corner with the desktop showing beneath it. It looks weird but I don't think there's much point to changing the implementation on these two platforms. tests/system/src/test/java/test/javafx/stage/SizingTest.java line 262: > 260: mode = EnumSource.Mode.INCLUDE, > 261: names = {"DECORATED", "UNDECORATED", "TRANSPARENT", > "UTILITY"}) > 262: void testMinSize(StageStyle stageStyle) { Yikes! This test uncovers a long-standing problem and is failing on macOS and Windows. In general glass will only send a resize notification if the window's size actually changes. When you set the width and height here JavaFX records the new values, sends them on to glass, and glass doesn't respond because the window size doesn't change. The property values do not get corrected to reflect the actual window size and the test fails. (I am not a fan of the JavaFX property-based API for setting window attributes. Basically JavaFX records an attribute change as if it has already happened and THEN sends it on to glass to be acted on. This creates endless complications like this.) tests/system/src/test/java/test/robot/javafx/stage/StageLocationTest.java line 52: > 50: private static final int TO_Y = 500; > 51: private static final Color COLOR = Color.RED; > 52: private static final double TOLERANCE = 0.00; You probably want a non-zero tolerance for color matching. ------------- PR Review: https://git.openjdk.org/jfx/pull/1789#pullrequestreview-2788538337 PR Review Comment: https://git.openjdk.org/jfx/pull/1789#discussion_r2056780346 PR Review Comment: https://git.openjdk.org/jfx/pull/1789#discussion_r2058792439 PR Review Comment: https://git.openjdk.org/jfx/pull/1789#discussion_r2058518481 PR Review Comment: https://git.openjdk.org/jfx/pull/1789#discussion_r2058818935 PR Review Comment: https://git.openjdk.org/jfx/pull/1789#discussion_r2058831795
