diff --git a/sky/packages/sky/lib/src/widgets/framework.dart b/sky/packages/sky/lib/src/widgets/framework.dart index 39caff46af1adc1f9798a7e9ca89cd56fd78a05c..765513676d5586f04bc2409210407e2942990d73 100644 --- a/sky/packages/sky/lib/src/widgets/framework.dart +++ b/sky/packages/sky/lib/src/widgets/framework.dart @@ -212,6 +212,15 @@ abstract class Widget { /// The parent of this widget in the widget tree. Widget get parent => _parent; + // The "generation" of a Widget is the frame in which it was last + // synced. We use this to tell if an instance of a Widget has moved + // to earlier in the tree so that when we come across where it used + // to be, we pretend it was never there. See syncChild(). + static int _currentGeneration = 1; + int _generation = 0; + bool get isFromOldGeneration => _generation < _currentGeneration; + void _markAsFromCurrentGeneration() { _generation = _currentGeneration; } + bool _mounted = false; bool _wasMounted = false; bool get mounted => _mounted; @@ -305,6 +314,11 @@ abstract class Widget { bool retainStatefulNodeIfPossible(Widget newNode) => false; void _sync(Widget old, dynamic slot) { + assert(isFromOldGeneration); + assert(old == null || old.isFromOldGeneration); + _markAsFromCurrentGeneration(); + if (old != null && old != this) + old._markAsFromCurrentGeneration(); if (key is GlobalKey) (key as GlobalKey)._didSync(); // TODO(ianh): Remove the cast once the analyzer is cleverer. } @@ -342,35 +356,63 @@ abstract class Widget { } void remove() { - walkChildren((Widget child) => child.remove()); + walkChildren((Widget child) { + if (child._generation <= _generation) + child.remove(); + }); _renderObject = null; setParent(null); } void detachRenderObject(); + Widget _getCandidateSingleChildFrom(Widget oldChild) { + Widget candidate = oldChild.singleChild; + if (candidate != null && !candidate.isFromOldGeneration) + candidate = null; + assert(candidate == null || candidate.parent == oldChild); + return candidate; + } + // Returns the child which should be retained as the child of this node. Widget syncChild(Widget newNode, Widget oldNode, dynamic slot) { + assert(() { + 'You have probably used a single instance of a Widget in two different places in the widget tree. Widgets can only be used in one place at a time.'; + return newNode == null || newNode.isFromOldGeneration; + }); + + if (oldNode != null && !oldNode.isFromOldGeneration) + oldNode = null; + if (newNode == oldNode) { - assert(newNode == null || newNode.mounted); + assert(newNode == null || newNode.isFromOldGeneration); assert(newNode is! RenderObjectWrapper || (newNode is RenderObjectWrapper && newNode._ancestor != null)); // TODO(ianh): Simplify this once the analyzer is cleverer - if (newNode != null) + if (newNode != null) { newNode.setParent(this); + newNode._markAsFromCurrentGeneration(); + } return newNode; // Nothing to do. Subtrees must be identical. } if (newNode == null) { - // the child in this slot has gone away + // the child in this slot has gone away (we know oldNode != null) + assert(oldNode != null); + assert(oldNode.isFromOldGeneration); assert(oldNode.mounted); oldNode.detachRenderObject(); oldNode.remove(); assert(!oldNode.mounted); + // we don't update the generation of oldNode, because there's + // still a chance it could be reused as-is later in the tree. return null; } if (oldNode != null) { + assert(newNode != null); + assert(newNode.isFromOldGeneration); + assert(oldNode.isFromOldGeneration); if (!_canSync(newNode, oldNode)) { assert(oldNode.mounted); // We want to handle the case where there is a removal of zero @@ -378,8 +420,7 @@ abstract class Widget { // ourselves with a Widget that is a descendant of the // oldNode, skipping the nodes in between. Let's try that. Widget deadNode = oldNode; - Widget candidate = oldNode.singleChild; - assert(candidate == null || candidate.parent == oldNode); + Widget candidate = _getCandidateSingleChildFrom(oldNode); oldNode = null; while (candidate != null && _shouldReparentDuringSync) { if (_canSync(newNode, candidate)) { @@ -389,13 +430,15 @@ abstract class Widget { oldNode = candidate; break; } - assert(candidate.singleChild == null || candidate.singleChild.parent == candidate); - candidate = candidate.singleChild; + candidate = _getCandidateSingleChildFrom(candidate); } deadNode.detachRenderObject(); deadNode.remove(); } if (oldNode != null) { + assert(newNode.isFromOldGeneration); + assert(oldNode.isFromOldGeneration); + assert(_canSync(newNode, oldNode)); if (oldNode.retainStatefulNodeIfPossible(newNode)) { assert(oldNode.mounted); assert(!newNode.mounted); @@ -410,7 +453,6 @@ abstract class Widget { } assert(oldNode == null || (oldNode.mounted == false && oldNode.parent == null)); - assert(!newNode.mounted); newNode.setParent(this); newNode._sync(oldNode, slot); assert(newNode.renderObject is RenderObject); @@ -439,7 +481,13 @@ abstract class Widget { nextPrefix = prefix + ' '; childrenString += lastChild.toString(nextPrefix, _adjustPrefixWithParentCheck(lastChild, nextStartPrefix)); } - return '$startPrefix${toStringName()}\n$childrenString'; + String suffix = ''; + if (_generation != _currentGeneration) { + int delta = _generation - _currentGeneration; + String sign = delta < 0 ? '' : '+'; + suffix = ' gen$sign$delta'; + } + return '$startPrefix${toStringName()}$suffix\n$childrenString'; } String toStringName() { if (key == null) @@ -465,7 +513,9 @@ abstract class Widget { } bool _canSync(Widget a, Widget b) { - return a.runtimeType == b.runtimeType && a.key == b.key; + return a.runtimeType == b.runtimeType && + a.key == b.key && + (a._generation == 0 || b._generation == 0); } @@ -786,6 +836,7 @@ abstract class Component extends Widget { void _buildIfDirty() { if (!_dirty || !_mounted) return; + assert(isFromOldGeneration); assert(renderObject != null); _sync(null, _slot); } @@ -900,7 +951,7 @@ void exitLayoutCallbackBuilder(LayoutCallbackBuilderHandle handle) { _inLayoutCallbackBuilder -= 1; return true; }); - Widget._notifyMountStatusChanged(); + _endSyncPhase(); } List _debugFrameTimes = []; @@ -942,8 +993,7 @@ void _buildDirtyComponents() { _inBuildDirtyComponents = false; sky.tracing.end('Component.flushBuild'); } - - Widget._notifyMountStatusChanged(); + _endSyncPhase(); } if (_shouldLogRenderDuration) { @@ -956,6 +1006,12 @@ void _buildDirtyComponents() { _debugFrameTimes.clear(); } } + +} + +void _endSyncPhase() { + Widget._currentGeneration += 1; + Widget._notifyMountStatusChanged(); } void _scheduleComponentForRender(Component component) { @@ -1111,9 +1167,10 @@ abstract class RenderObjectWrapper extends Widget { // top of the lists while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) { Widget oldChild = oldChildren[childrenTop]; + if (!oldChild.isFromOldGeneration) + break; assert(oldChild.mounted); Widget newChild = newChildren[childrenTop]; - assert(newChild == oldChild || !newChild.mounted); if (!_canSync(oldChild, newChild)) break; childrenTop += 1; @@ -1124,9 +1181,10 @@ abstract class RenderObjectWrapper extends Widget { // bottom of the lists while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) { Widget oldChild = oldChildren[oldChildrenBottom]; + if (!oldChild.isFromOldGeneration) + break; assert(oldChild.mounted); Widget newChild = newChildren[newChildrenBottom]; - assert(newChild == oldChild || !newChild.mounted); if (!_canSync(oldChild, newChild)) break; newChild = syncChild(newChild, oldChild, nextSibling); @@ -1145,13 +1203,15 @@ abstract class RenderObjectWrapper extends Widget { oldKeyedChildren = new Map(); while (childrenTop <= oldChildrenBottom) { Widget oldChild = oldChildren[oldChildrenBottom]; - assert(oldChild.mounted); - if (oldChild.key != null) { - oldKeyedChildren[oldChild.key] = oldChild; - } else { - syncChild(null, oldChild, null); + if (oldChild.isFromOldGeneration) { + assert(oldChild.mounted); + if (oldChild.key != null) { + oldKeyedChildren[oldChild.key] = oldChild; + } else { + syncChild(null, oldChild, null); + } + oldChildrenBottom -= 1; } - oldChildrenBottom -= 1; } } @@ -1163,7 +1223,7 @@ abstract class RenderObjectWrapper extends Widget { Key key = newChild.key; if (key != null) { oldChild = oldKeyedChildren[newChild.key]; - if (oldChild != null) { + if (oldChild != null && oldChild.isFromOldGeneration) { if (oldChild.runtimeType != newChild.runtimeType) oldChild = null; oldKeyedChildren.remove(key); @@ -1186,8 +1246,9 @@ abstract class RenderObjectWrapper extends Widget { childrenTop -= 1; Widget oldChild = oldChildren[childrenTop]; assert(oldChild.mounted); + assert(oldChild.isFromOldGeneration); Widget newChild = newChildren[childrenTop]; - assert(newChild == oldChild || !newChild.mounted); + assert(newChild.isFromOldGeneration); assert(_canSync(oldChild, newChild)); newChild = syncChild(newChild, oldChild, nextSibling); assert(newChild.mounted); @@ -1198,7 +1259,8 @@ abstract class RenderObjectWrapper extends Widget { if (haveOldNodes && !oldKeyedChildren.isEmpty) { for (Widget oldChild in oldKeyedChildren.values) - syncChild(null, oldChild, null); + if (oldChild.isFromOldGeneration) + syncChild(null, oldChild, null); } assert(renderObject == this.renderObject); // TODO(ianh): Remove this once the analyzer is cleverer diff --git a/sky/packages/sky/lib/src/widgets/mixed_viewport.dart b/sky/packages/sky/lib/src/widgets/mixed_viewport.dart index dd2b4203f3b4d37ff6c01d1fc962f53c72c64286..2eab1222d3a83fc6be55b3111979176178e2ed46 100644 --- a/sky/packages/sky/lib/src/widgets/mixed_viewport.dart +++ b/sky/packages/sky/lib/src/widgets/mixed_viewport.dart @@ -201,9 +201,15 @@ class MixedViewport extends RenderObjectWrapper { Widget widget = builder(index); assert(widget != null); assert(widget.key != null); + assert(widget.isFromOldGeneration); _Key key = new _Key.fromWidget(widget); Widget oldWidget = _childrenByKey[key]; assert(oldWidget != null); + assert(() { + 'One of the nodes that was in this MixedViewport was placed in another part of the tree, without the MixedViewport\'s token or builder being changed ' + + 'and without the MixedViewport\'s MixedViewportLayoutState object being told about that any of the children were invalid.'; + return oldWidget.isFromOldGeneration; + }); assert(oldWidget.renderObject.parent == renderObject); widget = syncChild(widget, oldWidget, renderObject.childAfter(oldWidget.renderObject)); assert(widget != null); @@ -225,8 +231,11 @@ class MixedViewport extends RenderObjectWrapper { Widget newWidget = builder(index); assert(newWidget != null); assert(newWidget.key != null); + assert(newWidget.isFromOldGeneration); final _Key key = new _Key.fromWidget(newWidget); Widget oldWidget = _childrenByKey[key]; + if (oldWidget != null && !oldWidget.isFromOldGeneration) + oldWidget = null; newWidget = syncChild(newWidget, oldWidget, _omit); assert(newWidget != null); // Update the offsets based on the newWidget's dimensions. @@ -254,6 +263,8 @@ class MixedViewport extends RenderObjectWrapper { assert(widget.key != null); // items in lists must have keys final _Key key = new _Key.fromWidget(widget); Widget oldWidget = _childrenByKey[key]; + if (oldWidget != null && !oldWidget.isFromOldGeneration) + oldWidget = null; widget = syncChild(widget, oldWidget, _omit); if (index >= offsets.length - 1) { assert(index == offsets.length - 1); @@ -286,6 +297,18 @@ class MixedViewport extends RenderObjectWrapper { layoutState._notifyListeners(); } + void _unsyncChild(Widget widget) { + assert(!widget.isFromOldGeneration); + // The following two lines are the equivalent of "syncChild(null, + // widget, null)", but actually doing that wouldn't work because + // widget is now from the new generation and so syncChild() would + // assume that that means someone else has already sync()ed with it + // and that it's wanted. But it's not wanted! We want to get rid of + // it. So we do it manually. + widget.detachRenderObject(); + widget.remove(); + } + void _doLayout(BoxConstraints constraints) { Map<_Key, Widget> newChildren = new Map<_Key, Widget>(); Map builtChildren = new Map(); @@ -334,7 +357,7 @@ class MixedViewport extends RenderObjectWrapper { builtChildren[index] = widget; } else { childrenByKey.remove(widgetKey); - syncChild(null, widget, null); + _unsyncChild(widget); } } } @@ -378,7 +401,7 @@ class MixedViewport extends RenderObjectWrapper { break; } childrenByKey.remove(widgetKey); - syncChild(null, widget, null); + _unsyncChild(widget); startIndex += 1; assert(startIndex == offsets.length - 1); } diff --git a/sky/unit/test/widget/stack_test.dart b/sky/unit/test/widget/stack_test.dart index 721aa2e57b3a79f72a822556acc360459bb52752..97a327190d3b16831782e183bb93704a3da11e5f 100644 --- a/sky/unit/test/widget/stack_test.dart +++ b/sky/unit/test/widget/stack_test.dart @@ -43,4 +43,29 @@ void main() { expect(container.renderObject.parentData.bottom, isNull); expect(container.renderObject.parentData.left, isNull); }); + + test('Can remove parent data', () { + WidgetTester tester = new WidgetTester(); + Container container; + + tester.pumpFrame(() { + container = new Container(width: 10.0, height: 10.0); + return new Stack([ new Positioned(left: 10.0, child: container) ]); + }); + + expect(container.renderObject.parentData.top, isNull); + expect(container.renderObject.parentData.right, isNull); + expect(container.renderObject.parentData.bottom, isNull); + expect(container.renderObject.parentData.left, equals(10.0)); + + tester.pumpFrame(() { + return new Stack([ container ]); + }); + + expect(container.renderObject.parentData.top, isNull); + expect(container.renderObject.parentData.right, isNull); + expect(container.renderObject.parentData.bottom, isNull); + expect(container.renderObject.parentData.left, isNull); + }); + } diff --git a/sky/unit/test/widget/syncing_test.dart b/sky/unit/test/widget/syncing_test.dart index b478ebc194064583199037fafe2f1d79a7c8257f..4d882bf682b5617bf82b0a4035141e748dd70e73 100644 --- a/sky/unit/test/widget/syncing_test.dart +++ b/sky/unit/test/widget/syncing_test.dart @@ -4,13 +4,15 @@ import 'package:test/test.dart'; import 'widget_tester.dart'; class TestState extends StatefulComponent { - TestState({this.child, this.state}); + TestState({ this.child, this.persistentState, this.syncedState }); Widget child; - int state; + int persistentState; + int syncedState; int syncs = 0; void syncConstructorArguments(TestState source) { child = source.child; - // we explicitly do NOT sync the state from the new instance + syncedState = source.syncedState; + // we explicitly do NOT sync the persistentState from the new instance // because we're using that to track whether we got recreated syncs += 1; } @@ -29,7 +31,7 @@ void main() { return new Container( child: new Container( child: new TestState( - state: 1, + persistentState: 1, child: new Container() ) ) @@ -38,21 +40,21 @@ void main() { TestState stateWidget = tester.findWidget((widget) => widget is TestState); - expect(stateWidget.state, equals(1)); + expect(stateWidget.persistentState, equals(1)); expect(stateWidget.syncs, equals(0)); tester.pumpFrame(() { return new Container( child: new Container( child: new TestState( - state: 2, + persistentState: 2, child: new Container() ) ) ); }); - expect(stateWidget.state, equals(1)); + expect(stateWidget.persistentState, equals(1)); expect(stateWidget.syncs, equals(1)); }); @@ -92,4 +94,94 @@ void main() { // // }); + test('swap instances around', () { + + WidgetTester tester = new WidgetTester(); + + Widget a, b; + tester.pumpFrame(() { + a = new TestState(persistentState: 0x61, syncedState: 0x41, child: new Text('apple')); + b = new TestState(persistentState: 0x62, syncedState: 0x42, child: new Text('banana')); + return new Column([]); + }); + GlobalKey keyA = new GlobalKey(); + GlobalKey keyB = new GlobalKey(); + + TestState foundA, foundB; + + tester.pumpFrame(() { + return new Column([ + new Container( + key: keyA, + child: a + ), + new Container( + key: keyB, + child: b + ) + ]); + }); + + foundA = (tester.findWidget((widget) => widget.key == keyA) as Container).child as TestState; + foundB = (tester.findWidget((widget) => widget.key == keyB) as Container).child as TestState; + + expect(foundA, equals(a)); + expect(foundA.persistentState, equals(0x61)); + expect(foundA.syncedState, equals(0x41)); + expect(foundB, equals(b)); + expect(foundB.persistentState, equals(0x62)); + expect(foundB.syncedState, equals(0x42)); + + tester.pumpFrame(() { + return new Column([ + new Container( + key: keyA, + child: a + ), + new Container( + key: keyB, + child: b + ) + ]); + }); + + foundA = (tester.findWidget((widget) => widget.key == keyA) as Container).child as TestState; + foundB = (tester.findWidget((widget) => widget.key == keyB) as Container).child as TestState; + + // same as before + expect(foundA, equals(a)); + expect(foundA.persistentState, equals(0x61)); + expect(foundA.syncedState, equals(0x41)); + expect(foundB, equals(b)); + expect(foundB.persistentState, equals(0x62)); + expect(foundB.syncedState, equals(0x42)); + + // now we swap the nodes over + // since they are both "old" nodes, they shouldn't sync with each other even though they look alike + + tester.pumpFrame(() { + return new Column([ + new Container( + key: keyA, + child: b + ), + new Container( + key: keyB, + child: a + ) + ]); + }); + + foundA = (tester.findWidget((widget) => widget.key == keyA) as Container).child as TestState; + foundB = (tester.findWidget((widget) => widget.key == keyB) as Container).child as TestState; + + expect(foundA, equals(b)); + expect(foundA.persistentState, equals(0x62)); + expect(foundA.syncedState, equals(0x42)); + expect(foundB, equals(a)); + expect(foundB.persistentState, equals(0x61)); + expect(foundB.syncedState, equals(0x41)); + + }); + }