Repository navigation
refactor(linux): introduce GTK3 surface helpers - #193943
richyo-codes wants to merge 4 commits into
Conversation
Encapsulate native surface, display, and monitor lookup behind inline helpers and use them in the GTK3 view. Preserve the existing GTK3 behavior and dependencies while preparing for a separate GTK4 implementation.
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces GTK abstraction wrappers fl_gtk.h and fl_gtk_gtk3.h to encapsulate GTK/GDK calls, and updates fl_view.cc to use these helper functions. The review feedback suggests adding defensive null checks in the new helper functions in fl_gtk_gtk3.h and in fl_view.cc's cursor_changed_cb to gracefully handle unrealized widgets and prevent potential GDK/GLib critical warnings.
| FlGdkSurface* window = | ||
| fl_gtk_widget_get_surface(gtk_widget_get_toplevel(GTK_WIDGET(self))); | ||
| g_autoptr(GdkCursor) cursor = | ||
| gdk_cursor_new_from_name(gdk_window_get_display(window), cursor_name); | ||
| gdk_cursor_new_from_name(fl_gtk_surface_get_display(window), cursor_name); | ||
| gdk_window_set_cursor(window, cursor); |
There was a problem hiding this comment.
If the widget is not yet realized, fl_gtk_widget_get_surface will return nullptr. Passing nullptr to fl_gtk_surface_get_display and subsequently to GDK functions will trigger multiple GDK/GLib critical warnings in the console. We should add a defensive null check for window before attempting to set the cursor.
FlGdkSurface* window =
fl_gtk_widget_get_surface(gtk_widget_get_toplevel(GTK_WIDGET(self)));
if (window == nullptr) {
return;
}
g_autoptr(GdkCursor) cursor =
gdk_cursor_new_from_name(fl_gtk_surface_get_display(window), cursor_name);
gdk_window_set_cursor(window, cursor);There was a problem hiding this comment.
This seems reasonable, but it would be an additional check the original code didn't do. Goal of this PR is to refactor to preserve existing GTK3 functionality while enabling a structure for GTK4 to be opted in
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
Verify the surface helpers forward the original widget, window and display pointers to GTK3 and return the native results unchanged, including a widget without a window. Make the existing display and monitor mock functions configurable while preserving their default behavior. Register the four checks in flutter_linux_unittests.
I added tests to verify the new helper functions still forward to the exact same original functions when using GTK3. If / when GTK4 support is added the test will be update to verify we don't call these functions and call GTK4 version instead. Other than getting feedback on where or not to include additional null checks. I don't think anything else to do to improve this PR. |
This PR encapsulates native surface, display, and monitor lookup behind a small set of inline helpers and updates the GTK3 view to use them.
The helpers follow GTK/GDK's existing concepts rather than introducing a new windowing abstraction. They bridge equivalent GTK3 and GTK4 APIs:
GdkWindow→GdkSurfacegtk_widget_get_window()→gtk_widget_get_native()+gtk_native_get_surface()gdk_window_get_display()→gdk_surface_get_display()gdk_display_get_monitor_at_window()→gdk_display_get_monitor_at_surface()This lets shared code use consistent surface/display/monitor terminology while keeping each implementation close to its native GTK API.
There is no intended GTK3 behavior change. This preserves the existing GTK3 types and behavior while establishing a narrow compatibility boundary for GTK4.
To see how this refactor makes room for GTK4, see commit
[0494de5](https://github.com/flutter/flutter/pull/193405/changes/0494de5b3eb373361c4a5e471da13da0569767ba).This refactor was extracted from the larger GTK4 work in #193405 for #94804 and #194698.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.