Use the WindowRegistry in the multiple_windows example app#184579
Conversation
github-actions
Bot
added
framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the custom KeyedWindowManager with the framework's WindowRegistry in the multiple windows example, refactoring several widgets to use WindowRegistry.of and WindowEntry for window management. Feedback highlights a bug in the WindowRegistry.of debug check which should verify _WindowRegistryScope instead of WindowScope. Additionally, improvements are suggested for the window toggle logic in PopupButton and TooltipButton to prevent potential null pointer exceptions during asynchronous destruction and to fix a missing method call to dispose.
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
Very nice clean up! Looks good overall, but please take a look at Gemini's feedback
loic-sharma
left a comment
loic-sharma
left a comment
There was a problem hiding this comment.
LGTM modulo the potentially incorrect doc. Thanks for cleaning this up!
| /// | ||
| /// {@macro flutter.widgets.windowing.experimental} | ||
| @internal | ||
| static WindowRegistry of(BuildContext context) { |
There was a problem hiding this comment.
On second thought, I'm leaning towards making this throw if isWindowingEnabled is false. Currently, WindowManager doesn't add a _WindowRegistryScope if isWindowingEnabled is false:
flutter/packages/flutter/lib/src/widgets/_window.dart
Lines 2379 to 2386 in fba3f2c
Thus, WindowRegistry.of will always throw if windowing is off. Some potential options:
- Option 1: Throw here if
isWindowingEnabledis false. This behavior would be consistent with other windowing APIs. - Option 2: Update
WindowManagerto add an empty_WindowRegistryScopeifisWindowingEnabledisfalse. I don't love this option - it feels likeWindowRegistrywill need a partial working state if windowing is off. - Option 3: Update the
ErrorHintbelow to explain that another reason this throws is if you have aWindowManagerancestor but windowing is off. We'd need to remember to clean this up in the future when we remove the windowing feature, though.
What do you think?
There was a problem hiding this comment.
The bad part of always throwing whenever one attempts to access the WindowRegistry is that its unavailability is currently the common case. For example, the MaterialApp now uses the presence of a WindowRegistry to decide whether or not to display a true dialog/tooltip. A common case (right now) would be for windowing to be turned off by the feature flag. This would lead the build() method to constantly throw, which isn't great.
In my opinion, this should behave similar to how other .of(...) methods behave. If the feature is unavailable for whatever reason, it will be null. Then the user does not have to worry about handling separate exceptional cases.
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM with nits 👍 . Thanks for coming back and cleaning this up now that we have WindowRegistry. I think the example is now a lot easier for devs to learn from.
| final KeyedWindowManager windowManager = KeyedWindowManagerAccessor.of( | ||
| context, | ||
| ); | ||
| final WindowRegistry windowRegistry = WindowRegistry.of(context); |
There was a problem hiding this comment.
Would it be possible to look this up inside of the onPressed callback? Not sure if that's an improvement, just thinking about it.
|
|
||
| final RegularWindowController mainWindow; | ||
|
|
||
| DataRow _buildRow(BaseWindowController controller, BuildContext context) { |
There was a problem hiding this comment.
Nit: Could this be a widget instead of a function? I try to lean that way when I see functions that return widgets, but I'm not sure whether it works out here or not.
There was a problem hiding this comment.
DataRow is actually not a widget: https://api.flutter.dev/flutter/material/DataRow-class.html
| /// | ||
| /// {@macro flutter.widgets.windowing.experimental} | ||
| @internal | ||
| static WindowRegistry of(BuildContext context) { |
There was a problem hiding this comment.
Is the point of this versus .maybeOf(context)! just to provide a better error message in debug mode?
There was a problem hiding this comment.
That and it is a common pattern throughout the code. There are certain contexts where lack of access to an InheritedWidget is nonsensical, so this declares a hard dependency on the presence of the WindowRegistry
| @internal | ||
| static WindowRegistry of(BuildContext context) { | ||
| final WindowRegistry? registry = maybeOf(context); | ||
| assert(() { |
There was a problem hiding this comment.
I wonder if the assert is useful here or not. In release mode, there is still guaranteed to be an error thrown (at registry!). It will just be a less useful error than the one in debug mode.
There was a problem hiding this comment.
I personally appreciate the helpful error message. Plus this matches existing of(...) calls, so I'm tempted to keep it as-is.
There was a problem hiding this comment.
I'd also lean to keeping this as-is as the error bubbles up that you need to wrap your app with a WindowManager widget to get a WindowRegistry inherited widget. I think this error will be particularly helpful for folks that try to use windowing APIs with their own custom design systems that aren't based on Material.
There was a problem hiding this comment.
I meant maybe we should remove the assert but keep its body, so we always throw the useful FlutterError instead of only throwing it in debug mode. Not a big deal though, I know this is about to merge :)
|
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. 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. |
flutter/flutter@9cd60b5...a0924c7 2026-04-07 [email protected] Reland "[data_assets] Cleanup tests" (flutter/flutter#184714) 2026-04-07 [email protected] Use the WindowRegistry in the multiple_windows example app (flutter/flutter#184579) 2026-04-07 [email protected] Introduce command to build a swift package for SwiftPM add to app integration (flutter/flutter#184660) 2026-04-07 [email protected] Have `flutter create` create a pubspec.lock to ensure pinned versions are being used. (flutter/flutter#175352) 2026-04-07 [email protected] [widgets/raw_menu_anchor.dart] Always call onClose and onCloseRequested on descendants before parent. (flutter/flutter#182357) 2026-04-07 [email protected] `WindowsPlugin` should not crash when ffiPlugin enabled (flutter/flutter#184695) 2026-04-06 [email protected] Use full goto.google.com hostname for go/ links (flutter/flutter#184679) 2026-04-06 [email protected] Apply rect clipping to surface views (flutter/flutter#184471) 2026-04-06 [email protected] [A11y] Allow percentage strings like "50%" as `SemanticsValue` for `ProgressIndicator` (flutter/flutter#183670) 2026-04-06 [email protected] Fix invisible accessibility element before scroll view (flutter/flutter#184155) 2026-04-06 [email protected] Roll Skia from 163dfdf500c7 to e264d870a380 (2 revisions) (flutter/flutter#184674) 2026-04-06 [email protected] Keep last character obscured when toggling obscureText (flutter/flutter#183488) 2026-04-06 [email protected] Parse scheme file with XML parser for SwiftPM migrator (flutter/flutter#184525) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…84579) ## What's new? - This pull request updates the multiple_windows example application use the new `WindowRegistry` from the `_window.dart` API to hold the collection of windows instead of the `KeyedWindowManager` that we had implemented prior. - It also adds `WindowRegistry.of` for a non-nullable access to the window registry. ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [AI contribution guidelines] and understand my responsibilities, or I am not using AI tools. - [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. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [AI contribution guidelines]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
3 participants
What's new?
WindowRegistryfrom the_window.dartAPI to hold the collection of windows instead of theKeyedWindowManagerthat we had implemented prior.WindowRegistry.offor a non-nullable access to the window registry.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.