[windows] implement regular windows and supporting method channels #157515
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. 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. |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
github-actions
bot
added
framework
|
@mattkae Should we put all the subsequent PRs in draft mode for now (assuming they are not yet ready for review)? Just so other triagers don't get confused. Thanks for all of this work by the way! CC @loic-sharma @cbracken Any idea who should take on getting all of this reviewed? |
If you would prefer that, I can put the others in draft mode to avoid confusion 👍
No problem :) Looking forward to working towards landing it ! |
I suspect @goderbauer would be the best person to review once he's back :) |
|
Hey @mattkae, welcome, what a first PR! 🎊 We’re excited to see all the progress on the multi-window project, thank you! Sharing from an offline sync for folks following these developments: Before reviewing these pull requests, we’re going to ensure we finish the design review so we are aligned on the final APIs and where they will live. Please see the linked design document and provide feedback. We’ll discuss this feedback in a future Dash Forum, these typically occur weekly and are open to all members of flutter-hackers. The date for this forum has not been set yet, as we are waiting for one of the reviewers to return from being out of office. Once they are back, we will set a date and communicate it on the Discord as usual. Thanks very much for your patience, and for all of the work here! |
|
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@goderbauer @loic-sharma If you are interested, this PR now reflect "declarative API" that we settled on in the spec. The tests still need to be updated, but this should give you a sense of the general look-and-feel of the API as we've described it so far in the document. |
github-actions
bot
added
the
tool
| std::optional<WindowState> state; | ||
| }; | ||
|
|
||
| } // namespace flutter |
There was a problem hiding this comment.
I don't know how I feel about this. Especially when looking at later PRs, which seem to put quite a bit of additional logic here, like positioning for popovers. Yes, we share some code between embedders already, but this seems very different as it implements a specific algorithm.
We have formal embedder API, "informal" platform channels, and now we have algorithms that embedders are expected to implement, placed at arbitrary location in the engine. What about out of tree embedders?
Wouldn't it be better if something like this was implemented in dart instead?
There was a problem hiding this comment.
In the case of positioning, modern compositors do this themselves, as an application never has all the context required to do this correctly (or is not allowed the context to maintain application isolation). For compositors that do not do this, the application needs to perform this, thus each compositor needs to implement this logic.
The difficulty in making shared dart code is the embedder is already running native code when it needs it. If we were to make this code run before requests reach the embedder it would make the API more complicated. In all cases, the shared code is more of a set of helper functions, as the behaviour always depends on the compositor in use.
There was a problem hiding this comment.
I don't quite understand how this addresses my concern. There is no precedent for sharing logic between embedders in-tree like this and I think this definitely needs further discussion.
I have proposal for synchronous ffi-based API https://docs.flutter.dev/go/multi-window-api-ffi. With synchronous API we can easily move all this to Dart code so that it can be accessed by all embedders, not just the "blessed" ones.
There was a problem hiding this comment.
Is this not similar to the shared text input model code?
There was a problem hiding this comment.
Very vaguely. TextInputModel doesn't implement particular algorithm, at least not in a way where all embedders rely on. For example on macOS we're only using a tiny subset of it. iOS or Android is not using it at all. So it's more of a utility class rather than defining logic that embedders are supposed to follow.
Also text input is one of the most problematic parts of Flutter, and if we were starting from scratch we would have likely chosen a very different approach, see #150460. So modeling new code after text input doesn't seem like the best idea.
There was a problem hiding this comment.
That's actually how this is meant to be used - as helpers - depending on platform, the embedder might use e.g. the positioner information itself (like on Windows) - on others (like Wayland) that context will be passed on verbatim to the compositor, so there's no point in making those calculations in Flutter at all. On others yet, child windows might not be supported altogether (mobile platforms, automotive).
The implementation here will only be shared between platforms that can meaningfully make use of it (floating window desktops - which sure, are the majority of them, but not all). Other platforms should default to not supporting child windows until/unless they grow support for at least a subset of them.
engine/src/flutter/shell/platform/windows/flutter_host_window_controller.cc
Outdated
Show resolved
Hide resolved
| map.insert( | ||
| {EncodableValue(kSizeKey), | ||
| EncodableValue(EncodableList{EncodableValue(size->width()), | ||
| EncodableValue(size->height())})}); |
There was a problem hiding this comment.
How does this compare with FlutterEngineSendWindowMetricsEvent? Can the size be different to the size of the view (i.e. the Flutter widget is combined with other native widgets). Can the application make use of the size information?
There was a problem hiding this comment.
They are essentially the same size. onWindowChanged sends the size of the client rectangle of the top-level window, which corresponds to the size of root view in logical coordinates.
robert-ancell
left a comment
robert-ancell
left a comment
There was a problem hiding this comment.
My Windows knowledge is pretty limited, but the code looks good to me. The only serious thing I noticed is the interaction with the window size and the view size, which might just need some clarification to ensure developers don't mix up the two.
engine/src/flutter/shell/platform/windows/flutter_host_window_controller.h
Outdated
Show resolved
Hide resolved
engine/src/flutter/shell/platform/windows/flutter_host_window_controller.cc
Outdated
Show resolved
Hide resolved
|
What is the reason for trying to merge this without the appropriate framework API? This is tightly coupled to added framework API, I don't quite understand how this can be reviewed and merged separately before approving the framework part. |
The reason is that the Google folks don't want to land a public API until (1) all multi-view bugs have been fixed and (2) we are certain that the API changes are more-or-less final. @loic-sharma can provide more details, but we settled on this in the end to make things move faster :) |
| WPARAM wparam, | ||
| LPARAM lparam) { | ||
| switch (message) { | ||
| case WM_NCDESTROY: { |
There was a problem hiding this comment.
Window destroying flow probably needs some thought? A very common scenario is displaying a prompt when closing the window (i.e. to prevent closing unsaved documents).
There was a problem hiding this comment.
WM_NCDESTROY is just for cleanup of resources after closing, the window is already closed and being torn down, so handling this doesn't imply any handling of closing scenarios and is just about resource management
There was a problem hiding this comment.
I meant the overall flow. For example destroying the window from Flutter involoves sending WM_CLOSE. But WM_CLOSE is the one message we should actually handle, prevent default and notify the framework so that we can give the user a way to prevent closing of windows if needed.
There was a problem hiding this comment.
WM_CLOSE is handled by the app lifecycle manager, but it only works for the last window. We expected to have a proper window lifecycle manager for this. Anyway, the delegate pattern you're using in the FFI branch seems like a nice, cleaner alternative.
|
Here is my work in progress branch based on this work but converting it to ffi: https://github.com/knopp/flutter/tree/windows_multi_window_ffi It's bit rough but it mostly works. If you compare the It also doesn't need to need to sync state from native (i.e. size). There is only one source of truth and that is whatever winapi provides at that moment. The prototype also allows handling win32 messages in dart code. Down the line this would allow for packages to customize window behavior, i.e. overriding |
#37) This simplifies the logic for focusing the root view when the top-level window is activated. It removes the handling of `WM_MOUSEACTIVATE`, as `WM_ACTIVATE` already accounts for activations via mouse clicks. Additionally, it ensures that the view is not focused when the top-level window is being deactivated.
Both the requested window size and the size notified by I added an extra comment to the |
|
I think down the line we want to refer to size as clientSize or contentSize (I'd vote for contentSize). Some platforms support setting frame size too, and we will want to expose it, even if it's per platform specific call. So it would not good to get rid of ambiguity from beginning. |
|
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. |
|
how much work left before it can be merged? |
|
@wanjm there's a new approach to crossing the Dart / native boundary in the works: #165072 Not too long! We have agreement on the Dart API, multi-view prerequisites mostly dealt with, just need to work things out on the three desktop platforms so we're confident with the solution. |
Is the plan to move over to this approach, or to continue on with method channels until the FFI approach is finalized and ready? If the plan is to switch to FFI now, does that impact the time until multi-window can be delivered? |
Plan is to go for FFI, no point introducing a lot of code that will be dropped soon. Yes, it does mean it's delayed some weeks. But it's closer than you might think: #165072. |
|
Thanks for the reply! Glad the delay will only be a few weeks. |
|
The implementation of multi-window in Flutter has drifted a lot since this PR was opened. Please see #168728 for the latest implementation. We're currently focusing on an FFI-based approach in contrast to the async method channels approach that we implemented here. |
12 participants
This pull request only makes changes to the windows embedder API and wires a flag through the framework. This does not include any user-facing API changes.
Design doc: https://docs.google.com/document/d/1eQG-IS7r4_S9_h50MY_hSGwUVtgSTRSLzu7MLzPNd2Y/edit?tab=t.0
To test
Don't forget the
--enable-multi-windowflag or it will not workWhat's new?
Framework
--enable-multi-windowflag to turn on multi window featuresEngine
Pre-launch Checklist
///).