8354943: [Linux] Simplify and update glass gtk backend: window sizing, positioning, and state management issues #1789
Conversation
…ning, and state management issues
|
👋 Welcome back tsayao! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/reviewers 2 reviewers Reviewers: @lukostyra @kevinrushforth @beldenfox If you have time, your review would be appreciated as well. |
|
@kevinrushforth |
|
Thanks for adding these tests. I'm currently tracking down two bugs on macOS that should have been caught a long time ago. Looks like these tests cover those cases (finally). I will at least review and run the tests on macOS and report back. I might have time to run the tests on Windows 11. I don't have a Windows 10 box to test one. |
|
I'll give this a thorough read and report back. In the meantime, I recently was tackling JDK-8321624 and i have to wonder if it got fixed in the process of your changes? It is intermittent, I managed to get it to fail locally on my VM and it seemed like a race between window manager showing the window and us wanting to move it. Judging by the list of bugs you fixed this one could maybe also make he list. |
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
tests/system/src/test/java/test/robot/javafx/stage/StageLocationTest.java
Outdated
Show resolved
Hide resolved
…ng when unresizable) - Improve code on gtk_window.cpp - Add delta for sizing (as Martin suggested) - Add constants for wait times (as Martin suggested)
The Since I changed the positioning code and it does passes the tests, it might be fixed. |
|
Please wait a little before reviewing the glass GTK part, as I may have found a way to improve it further |
Good news! Problems were usually on slower environments ex. VMs. I would add this test to the list as well, and I'll verify for its stability during my review. |
tests/system/src/test/java/test/robot/javafx/stage/StageAttributesTest.java
Outdated
Show resolved
Hide resolved
- Notify maximize/fullscreen before show on native side
|
@tsayao this pull request can not be integrated into git checkout 8354943
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
- Remove work-around to allow unresizable Stages to be resized - Other minor adjustments
…er ignoring too often focus requests
- Minor adjustments
# Conflicts: # modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp # modules/javafx.graphics/src/main/native-glass/gtk/glass_general.h # modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp # modules/javafx.graphics/src/main/native-glass/gtk/glass_window.h
- Rename test methods
- Fix mistakes when merging
…ly when they change
|
I think I'm mostly done and this can be reviewed now. |
modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkView.java
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-glass/gtk/glass_general.cpp
Outdated
Show resolved
Hide resolved
modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp
Outdated
Show resolved
Hide resolved
|
I have updated the main explanation of the changes. I also ran extensive tests on XWayland, Xorg, KDE, GNOME, and across different OS versions and distributions. |
This is a continuation to JDK-8236651 and it aims to stabilize the linux glass gtk backend.
This is a refactor of the Glass GTK implementation, primarily focused on window size, positioning, and state management to resolve numerous issues.
The main change is that GtkWindow (which is a GtkWidget) has been replaced with a direct use of GdkWindow for windows. This makes sense because GTK includes its own rendering pipeline, whereas JavaFX renders directly to the underlying system window (such as the X11 window) via Prism ES2 using GL and GLX. Most GTK window-related calls have equivalent GDK calls. Since GDK serves as an abstraction over the system window and input events, it aligns better with the purposes of Glass. Additionally, using GTK required various workarounds to integrate with Glass, which are no longer necessary when using GDK directly.
It uses a simple C++ Observable to notify the Java side about changes. This approach is more straightforward, as notifications occur at many points and the previous implementation was becoming cluttered.
Previously, there were three separate context classes, two of which were used for Java Web Start and Applets. These have now been unified, as they are no longer required.
Many tests were added to ensure everything is working correctly. I noticed that some tests produced different results depending on the StageStyle, so they now use @ParameterizedTest to vary the StageStyle.
A manual test is also provided. I did not use MonkeyTester for this, as it was quicker to simply run and test manually:
java @build/run.args tests/manual/stage/TestStage.javaWhile this is focused on XWayland, everything works on pure Xorg as well.
List of fixed issues:
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1789/head:pull/1789$ git checkout pull/1789Update a local copy of the PR:
$ git checkout pull/1789$ git pull https://git.openjdk.org/jfx.git pull/1789/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1789View PR using the GUI difftool:
$ git pr show -t 1789Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1789.diff
Using Webrev
Link to Webrev Comment