flutter icon indicating copy to clipboard operation
flutter copied to clipboard

Remove _inDirtyList assert

Open LongCatIsLooong opened this issue 1 year ago • 3 comments

Fixes https://github.com/flutter/flutter/issues/154060

The error message doesn't make sense to me since one can call setState during 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:

├─child 1: _RenderLayoutBuilder#7c319 NEEDS-LAYOUT NEEDS-PAINT
│   creator: LayoutBuilder ← _BodyBuilder ← MediaQuery ←
│     LayoutId-[<_ScaffoldSlot.body>] ← CustomMultiChildLayout ←
│     _ActionsScope ← Actions ← AnimatedBuilder ← DefaultTextStyle ←
│     AnimatedDefaultTextStyle ← _InkFeatures-[GlobalKey#1f6eb ink
│     renderer] ← NotificationListener<LayoutChangedNotification> ← ⋯
│   parentData: offset=Offset(0.0, 0.0); id=_ScaffoldSlot.body
│   constraints: MISSING
│   size: MISSING

So https://github.com/flutter/flutter/pull/154681 doesn't really fix #154060 since the layout callback cannot be run without a set of valid constraints.

Before the BuildScope change all _inDirtyList flags were unset after the BuildOwner finishes rebuilding the widget tree, so LayoutBuilder._inDirtyLst is always set to false after a frame even for layout builders that were never laid out. With the BuildScope change, LayoutBuilder has its own BuildScope which 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.

LongCatIsLooong avatar Sep 05 '24 23:09 LongCatIsLooong

After some digging around, I think this assert is supposed to guard against calling scheduleBuildFor multiple times for the same element. Element.markNeedsBuild (which is called by setState) guards its call to scheduleBuildFor on whether the element is dirty. If it isn't dirty, it returns right there and we skip all the build scheduling steps:

https://github.com/flutter/flutter/blob/84757af9f809f6709d781779452f8b72063f9640/packages/flutter/lib/src/widgets/framework.dart#L5210-L5214

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

goderbauer avatar Sep 06 '24 20:09 goderbauer

After some digging around, I think this assert is supposed to guard against calling scheduleBuildFor multiple times for the same element. Element.markNeedsBuild (which is called by setState) guards its call to scheduleBuildFor on whether the element is dirty. If it isn't dirty, it returns right there and we skip all the build scheduling steps:

https://github.com/flutter/flutter/blob/84757af9f809f6709d781779452f8b72063f9640/packages/flutter/lib/src/widgets/framework.dart#L5210-L5214

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.

~I think it's the LayoutBuilderElement that's triggering the error but it doesn't actually schedule multiple builds. If the render object couldn't do layout then the element stays dirty and _inDirtyList is always true. I'll move the check to the BuildScope class then.~

Unfortunately that didn't work. I think LayoutBuilderElement can be inflated via treewalk, but it has its own BuildScope is the underlying problem. I have 2 alternative solutions but both feel sketchy:

LongCatIsLooong avatar Sep 06 '24 21:09 LongCatIsLooong

Alternative 1: Changes the signature of BuildOwner.buildScope so it

-  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 updateChild happens in performLayout):

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

LongCatIsLooong avatar Sep 07 '24 00:09 LongCatIsLooong

update Element.dirty docs

LongCatIsLooong avatar Sep 12 '24 21:09 LongCatIsLooong