From 9c08836273995e5ba6bf27e3cbe3465ae3730b92 Mon Sep 17 00:00:00 2001 From: Adam Barth Date: Fri, 11 Mar 2016 12:06:30 -0800 Subject: [PATCH] Handle the case of reparenting while updating children Instead of trying to flush the detached children from the child list, we keep the set of detached children up to date and query on every read. --- .../flutter/lib/src/widgets/framework.dart | 42 ++++---- .../test/widget/reparent_state_test.dart | 95 +++++++++++++++++++ 2 files changed, 113 insertions(+), 24 deletions(-) diff --git a/packages/flutter/lib/src/widgets/framework.dart b/packages/flutter/lib/src/widgets/framework.dart index faea8b4fe70..8bf1794cb3d 100644 --- a/packages/flutter/lib/src/widgets/framework.dart +++ b/packages/flutter/lib/src/widgets/framework.dart @@ -1536,10 +1536,14 @@ abstract class RenderObjectElement extends Buildab /// Attempts to update the given old children list using the given new /// widgets, removing obsolete elements and introducing new ones as necessary, /// and then returns the new child list. - List updateChildren(List oldChildren, List newWidgets) { + List updateChildren(List oldChildren, List newWidgets, { Set detachedChildren }) { assert(oldChildren != null); assert(newWidgets != null); + Element replaceWithNullIfDetached(Element child) { + return detachedChildren != null && detachedChildren.contains(child) ? null : child; + } + // This attempts to diff the new child list (this.children) with // the old child list (old.children), and update our renderObject // accordingly. @@ -1582,7 +1586,7 @@ abstract class RenderObjectElement extends Buildab // Update the top of the list. while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) { - Element oldChild = oldChildren[oldChildrenTop]; + Element oldChild = replaceWithNullIfDetached(oldChildren[oldChildrenTop]); Widget newWidget = newWidgets[newChildrenTop]; assert(oldChild == null || oldChild._debugLifecycleState == _ElementLifecycle.active); if (oldChild == null || !Widget.canUpdate(oldChild.widget, newWidget)) @@ -1597,7 +1601,7 @@ abstract class RenderObjectElement extends Buildab // Scan the bottom of the list. while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) { - Element oldChild = oldChildren[oldChildrenBottom]; + Element oldChild = replaceWithNullIfDetached(oldChildren[oldChildrenBottom]); Widget newWidget = newWidgets[newChildrenBottom]; assert(oldChild == null || oldChild._debugLifecycleState == _ElementLifecycle.active); if (oldChild == null || !Widget.canUpdate(oldChild.widget, newWidget)) @@ -1612,7 +1616,7 @@ abstract class RenderObjectElement extends Buildab if (haveOldChildren) { oldKeyedChildren = new Map(); while (oldChildrenTop <= oldChildrenBottom) { - Element oldChild = oldChildren[oldChildrenTop]; + Element oldChild = replaceWithNullIfDetached(oldChildren[oldChildrenTop]); assert(oldChild == null || oldChild._debugLifecycleState == _ElementLifecycle.active); if (oldChild != null) { if (oldChild.widget.key != null) @@ -1663,7 +1667,7 @@ abstract class RenderObjectElement extends Buildab // Update the bottom of the list. while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) { Element oldChild = oldChildren[oldChildrenTop]; - assert(oldChild != null); + assert(replaceWithNullIfDetached(oldChild) != null); assert(oldChild._debugLifecycleState == _ElementLifecycle.active); Widget newWidget = newWidgets[newChildrenTop]; assert(Widget.canUpdate(oldChild.widget, newWidget)); @@ -1678,8 +1682,10 @@ abstract class RenderObjectElement extends Buildab // clean up any of the remaining middle nodes from the old list if (haveOldChildren && oldKeyedChildren.isNotEmpty) { - for (Element oldChild in oldKeyedChildren.values) - _deactivateChild(oldChild); + for (Element oldChild in oldKeyedChildren.values) { + if (detachedChildren == null || !detachedChildren.contains(oldChild)) + _deactivateChild(oldChild); + } } return newChildren; @@ -1808,19 +1814,9 @@ class MultiChildRenderObjectElement exte } List _children; - // We null out detached children lazily to avoid O(n^2) work walking _children + // We keep a set of detached children to avoid O(n^2) work walking _children // repeatedly to remove children. - Set _detachedChildren; - - void _replaceDetachedChildrenWithNull() { - if (_detachedChildren != null && _detachedChildren.isNotEmpty) { - for (int i = 0; i < _children.length; ++i) { - if (_detachedChildren.contains(_children[i])) - _children[i] = null; - } - _detachedChildren.clear(); - } - } + final Set _detachedChildren = new HashSet(); void insertChildRenderObject(RenderObject child, Element slot) { final ContainerRenderObjectMixin renderObject = this.renderObject; @@ -1860,15 +1856,13 @@ class MultiChildRenderObjectElement exte } void visitChildren(ElementVisitor visitor) { - _replaceDetachedChildrenWithNull(); for (Element child in _children) { - if (child != null) + if (!_detachedChildren.contains(child)) visitor(child); } } bool detachChild(Element child) { - _detachedChildren ??= new Set(); _detachedChildren.add(child); _deactivateChild(child); return true; @@ -1888,8 +1882,8 @@ class MultiChildRenderObjectElement exte void update(T newWidget) { super.update(newWidget); assert(widget == newWidget); - _replaceDetachedChildrenWithNull(); - _children = updateChildren(_children, widget.children); + _children = updateChildren(_children, widget.children, detachedChildren: _detachedChildren); + _detachedChildren.clear(); } } diff --git a/packages/flutter/test/widget/reparent_state_test.dart b/packages/flutter/test/widget/reparent_state_test.dart index c3448267115..99a84e01e6d 100644 --- a/packages/flutter/test/widget/reparent_state_test.dart +++ b/packages/flutter/test/widget/reparent_state_test.dart @@ -196,4 +196,99 @@ void main() { expect(keyState.marker, equals("marked")); }); }); + + test('Reparent during update children', () { + testWidgets((WidgetTester tester) { + GlobalKey key = new GlobalKey(); + + tester.pumpWidget(new Stack( + children: [ + new StateMarker(key: key), + new Container(width: 100.0, height: 100.0), + ] + )); + + StateMarkerState keyState = key.currentState; + keyState.marker = "marked"; + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0), + new StateMarker(key: key), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + + tester.pumpWidget(new Stack( + children: [ + new StateMarker(key: key), + new Container(width: 100.0, height: 100.0), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + }); + }); + + test('Reparent to child during update children', () { + testWidgets((WidgetTester tester) { + GlobalKey key = new GlobalKey(); + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0), + new StateMarker(key: key), + new Container(width: 100.0, height: 100.0), + ] + )); + + StateMarkerState keyState = key.currentState; + keyState.marker = "marked"; + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0, child: new StateMarker(key: key)), + new Container(width: 100.0, height: 100.0), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0), + new StateMarker(key: key), + new Container(width: 100.0, height: 100.0), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0), + new Container(width: 100.0, height: 100.0, child: new StateMarker(key: key)), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + + tester.pumpWidget(new Stack( + children: [ + new Container(width: 100.0, height: 100.0), + new StateMarker(key: key), + new Container(width: 100.0, height: 100.0), + ] + )); + + expect(key.currentState, equals(keyState)); + expect(keyState.marker, equals("marked")); + }); + }); }