Commit 5125bd5d authored by Hixie's avatar Hixie

Change how we decide if two nodes can sync.

If it's a StatefulComponent, then it's ok to reuse it so long as it
hasn't been initialised.

If it's a regular Component or a TagNode, then it's always ok to reuse.

If it's a RenderObjectWrapper, then it's ok to reuse so long as it
doesn't have a renderObject.

To put it another way, this changes how we prevent the following
nonsensical situations from arising:

 - Sync two stateful StatefulComponents together
 - Sync two RenderObjectWrappers with RenderObjects together

When either of those cases happen, we just drop the old one on the
ground and use the new one unchanged.
parent 639f9d9b
...@@ -220,6 +220,15 @@ abstract class Widget { ...@@ -220,6 +220,15 @@ abstract class Widget {
bool get isFromOldGeneration => _generation < _currentGeneration; bool get isFromOldGeneration => _generation < _currentGeneration;
void _markAsFromCurrentGeneration() { _generation = _currentGeneration; } void _markAsFromCurrentGeneration() { _generation = _currentGeneration; }
bool get _hasState => false;
static bool _canSync(Widget a, Widget b) {
assert(a != null);
assert(b != null);
return a.runtimeType == b.runtimeType &&
a.key == b.key &&
(!a._hasState || !b._hasState);
}
bool _mounted = false; bool _mounted = false;
bool _wasMounted = false; bool _wasMounted = false;
bool get mounted => _mounted; bool get mounted => _mounted;
...@@ -310,6 +319,9 @@ abstract class Widget { ...@@ -310,6 +319,9 @@ abstract class Widget {
// if the node has become stateful and should be retained. // if the node has become stateful and should be retained.
// This is called immediately before _sync(). // This is called immediately before _sync().
// Component.retainStatefulNodeIfPossible() calls syncConstructorArguments(). // Component.retainStatefulNodeIfPossible() calls syncConstructorArguments().
// This is only ever called on a node A with an argument B if _canSync(A, B)
// is true. It's ok if after calling retainStatefulNodeIfPossible(), the two
// nodes no longer return true from _canSync().
bool retainStatefulNodeIfPossible(Widget newNode) => false; bool retainStatefulNodeIfPossible(Widget newNode) => false;
void _sync(Widget old, dynamic slot) { void _sync(Widget old, dynamic slot) {
...@@ -412,7 +424,7 @@ abstract class Widget { ...@@ -412,7 +424,7 @@ abstract class Widget {
assert(newNode != null); assert(newNode != null);
assert(newNode.isFromOldGeneration); assert(newNode.isFromOldGeneration);
assert(oldNode.isFromOldGeneration); assert(oldNode.isFromOldGeneration);
if (!_canSync(newNode, oldNode)) { if (!Widget._canSync(newNode, oldNode)) {
assert(oldNode.mounted); assert(oldNode.mounted);
// We want to handle the case where there is a removal of zero // We want to handle the case where there is a removal of zero
// or more widgets. In this case, we should be able to sync // or more widgets. In this case, we should be able to sync
...@@ -423,7 +435,7 @@ abstract class Widget { ...@@ -423,7 +435,7 @@ abstract class Widget {
oldNode = null; oldNode = null;
while (candidate != null) { while (candidate != null) {
if (_canSync(newNode, candidate)) { if (Widget._canSync(newNode, candidate)) {
assert(candidate.parent != null); assert(candidate.parent != null);
assert(candidate.parent.singleChild == candidate); assert(candidate.parent.singleChild == candidate);
if (candidate.renderObject != deadNode.renderObject) { if (candidate.renderObject != deadNode.renderObject) {
...@@ -446,7 +458,7 @@ abstract class Widget { ...@@ -446,7 +458,7 @@ abstract class Widget {
if (oldNode != null) { if (oldNode != null) {
assert(newNode.isFromOldGeneration); assert(newNode.isFromOldGeneration);
assert(oldNode.isFromOldGeneration); assert(oldNode.isFromOldGeneration);
assert(_canSync(newNode, oldNode)); assert(Widget._canSync(newNode, oldNode));
if (oldNode.retainStatefulNodeIfPossible(newNode)) { if (oldNode.retainStatefulNodeIfPossible(newNode)) {
assert(oldNode.mounted); assert(oldNode.mounted);
assert(!newNode.mounted); assert(!newNode.mounted);
...@@ -520,12 +532,6 @@ abstract class Widget { ...@@ -520,12 +532,6 @@ abstract class Widget {
} }
} }
bool _canSync(Widget a, Widget b) {
return a.runtimeType == b.runtimeType &&
a.key == b.key &&
(a._generation == 0 || b._generation == 0);
}
// Descendants of TagNode provide a way to tag RenderObjectWrapper and // Descendants of TagNode provide a way to tag RenderObjectWrapper and
// Component nodes with annotations, such as event listeners, // Component nodes with annotations, such as event listeners,
...@@ -881,10 +887,12 @@ abstract class StatefulComponent extends Component { ...@@ -881,10 +887,12 @@ abstract class StatefulComponent extends Component {
bool _isStateInitialized = false; bool _isStateInitialized = false;
bool get _hasState => _isStateInitialized;
bool retainStatefulNodeIfPossible(StatefulComponent newNode) { bool retainStatefulNodeIfPossible(StatefulComponent newNode) {
assert(newNode != null); assert(newNode != null);
assert(!newNode._isStateInitialized); assert(!newNode._isStateInitialized);
assert(_canSync(this, newNode)); assert(Widget._canSync(this, newNode));
assert(_child != null); assert(_child != null);
newNode._child = _child; newNode._child = _child;
...@@ -1062,6 +1070,8 @@ abstract class RenderObjectWrapper extends Widget { ...@@ -1062,6 +1070,8 @@ abstract class RenderObjectWrapper extends Widget {
void insertChildRenderObject(RenderObjectWrapper child, dynamic slot); void insertChildRenderObject(RenderObjectWrapper child, dynamic slot);
void detachChildRenderObject(RenderObjectWrapper child); void detachChildRenderObject(RenderObjectWrapper child);
bool get _hasState => _renderObject != null;
void retainStatefulRenderObjectWrapper(RenderObjectWrapper newNode) { void retainStatefulRenderObjectWrapper(RenderObjectWrapper newNode) {
newNode._renderObject = _renderObject; newNode._renderObject = _renderObject;
newNode._ancestor = _ancestor; newNode._ancestor = _ancestor;
...@@ -1079,7 +1089,6 @@ abstract class RenderObjectWrapper extends Widget { ...@@ -1079,7 +1089,6 @@ abstract class RenderObjectWrapper extends Widget {
if (_ancestor is RenderObjectWrapper) if (_ancestor is RenderObjectWrapper)
_ancestor.insertChildRenderObject(this, slot); _ancestor.insertChildRenderObject(this, slot);
} else { } else {
assert(_canSync(this, old));
_renderObject = old.renderObject; _renderObject = old.renderObject;
_ancestor = old._ancestor; _ancestor = old._ancestor;
assert(_renderObject != null); assert(_renderObject != null);
...@@ -1181,7 +1190,7 @@ abstract class RenderObjectWrapper extends Widget { ...@@ -1181,7 +1190,7 @@ abstract class RenderObjectWrapper extends Widget {
break; break;
assert(oldChild.mounted); assert(oldChild.mounted);
Widget newChild = newChildren[childrenTop]; Widget newChild = newChildren[childrenTop];
if (!_canSync(oldChild, newChild)) if (!Widget._canSync(oldChild, newChild))
break; break;
childrenTop += 1; childrenTop += 1;
} }
...@@ -1195,7 +1204,7 @@ abstract class RenderObjectWrapper extends Widget { ...@@ -1195,7 +1204,7 @@ abstract class RenderObjectWrapper extends Widget {
break; break;
assert(oldChild.mounted); assert(oldChild.mounted);
Widget newChild = newChildren[newChildrenBottom]; Widget newChild = newChildren[newChildrenBottom];
if (!_canSync(oldChild, newChild)) if (!Widget._canSync(oldChild, newChild))
break; break;
newChild = syncChild(newChild, oldChild, nextSibling); newChild = syncChild(newChild, oldChild, nextSibling);
assert(newChild.mounted); assert(newChild.mounted);
...@@ -1259,7 +1268,7 @@ abstract class RenderObjectWrapper extends Widget { ...@@ -1259,7 +1268,7 @@ abstract class RenderObjectWrapper extends Widget {
assert(oldChild.isFromOldGeneration); assert(oldChild.isFromOldGeneration);
Widget newChild = newChildren[childrenTop]; Widget newChild = newChildren[childrenTop];
assert(newChild.isFromOldGeneration); assert(newChild.isFromOldGeneration);
assert(_canSync(oldChild, newChild)); assert(Widget._canSync(oldChild, newChild));
newChild = syncChild(newChild, oldChild, nextSibling); newChild = syncChild(newChild, oldChild, nextSibling);
assert(newChild.mounted); assert(newChild.mounted);
assert(oldChild == newChild || oldChild == null || !oldChild.mounted); assert(oldChild == newChild || oldChild == null || !oldChild.mounted);
......
...@@ -32,8 +32,10 @@ class HomogeneousViewport extends RenderObjectWrapper { ...@@ -32,8 +32,10 @@ class HomogeneousViewport extends RenderObjectWrapper {
ScrollDirection direction; ScrollDirection direction;
double startOffset; double startOffset;
bool _layoutDirty = true;
List<Widget> _children; List<Widget> _children;
bool _layoutDirty = true;
int _layoutFirstIndex;
int _layoutItemCount;
RenderBlockViewport get renderObject => super.renderObject; RenderBlockViewport get renderObject => super.renderObject;
...@@ -56,10 +58,16 @@ class HomogeneousViewport extends RenderObjectWrapper { ...@@ -56,10 +58,16 @@ class HomogeneousViewport extends RenderObjectWrapper {
super.remove(); super.remove();
_children.clear(); _children.clear();
_layoutDirty = true; _layoutDirty = true;
assert(() {
_layoutFirstIndex = null;
_layoutItemCount = null;
return true;
});
} }
void walkChildren(WidgetTreeWalker walker) { void walkChildren(WidgetTreeWalker walker) {
if (_children == null) return; if (_children == null)
return;
for (Widget child in _children) for (Widget child in _children)
walker(child); walker(child);
} }
...@@ -113,9 +121,6 @@ class HomogeneousViewport extends RenderObjectWrapper { ...@@ -113,9 +121,6 @@ class HomogeneousViewport extends RenderObjectWrapper {
} }
} }
int _layoutFirstIndex;
int _layoutItemCount;
void layout(BoxConstraints constraints) { void layout(BoxConstraints constraints) {
LayoutCallbackBuilderHandle handle = enterLayoutCallbackBuilder(); LayoutCallbackBuilderHandle handle = enterLayoutCallbackBuilder();
try { try {
......
...@@ -78,8 +78,9 @@ class MixedViewportLayoutState { ...@@ -78,8 +78,9 @@ class MixedViewportLayoutState {
} }
class MixedViewport extends RenderObjectWrapper { class MixedViewport extends RenderObjectWrapper {
MixedViewport({ Key key, this.startOffset, this.direction: ScrollDirection.vertical, this.builder, this.token, this.layoutState }) MixedViewport({ this.startOffset, this.direction: ScrollDirection.vertical, this.builder, this.token, MixedViewportLayoutState layoutState })
: super(key: key) { : layoutState = layoutState, super(key: new ObjectKey(layoutState)) {
// using the layout state as the key is important to prevent us from being synced with someone with a different layout state
assert(this.layoutState != null); assert(this.layoutState != null);
} }
......
import 'package:sky/animation.dart';
import 'package:sky/widgets.dart';
import 'package:test/test.dart';
import 'widget_tester.dart';
class SecondComponent extends StatefulComponent {
SecondComponent(this.navigator);
Navigator navigator;
void syncConstructorArguments(SecondComponent source) {
navigator = source.navigator;
}
Widget build() {
return new GestureDetector(
onTap: navigator.pop,
child: new Container(
decoration: new BoxDecoration(
backgroundColor: new Color(0xFFFF00FF)
),
child: new Text('Y')
)
);
}
}
class FirstComponent extends Component {
FirstComponent(this.navigator);
final Navigator navigator;
Widget build() {
return new GestureDetector(
onTap: () {
navigator.pushNamed('/second');
},
child: new Container(
decoration: new BoxDecoration(
backgroundColor: new Color(0xFFFFFF00)
),
child: new Text('X')
)
);
}
}
void main() {
test('Can navigator to and from a stateful component', () {
WidgetTester tester = new WidgetTester();
final NavigationState routes = new NavigationState([
new Route(
name: '/',
builder: (navigator, route) => new FirstComponent(navigator)
),
new Route(
name: '/second',
builder: (navigator, route) => new SecondComponent(navigator)
)
]);
tester.pumpFrame(() {
return new Navigator(routes);
});
tester.tap(tester.findText('X'));
scheduler.beginFrame(10.0);
scheduler.beginFrame(20.0);
scheduler.beginFrame(30.0);
scheduler.beginFrame(1000.0);
tester.tap(tester.findText('Y'));
scheduler.beginFrame(1010.0);
scheduler.beginFrame(1020.0);
scheduler.beginFrame(1030.0);
scheduler.beginFrame(2000.0);
});
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment