8354943: [Linux] Simplify and update glass gtk backend: window sizing, positioning, and state management issues#2139
8354943: [Linux] Simplify and update glass gtk backend: window sizing, positioning, and state management issues#2139tsayao wants to merge 42 commits into
Conversation
…, positioning, 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. |
Webrevs
|
|
We'll need to evaluate this approach vs that of #1789 and choose one or the other. It is accurate to say that the main difference between the two approaches is that PR #1789 replaces GtkWindow with GdkWindow whereas this PR sticks with GtkWindow? What other differences will help guide the reviewers? Does this PR address the same set of issues that PR #1789 does? That one lists 10 additional bugs as also being resolved by that PR. /reviewers 2 reviewers |
|
@kevinrushforth |
With the introduction of #2025, a By reverting to Experience shows that such changes can introduce regressions. Since this PR preserves the use of
At this time, the answer is likely yes, as all tests are passing on Ubuntu 24.04 with XWayland. I will follow up with additional validation and explicitly list the confirmed fixes. The decision between #1789 and this PR will ultimately depend on the outcome of these tests.
|
|
/issue add 8354943,8316425,8316891,8337400,8353612,8355073,8353556,8321624,8367893,8366009 |
|
@tsayao This issue is referenced in the PR title - it will now be updated. Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: Adding additional issue to issue list: |
- Improve tests to be more consistent - GdkWindow -> GtkWindow related fixes
- Prevent repeated 'a' being typed - Small fixes
# Conflicts: # modules/javafx.graphics/src/main/native-glass/gtk/glass_window.cpp
|
|
|
@tsayao should you add JDK-8365201 to the list too? Edit: oh sorry, I just read that this is an alternative to the other proposal. So the question is, does this fixes the above-mentioned issue too? |
I still need to set up a multi-monitor environment to properly test it, but it will probably fix this issue as well. |
|
/template append |
- Add test for view offset
|
I’m finishing up the final details and expect to submit the last commit soon, aside from review changes. |
|
The pull request is almost ready. I'm just dealing with a few remaining failing tests. |
Not a problem. If you want you can turn this PR into a Draft and then once you're done remove the Draft status, that way we'll know what's the situation with this PR and Skara should be a bit more relaxed when it comes to timing it out. |
# Conflicts: # modules/javafx.graphics/src/main/native-glass/gtk/glass_window.h
… fails intermittently" This reverts commit c6b9dd7.
|
All tests pass on Xorg and XWayland (primarily on Ubuntu 24.04). The new tests were also validated on macOS Tahoe. Additionally, extensive manual testing was conducted. |
| } | ||
|
|
||
| if (gdk_cursor) { | ||
| g_object_unref(gdk_cursor); |
There was a problem hiding this comment.
The gdk_cursor field is unref'd here and in the destructor of WindowContext, but _setCustomCursor() passes in a cursor owned by com.sun.glass.ui.Cursor on the Java side. This may potentially destroy a cursor that is still used on the Java side. If WindowContext needs to participate in refcounting, it should probably take its own g_object_ref() for incoming cursors.
There was a problem hiding this comment.
Looking at the GTK code, gdk_window_set_cursor() already unreferences the currently assigned cursor, so I removed the explicit unref. The custom cursor unref was causing a crash.
| } | ||
|
|
||
| glong to_screen = getScreenPtrForLocation(geometry.x, geometry.y); | ||
| glong to_screen = getScreenPtrForLocation(event->x, event->y); |
There was a problem hiding this comment.
getScreenPtrForLocation(), which calls gdk_screen_get_monitor_at_point(), expects coordinates "in the virtual screen". You pass in coordinates specified "relative to its parent", can you verify that this is intended?
There was a problem hiding this comment.
The parent of a toplevel or a popup (override-redirect) window on X11 is the root window which covers the entire screen. But I've found some problems while testing it and fixed it.
|
|
||
| #define ALPHA_CHANNEL_ERROR_MSG \ | ||
| "Can't create transparent stage, because your screen doesn't support alpha channel. " \ | ||
| "You need to enable XComposite extension.\n" |
There was a problem hiding this comment.
If you are touching this code, maybe you should consider improving the language. In particular, readers shouldn't be colloquially addressed with "you". Maybe something like:
"Cannot create a transparent stage because the screen does not support an alpha channel without enabling the XComposite extension."
There was a problem hiding this comment.
I updated the error message (though it's very unlikely to ever be shown).
| void invalidate() { | ||
| if (!onChange) return; | ||
| if (notifying) { | ||
| fprintf(stderr,"WARNING: Re-entrant call to Observable::invalidate() (possible cyclic dependency)\n"); |
There was a problem hiding this comment.
Should this use the LOG macro, or alternatively be hidden behind the VERBOSE flag?
There was a problem hiding this comment.
I think this is closer to an error than a log message. On the other hand, it's something that should be caught during development.
| view_size.set({boundsW, boundsH}); | ||
| } | ||
|
|
||
| if (mapped) { |
There was a problem hiding this comment.
This looks a bit suspicious: move_resize() uses mapped to decide whether to call gtk_window_resize() or gtk_window_set_default_size(). The old code used gtk_widget_get_realized() and resized any realized window. Since the mapped field is always false for a POPUP window, the new code might not resize an already-realized popup.
There was a problem hiding this comment.
Popups go through the same process of setting mapped to true. However, I'll do some testing to verify popup move and resize behavior.
There was a problem hiding this comment.
Only WindowContext::process_map() seems to set mapped = true, but not if window_type == POPUP.
There was a problem hiding this comment.
You're right, I'll fix it. That said, I'd still like to run some additional tests, as this slipped through unnoticed (at least on my end). There isn't any evident issue in the most common popup use cases, such as menus, tooltips, and dropdowns, but it may still affect less common scenarios.
There was a problem hiding this comment.
I've fixed it and done some testing.
…ompression) - Remove cursor unref (it's aready de-referenced on gdk_window_set_cursor) - Improve XComposite message
|
/issue add 8365201 |
|
@tsayao |
5 participants
This is a continuation to JDK-8236651 and it aims to stabilize the linux glass gtk backend.
It refactors the Glass GTK implementation with a primary focus on window sizing, positioning, and state management, addressing a number of long-standing issues.
Previously, three separate context classes existed, two of which were used for Java Web Start and Applets. These have been unified, as they are no longer required.
Additional tests have been introduced to improve coverage. Some tests produced different results depending on the StageStyle, so they have been converted to use
@ParameterizedTestto exercise multiple styles.Although the primary focus is XWayland, the changes have also been verified to work correctly on Xorg.
This replaces #1789. It removes the use of GdkWindow in favor of GtkWindow, reducing risk and simplifying the review process while preserving the same set of bug fixes. Additionally, #2025 requires a
GtkWindowto be used when setting the parent of the file chooser dialog.To show debug messages, build with
-PCONF=DebugNativeand run with-Djdk.gtk.verbose=true. Log categories can be passed with-Dglass.gtk.logCategories=CATEGORYCATEGORYcan be one or more of:Multiple categories can be specified by separating them with commas (e.g. size,focus,input).
A manual test is provided:
java @build/run.args tests/manual/stage/TestStage.javaWhen a window property is set, it is reported immediately. However, once it reaches the native Glass layer, it may be adjusted or rejected, causing the property to be updated again. Introducing a delay helps ensure the final state has been applied before it is verified.
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2139/head:pull/2139$ git checkout pull/2139Update a local copy of the PR:
$ git checkout pull/2139$ git pull https://git.openjdk.org/jfx.git pull/2139/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2139View PR using the GUI difftool:
$ git pr show -t 2139Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2139.diff
Using Webrev
Link to Webrev Comment