Add Linux implementation of popup windows - #185866
Conversation
|
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 implements popup window support for the Linux platform by introducing the PopupWindowControllerLinux class and extending FlWindowMonitor with an on_moved_to_rect callback to track window positioning. Feedback identifies a potential race condition in the PopupWindowControllerLinux constructor where the rootView lookup may fail because the view registration in the PlatformDispatcher is asynchronous. Additionally, the native signal connection for moved-to-rect is noted as fragile, with recommendations to handle widget realization and GDK version compatibility more robustly to avoid runtime warnings or crashes.
mattkae
left a comment
There was a problem hiding this comment.
Very straightforward!
Is this working correctly with focusing the textbox and dismissing it when another window that is not the parent is focused?
| final _GtkWindow? parentWindow = _owner._windows[_parent.rootView.viewId]; | ||
| final _FlView? view = _owner._views[_parent.rootView.viewId]; | ||
| var offset = (0, 0); | ||
| if (parentWindow != null && view != null) { |
There was a problem hiding this comment.
The parentWindow being null is a logic error, right?
There was a problem hiding this comment.
Either a logic error or some external thing caused the parent window to be destroyed.
There was a problem hiding this comment.
Should we at least log so that we know about it?
There was a problem hiding this comment.
We can save this for later though, not blocking
No description provided.