Expose Gtk objects on _window_linux.dart in order to better support out of tree LinuxWindowingOwners#188917
Expose Gtk objects on _window_linux.dart in order to better support out of tree LinuxWindowingOwners#188917mattkae wants to merge 1 commit into
Conversation
…ut of tree LinuxWindowingOwners
|
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. |
There was a problem hiding this comment.
Code Review
This pull request refactors the Linux windowing implementation by introducing a GtkWindowRegistrar to track native GTK windows and Flutter views, and exposes previously private classes and enums as @internal public types. The review feedback suggests marking GtkWindowRegistrar as final, adding assertions to prevent duplicate registrations, throwing specific StateErrors instead of generic Exceptions when lookups fail, and removing obsolete // ignore: unused_field comments from the newly exposed enums.
| @internal | ||
| class GtkWindowRegistrar { |
There was a problem hiding this comment.
Since GtkWindowRegistrar is a concrete utility class meant to be used directly rather than extended or implemented, consider marking it as final to adhere to Dart 3 class modifiers best practices.
@internal
final class GtkWindowRegistrar {| void register({required int viewId, required GtkWindow window, required FlView view}) { | ||
| _windows[viewId] = window; | ||
| _views[viewId] = view; | ||
| } |
There was a problem hiding this comment.
To prevent silent bugs, memory leaks, or incorrect window parenting due to duplicate registrations, consider adding assertions to ensure the viewId is not already registered.
void register({required int viewId, required GtkWindow window, required FlView view}) {
assert(!_windows.containsKey(viewId), 'A window with viewId $viewId is already registered.');
assert(!_views.containsKey(viewId), 'A view with viewId $viewId is already registered.');
_windows[viewId] = window;
_views[viewId] = view;
}| final GtkWindow? parentWindow = owner.registrar.windowForViewId(parent.rootView.viewId); | ||
| if (parentWindow == null) { | ||
| throw Exception('Failed to find dialog parent window'); | ||
| } |
There was a problem hiding this comment.
In Dart, it is preferred to throw a more specific error like StateError or ArgumentError instead of a generic Exception when a lookup fails or an invalid state is encountered.
final GtkWindow? parentWindow = owner.registrar.windowForViewId(parent.rootView.viewId);
if (parentWindow == null) {
throw StateError('Failed to find dialog parent window');
}| final GtkWindow? parentWindow = _owner.registrar.windowForViewId(_parent.rootView.viewId); | ||
| if (parentWindow == null) { | ||
| throw Exception('Failed to find tooltip parent window'); | ||
| } |
There was a problem hiding this comment.
In Dart, it is preferred to throw a more specific error like StateError or ArgumentError instead of a generic Exception when a lookup fails or an invalid state is encountered.
final GtkWindow? parentWindow = _owner.registrar.windowForViewId(_parent.rootView.viewId);
if (parentWindow == null) {
throw StateError('Failed to find tooltip parent window');
}| final GtkWindow? parentWindow = _owner.registrar.windowForViewId(_parent.rootView.viewId); | ||
| if (parentWindow == null) { | ||
| throw Exception('Failed to find popup parent window'); | ||
| } |
There was a problem hiding this comment.
In Dart, it is preferred to throw a more specific error like StateError or ArgumentError instead of a generic Exception when a lookup fails or an invalid state is encountered.
final GtkWindow? parentWindow = _owner.registrar.windowForViewId(_parent.rootView.viewId);
if (parentWindow == null) {
throw StateError('Failed to find popup parent window');
}| enum GtkWindowType { | ||
| toplevel, | ||
| // ignore: unused_field | ||
| popup, | ||
| } |
There was a problem hiding this comment.
Since GtkWindowType is now a public enum (even though marked @internal), the Dart analyzer will not flag its values as unused. The // ignore: unused_field comment is obsolete and can be removed.
enum GtkWindowType {
toplevel,
popup,
}| enum GdkWindowTypeHint { | ||
| // ignore: unused_field | ||
| normal, | ||
| dialog, |
There was a problem hiding this comment.
Since GdkWindowTypeHint is now a public enum (even though marked @internal), the Dart analyzer will not flag its values as unused. The // ignore: unused_field comment is obsolete and can be removed.
enum GdkWindowTypeHint {
normal,
dialog,| enum GdkGravity { | ||
| // ignore: unused_field | ||
| none, | ||
| northWest, |
There was a problem hiding this comment.
Since GdkGravity is now a public enum (even though marked @internal), the Dart analyzer will not flag its values as unused. The // ignore: unused_field comment is obsolete and can be removed.
enum GdkGravity {
none,
northWest,|
Two things immediately come to mind:
|
Problem
While authoring https://github.com/mattkae/layer_shell.dart, I realized that the Linux Windowing platform had a few major problems when it came to extending it for out-of-tree windowing owners:
_windowsor_viewsmaps, which meant that they could never be parents. This is invalid, as other windows (e.g. layer shell windows) may indeed want to be valid parents and open popups. For example, a top panel may want to open a sound control dropdown.RegularWindowLinux, but that was unfortunately not possible because the regular windows realize themselves by the time the constructor returns. Gtk layer shell windows require that the window become layer shell before it is realized. We could have added an odd flag to the regular window, but that felt like duct-tape over a larger issue whereby an external author could NOT do anything sufficiently complex with the current API.@internalindicator so that users are aware that they are doing something funky.What's new?
GtkWindowRegistrarthat can be used to register and unregister 3rd party windowsGdkWindowTypeGdkWindowStateGtkWindowFlWindowMonitorFlViewFlEngineFlViewMonitorGdkGravityGdkWindowTypeHintGdkAnchorHintGObjectGtkWidgetThis may seem like a drastic export, and we may want slim it down. However, if a user is extending the Linux windowing API for their own usecase, they will 99% of the time want these lower level, less stable constructs to play with.
The user is doing something weird by making the cast to a Linux-specific API, and they should have the full capabilities of that weirdness at their disposal if that's what they choose.
In my opinion, the
@internalflag can stay around these APIs forever. They do NOT need to be stabilized at the same time as the rest of the windowing APIs, as they provide potentially unstable but useful behavior.Associated PR: mattkae/layer_shell.dart#1
Pre-launch Checklist
///).