OverlayPortal.childLayoutBuilder should rebuild when OverlayPortal rebuilds.
#165331
Conversation
…rebuilds. The builder callback can capture free variables. So the it must be re-evaluated every time `OverlayPortal` rebuilds.
| } | ||
|
|
||
| @override | ||
| void ensureFrameCallbacksRegistered() { |
There was a problem hiding this comment.
nit: can create a different method to call the ensureFrameCallbacksRegistered
| ..dispose(), | ||
| ); | ||
|
|
||
| Widget builder(BuildContext _, OverlayChildLayoutInfo _) => ColoredBox(color: color); |
There was a problem hiding this comment.
I am not sure if this code is the correct usage of builder. In a widget build method, it will be developer's job to ensure the widget rebuild if it reference a state outside of the build method. Either turn that into changenotifier and use ChangeNotifierBuilder, or use inherited widget to do dependsOnInheritedWidget.
There was a problem hiding this comment.
I don't think that's true. In a Builder / StatefulBuilder / LayoutBuilder the developer should be allowed to capture free variables in the callback. The framework should rebuild the entire widget tree if the root is dirty (unless it hits a cached widget) otherwise every builder callback will have to be closed functions because they may use variables defined in its parent widget's State. Otherwise the syntax will be way too verbose, and builder widgets will be a pain to use / debug.
There was a problem hiding this comment.
yeah this makes sense consider that is true for other builder type widget. SGTM then
| ..dispose(), | ||
| ); | ||
|
|
||
| Widget builder(BuildContext _, OverlayChildLayoutInfo _) => ColoredBox(color: color); |
There was a problem hiding this comment.
yeah this makes sense consider that is true for other builder type widget. SGTM then
| @@ -2615,7 +2615,7 @@ class _OverlayChildLayoutBuilder extends AbstractLayoutBuilder<OverlayChildLayou | |||
| ) => _RenderLayoutBuilder(); | |||
|
|
|||
| @override | |||
| bool updateShouldRebuild(_OverlayChildLayoutBuilder oldWidget) => oldWidget.builder != builder; | |||
| bool updateShouldRebuild(_OverlayChildLayoutBuilder oldWidget) => true; | |||
There was a problem hiding this comment.
This is the same as default implementation, I think we can remove this override
…rlayPortal` rebuilds. (flutter/flutter#165331)
The builder callback can capture free variables. So it must be re-evaluated every time
OverlayPortalrebuilds.Also defining the builder callback as a method in the State class seems to be a common pattern. So the
oldWidget.builder != buildercheck may prevent necessary rebuilds when that's the case.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.