-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Mark _LayoutBuilderElement as always clean
#154694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
After some digging around, I think this assert is supposed to guard against calling flutter/packages/flutter/lib/src/widgets/framework.dart Lines 5210 to 5214 in 84757af
Can you help me understand why it should conceptually be acceptable to schedule multiple builds for the same element? (I do agree that the ErrorHint is bogus.) |
Oh that makes sense thanks for looking into it.
Unfortunately that didn't work. I think |
|
Alternative 1: Changes the signature of - void buildScope(Element context, [ VoidCallback? callback ]) {
- final BuildScope buildScope = context.buildScope;
+ void buildScope(Element context, [ VoidCallback? callback, BuildScope? buildScopeOverride ]) {
+ final BuildScope buildScope = buildScopeOverride ?? context.buildScope;
if (callback == null && buildScope._dirtyElements.isEmpty) {
return;
}
modified packages/flutter/lib/src/widgets/layout_builder.dart
@@ -78,6 +78,31 @@ abstract class ConstrainedLayoutBuilder<ConstraintType extends Constraints> exte
// updateRenderObject is redundant with the logic in the LayoutBuilderElement below.
}
+class _BuildScopeOwner extends ProxyWidget {
+ const _BuildScopeOwner(this.buildScope, Widget child) : super(child: child);
+
+ final BuildScope buildScope;
+
+ @override
+ Element createElement() => _ElementWithBuildScope(this);
+}
+
+class _ElementWithBuildScope extends ComponentElement {
+ _ElementWithBuildScope(_BuildScopeOwner super.widget);
+
+ @override
+ BuildScope get buildScope => (widget as _BuildScopeOwner).buildScope;
+
+ @override
+ void update(Widget newWidget) {
+ super.update(newWidget);
+ rebuild(force: true);
+ }
+
+ @override
+ Widget build() => (widget as _BuildScopeOwner).child;
+}
+
class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderObjectElement {
_LayoutBuilderElement(ConstrainedLayoutBuilder<ConstraintType> super.widget);
@@ -86,9 +111,6 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
Element? _child;
- @override
- BuildScope get buildScope => _buildScope;
-
late final BuildScope _buildScope = BuildScope(scheduleRebuild: _scheduleRebuild);
// To schedule a rebuild, markNeedsLayout needs to be called on this Element's
@@ -100,7 +122,6 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
if (_deferredCallbackScheduled) {
return;
}
-
final bool deferMarkNeedsLayout = switch (SchedulerBinding.instance.schedulerPhase) {
SchedulerPhase.idle || SchedulerPhase.postFrameCallbacks => true,
SchedulerPhase.transientCallbacks || SchedulerPhase.midFrameMicrotasks || SchedulerPhase.persistentCallbacks => false,
@@ -208,6 +229,7 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
),
);
}
+ built = _BuildScopeOwner(_buildScope, built);
try {
_child = updateChild(_child, built, null);
assert(_child != null);
@@ -233,7 +255,7 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
final VoidCallback? callback = _needsBuild || (constraints != _previousConstraints)
? updateChildCallback
: null;
- owner!.buildScope(this, callback);
+ owner!.buildScope(this, callback, _buildScope);
}Alternative 2, LayoutBuilderElement can never be dirty or be added to the dirty list (because @override
void markNeedsBuild() {
- super.markNeedsBuild();
+ // super.markNeedsBuild();
renderObject.markNeedsLayout();
_needsBuild = true;
}
@override
void performRebuild() {
// This gets called if markNeedsBuild() is called on us.
// That might happen if, e.g., our builder uses Inherited widgets.
// Force the callback to be called, even if the layout constraints are the
// same. This is because that callback may depend on the updated widget
// configuration, or an inherited widget.
renderObject.markNeedsLayout();
_needsBuild = true;
super.performRebuild(); // Calls widget.updateRenderObject (a no-op in this case).
} |
_LayoutBuilderElement as always clean
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still trying to wrap my head around this. Can you explain a bit the order of things that happens that cause the assertion to be thrown?
| @override | ||
| void markNeedsBuild() { | ||
| super.markNeedsBuild(); | ||
| // Calling super.markNeedsBuild is not needed. This Element does not need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the case, will this class's performRebuild be called at all? if the assumption is this element is never dirty, we should probably assert the performRebuild not getting called at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when the parent Element calls updateChild (i.e. in treewalk).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, I just found out the RenderObjectElement overrides the update
| } | ||
| // When reactivating an inactivate Element, _scheduleBuildFor should only be | ||
| // called within _flushDirtyElements. | ||
| if (!_debugBuilding && element._inDirtyList) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check assume repeated call happens within the same frame, which is no longer the case in the layoutbuilder.
WDYT about updating the check to take into account that it is not guarantee the build will be flush every frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One possible way is to somehow only enforce this on root build scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually know I think of it the current pr makes more sense. there is just nothing to do for rebuild for layoutBuilder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the assert should still hold: #154681
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
update |
flutter/flutter@303f222...2d30fe4 2024-09-13 [email protected] Roll Packages from 91caa7a to 330581f (4 revisions) (flutter/flutter#155171) 2024-09-13 [email protected] Fix: Flicker when reorderable list doesn't change its position (flutter/flutter#151026) 2024-09-13 [email protected] Stop reading .packages from flutter_tools. (flutter/flutter#154912) 2024-09-13 [email protected] Roll Flutter Engine from 70109e3b40c0 to bef48e87f438 (1 revision) (flutter/flutter#155156) 2024-09-13 [email protected] Roll Flutter Engine from d917a15823f3 to 70109e3b40c0 (1 revision) (flutter/flutter#155151) 2024-09-13 [email protected] Roll Flutter Engine from 94696ed75dea to d917a15823f3 (1 revision) (flutter/flutter#155147) 2024-09-13 [email protected] Fix TextField content should be selected on desktop when gaining focus (flutter/flutter#154916) 2024-09-13 [email protected] Roll Flutter Engine from 04802b779045 to 94696ed75dea (1 revision) (flutter/flutter#155144) 2024-09-13 [email protected] Roll Flutter Engine from 3d8163f47919 to 04802b779045 (2 revisions) (flutter/flutter#155138) 2024-09-13 [email protected] Roll Flutter Engine from 4d5fea97e933 to 3d8163f47919 (1 revision) (flutter/flutter#155136) 2024-09-13 [email protected] Mark `_LayoutBuilderElement` as always clean (flutter/flutter#154694) 2024-09-13 [email protected] Roll Flutter Engine from 8609af642725 to 4d5fea97e933 (7 revisions) (flutter/flutter#155134) 2024-09-12 [email protected] Disable fuchsia in flutter_tools (flutter/flutter#155111) 2024-09-12 [email protected] Address frame policy benchmark flakes (flutter/flutter#155130) 2024-09-12 [email protected] Roll Flutter Engine from 48ddaf578fb0 to 8609af642725 (11 revisions) (flutter/flutter#155128) 2024-09-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.1 to 7.0.2 (flutter/flutter#155126) 2024-09-12 [email protected] Prevent the keyboard from reshowing on iOS (flutter/flutter#154584) 2024-09-12 [email protected] fix(Linux): specify application id (flutter/flutter#154522) 2024-09-12 [email protected] update changelog on master (flutter/flutter#155109) 2024-09-12 [email protected] iOS: update provisioning profile for 2024-2025 cert (flutter/flutter#155101) 2024-09-12 [email protected] Roll Packages from 4c18648 to 91caa7a (2 revisions) (flutter/flutter#155103) 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
Fixes #154060
The error message doesn't make sense to me since one can call
setStateduring the idle phase, and I'm not sure what this is guarding against without the right error message.In the case of #154060 the layout builder was never laid out:
So #154681 doesn't really fix #154060 since the layout callback cannot be run without a set of valid constraints.
Before the
BuildScopechange all_inDirtyListflags were unset after theBuildOwnerfinishes rebuilding the widget tree, soLayoutBuilder._inDirtyLstis always set to false after a frame even for layout builders that were never laid out.With the
BuildScopechange,LayoutBuilderhas its ownBuildScopewhich is only flushed after LayoutBuilder gets its constraints.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.