flutter icon indicating copy to clipboard operation
flutter copied to clipboard

feature: implement createRegularWindow and add it to the reference app

Open mattkae opened this issue 1 year ago • 3 comments

This relies on the engine built from this branch: https://github.com/flutter/engine/pull/56094

Design doc: https://docs.google.com/document/d/1eQG-IS7r4_S9_h50MY_hSGwUVtgSTRSLzu7MLzPNd2Y/edit?tab=t.0

This PR is a part of a group of PRs. Going from first to last, these must be merged in the following order:

  • #157515
  • #157517
  • #157518
  • #157519
  • #157520
  • #157521
  • #157522
  • #157523
  • #157524
  • #157525

This pull request:

  • Lays the foundational data models for multi window Flutter applications
  • Implements createRegular for the creation of regular toplevel windows
  • Add the multi_window_ref_app example application to the project to demonstrate the creation of a regular window
  • Add an initial unit test for the creation of regular windows
  • Adapt the runner code of a typical single window application for multi window

image

Pre-launch Checklist

  • [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • [x] I signed the [CLA].
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I added new tests to check the change I am making, or this PR is [test-exempt].
  • [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • [x] All existing and new tests are passing.

mattkae avatar Oct 24 '24 16:10 mattkae

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.

flutter-dashboard[bot] avatar Oct 24 '24 16:10 flutter-dashboard[bot]

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.

google-cla[bot] avatar Oct 24 '24 16:10 google-cla[bot]

@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?

justinmc avatar Oct 30 '24 18:10 justinmc

@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.

If you would prefer that, I can put the others in draft mode to avoid confusion :+1:

Thanks for all of this work by the way!

No problem :) Looking forward to working towards landing it !

mattkae avatar Oct 31 '24 09:10 mattkae

CC @loic-sharma @cbracken Any idea who should take on getting all of this reviewed?

I suspect @goderbauer would be the best person to review once he's back :)

loic-sharma avatar Oct 31 '24 22:10 loic-sharma

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!

Piinks avatar Nov 07 '24 16:11 Piinks

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 package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

flutter-dashboard[bot] avatar Nov 27 '24 16:11 flutter-dashboard[bot]

@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.

mattkae avatar Dec 12 '24 13:12 mattkae

@goderbauer @jtmcdole can you find the right reviewers for this non-tool portions of this PR?

bkonyi avatar Jan 09 '25 21:01 bkonyi

I am wondering if --enable-multi-window is the best way to go about this. Is there a reason why this would be a flag that has to be specified for each build, rather then either flutter config or enabled simply through embedder API? The launcher needs to be modified for multi-window anyway.

knopp avatar Feb 28 '25 21:02 knopp

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.

knopp avatar Mar 03 '25 03:03 knopp

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 :)

mattkae avatar Mar 03 '25 13:03 mattkae

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 window.dart to original you will notice that it doesn't need any futures, isReady checks or future builders.

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 WM_NCHITTEST which is required to make custom frame windows.

knopp avatar Mar 04 '25 16:03 knopp

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.

Both the requested window size and the size notified by onWindowChanged refer to the client rectangle size, which matches the root view size. The application is unaware of the window frame size or the window rectangle size (frame + drop shadow).

I added an extra comment to the SendOnWindowChanged declaration for clarity.

hbatagelo avatar Mar 11 '25 15:03 hbatagelo

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.

knopp avatar Mar 11 '25 17:03 knopp

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.

flutter-dashboard[bot] avatar Mar 26 '25 21:03 flutter-dashboard[bot]

how much work left before it can be merged?

wanjm avatar Mar 27 '25 02:03 wanjm

@wanjm there's a new approach to crossing the Dart / native boundary in the works:

https://github.com/flutter/flutter/pull/165072 https://docs.google.com/document/d/1UEaxd_aIlsPMzkfB6oyeYaKzwHadrWGyMzOS33oSbK4/edit

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.

Saviq avatar Mar 27 '25 08:03 Saviq

@wanjm there's a new approach to crossing the Dart / native boundary in the works:

#165072 https://docs.google.com/document/d/1UEaxd_aIlsPMzkfB6oyeYaKzwHadrWGyMzOS33oSbK4/edit

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?

GroovinChip avatar Apr 03 '25 15:04 GroovinChip

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.

Saviq avatar Apr 03 '25 17:04 Saviq

Thanks for the reply! Glad the delay will only be a few weeks.

GroovinChip avatar Apr 03 '25 17:04 GroovinChip

The implementation of multi-window in Flutter has drifted a lot since this PR was opened. Please see https://github.com/flutter/flutter/pull/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.

mattkae avatar May 22 '25 18:05 mattkae