Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Remove single view assumptions from window.dart - #38453

Merged
auto-submit[bot] merged 46 commits into
flutter-team-archive:mainfrom
a-wallen:refactor_fl_view
Jan 5, 2023
Merged

Remove single view assumptions from window.dart#38453
auto-submit[bot] merged 46 commits into
flutter-team-archive:mainfrom
a-wallen:refactor_fl_view

Conversation

@a-wallen

@a-wallen a-wallen commented Dec 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Removes FlutterView to support multi-window.

Fixes flutter/flutter#112204

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@a-wallen
a-wallen force-pushed the refactor_fl_view branch 2 times, most recently from c8592e2 to e54d32d Compare December 22, 2022 17:50
goderbauer
goderbauer reviewed Dec 22, 2022
View reviewed changes

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, this is still a draft, just some fly-by comments.

Comment thread lib/ui/window.dart Resolved
Comment thread lib/ui/window.dart Resolved
Comment thread lib/web_ui/lib/platform_dispatcher.dart Outdated
set onPlatformConfigurationChanged(VoidCallback? callback);

Iterable<FlutterView> get views;
Map<Object, FlutterView> get views;

@goderbauer goderbauer Dec 22, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't expose this as a Map. Maps are mutable and we don't want the framework/apps to be able to mutate this object. It's also an (unnecessary) API break.

We have however talked about exposing a new method, something like getViewById that allows you to, well, get a view by passing in an ID without exposing the entire Maps interface.

Comment thread lib/ui/platform_dispatcher.dart Resolved
@flutter-dashboard flutter-dashboard Bot added the platform-web Code specifically for the web engine label Dec 22, 2022
@a-wallen
a-wallen marked this pull request as ready for review December 22, 2022 22:20
@flutter-dashboard

flutter-dashboard Bot commented Dec 22, 2022

Copy link
Copy Markdown

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #38453 at sha b3fed4a

@a-wallen
a-wallen marked this pull request as draft December 22, 2022 23:02
@flutter-dashboard

flutter-dashboard Bot commented Dec 22, 2022

Copy link
Copy Markdown

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@a-wallen
a-wallen marked this pull request as ready for review January 3, 2023 18:46
dkwingsmt
dkwingsmt reviewed Jan 3, 2023
View reviewed changes
Comment thread lib/ui/platform_dispatcher.dart Outdated
const ViewConfiguration({
this.view,
@Deprecated('''
Renaming window to view since `FlutterWindow` has been removed from dart::ui.

@dkwingsmt dkwingsmt Jan 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't understand this deprecation message. It should be something actionable in the form of "Use xxxx instead." For example, "Use the view argument instead. The window argument has been renamed to view."
  2. If it's a rename, shouldn't we point them to the same value? The two parameter should be exclusive (assert that only one of them is non-null) and be assigned to the same field (_view) and both getter should return this field.

@a-wallen a-wallen Jan 3, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right 👍

@a-wallen a-wallen Jan 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about

      `FlutterWindow` has been removed from dart::ui, and the window property is no longer semantically sound
       use the `view` property instead.

@dkwingsmt dkwingsmt Jan 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion the explanation is unnecessary here. Just tell the user how to change it. If you really want to explain it, add a link to the design doc, or to this PR with an expanded paragraph of description.

@loic-sharma loic-sharma Jan 3, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reopening as I don't think the second part was addressed:

If it's a rename, shouldn't we point them to the same value? The two parameter should be exclusive (assert that only one of them is non-null) and be assigned to the same field (_view) and both getter should return this field.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 7, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 7, 2023
@engine-flutter-autoroll
71f9207 33d7f8a1b Remove single view assumptions from window.dart (f…
ae8221f 
…lutter-team-archive/engine#38453) (flutter/flutter#118069)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jan 7, 2023
@engine-flutter-autoroll
71f9207 33d7f8a1b Remove single view assumptions from window.dart (f…
c6ca611 
…lutter-team-archive/engine#38453) (flutter/flutter#118069)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jan 7, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jan 7, 2023
@engine-flutter-autoroll
71f9207 33d7f8a1b Remove single view assumptions from window.dart (f…
4425213 
…lutter-team-archive/engine#38453) (flutter/flutter#118069)
stuartmorgan-g pushed a commit to flutter/packages that referenced this pull request Jan 9, 2023
@engine-flutter-autoroll
Roll Flutter from 5070620 to 7ddf42e (5 revisions) (#3031)
38baaa2 
* 6d7c507 Update docstring (flutter/flutter#118072)

* e0f89e7 Fix out-of-sync ExpansionPanel animation (flutter/flutter#105024)

* 0e55504 Roll Plugins from 320461910156 to 276cfd4b212d (2 revisions) (flutter/flutter#118099)

* 71f9207 33d7f8a1b Remove single view assumptions from `window.dart` (flutter-team-archive/engine#38453) (flutter/flutter#118069)

* 7ddf42e InteractiveViewer parameter to return to pre-3.3 trackpad/Magic Mouse behaviour (flutter/flutter#114280)
@ditman

ditman commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

🎉 just saw the notification that this landed. Kudos!

@a-wallen a-wallen mentioned this pull request Jan 11, 2023
Closed
stuartmorgan-g pushed a commit to flutter-team-archive/plugins that referenced this pull request Jan 11, 2023
@engine-flutter-autoroll
Roll Flutter from 5070620 to 7ddf42e (5 revisions) (#6923)
1226684 
* 6d7c507 Update docstring (flutter/flutter#118072)

* e0f89e7 Fix out-of-sync ExpansionPanel animation (flutter/flutter#105024)

* 0e55504 Roll Plugins from 3204619 to 276cfd4 (2 revisions) (flutter/flutter#118099)

* 71f9207 33d7f8a1b Remove single view assumptions from `window.dart` (flutter-team-archive/engine#38453) (flutter/flutter#118069)

* 7ddf42e InteractiveViewer parameter to return to pre-3.3 trackpad/Magic Mouse behaviour (flutter/flutter#114280)
@ditman ditman mentioned this pull request Jan 13, 2023
Closed
esouthren pushed a commit to esouthren/flutter that referenced this pull request Jan 15, 2023
@engine-flutter-autoroll @esouthren
33d7f8a Remove single view assumptions from window.dart (flutter-te…
fd6372e 
…am-archive/engine#38453) (flutter#118069)
esouthren added a commit to flutter/flutter that referenced this pull request Jan 16, 2023
@esouthren @guidezpl
@engine-flutter-autoroll @Jasguerrero @darrenaustin @asashour @goderbauer @gnprice @CicadaCinema @thkim1011 @Renzo-Olivares Renzo Olivares @srawlins @peixinli @moffatman @mraleph @christopherfujino @fluttergithubbot @camsim99 @LongCatIsLooong @drewroengoogle @jason-simmons @nehalvpatel @gmackall @mackall-work @M97Chahboun @a-wallen a-wallen @morriskurz @LucasXu0 @jmagman @helin24 @TahaTesser @nshahan @yjbanov @bleroux @bdero @loic-sharma Jonah Williams @Time1ess @sigurdm @rydmike @dependabot @isaacscha @andrewkolos @QuncCccccc @joshualitt @awesometic @parlough @IvoB1987 @godofredoc @bernaferrari @IchordeDionysos @HansMuller @vashworth