From 8bd778175561f315b9d809da3aa63cc0bf254437 Mon Sep 17 00:00:00 2001 From: Hixie Date: Wed, 17 Jun 2015 13:56:06 -0700 Subject: [PATCH] Clean up how we remove nodes from the render tree, and make sure we reinsert them in the right place. Fixes weird behaviours when the old and new children of TagNodes can't be synced. R=abarth@chromium.org Review URL: https://codereview.chromium.org/1182463006. --- sdk/lib/widgets/scaffold.dart | 10 +-- sdk/lib/widgets/widget.dart | 68 +++++++++++++------ tests/widgets/syncs1-expected.txt | 108 ++++++++++++++++++++++++++++++ tests/widgets/syncs1.dart | 100 +++++++++++++++++++++++++++ 4 files changed, 263 insertions(+), 23 deletions(-) create mode 100644 tests/widgets/syncs1-expected.txt create mode 100644 tests/widgets/syncs1.dart diff --git a/sdk/lib/widgets/scaffold.dart b/sdk/lib/widgets/scaffold.dart index ae750c573..b9b5e508e 100644 --- a/sdk/lib/widgets/scaffold.dart +++ b/sdk/lib/widgets/scaffold.dart @@ -185,10 +185,12 @@ class Scaffold extends RenderObjectWrapper { root[slot] = child != null ? child.root : null; } - void removeChild(Widget node) { - assert(node != null); - root.remove(node.root); - super.removeChild(node); + void detachChildRoot(RenderObjectWrapper child) { + final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer + assert(root is RenderScaffold); + assert(root == child.root.parent); + root.remove(child.root); + assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer } void remove() { diff --git a/sdk/lib/widgets/widget.dart b/sdk/lib/widgets/widget.dart index e96f3d22d..11fb763a7 100644 --- a/sdk/lib/widgets/widget.dart +++ b/sdk/lib/widgets/widget.dart @@ -90,11 +90,6 @@ abstract class Widget { // 'slot' is the identifier that the parent RenderObjectWrapper uses to know // where to put this descendant - void remove() { - _root = null; - setParent(null); - } - Widget findAncestor(Type targetType) { var ancestor = _parent; while (ancestor != null && !reflectClass(ancestor.runtimeType).isSubtypeOf(reflectClass(targetType))) @@ -102,10 +97,17 @@ abstract class Widget { return ancestor; } + void remove() { + _root = null; + setParent(null); + } + void removeChild(Widget node) { node.remove(); } + void detachRoot(); + // Returns the child which should be retained as the child of this node. Widget syncChild(Widget node, Widget oldNode, dynamic slot) { @@ -113,12 +115,14 @@ abstract class Widget { if (node == oldNode) { assert(node == null || node.mounted); + assert(node is! RenderObjectWrapper || node._ancestor != null); return node; // Nothing to do. Subtrees must be identical. } if (node == null) { // the child in this slot has gone away assert(oldNode.mounted); + oldNode.detachRoot(); removeChild(oldNode); assert(!oldNode.mounted); return null; @@ -138,6 +142,7 @@ abstract class Widget { if (oldNode != null && (oldNode.runtimeType != node.runtimeType || oldNode.key != node.key)) { assert(oldNode.mounted); + oldNode.detachRoot(); removeChild(oldNode); oldNode = null; } @@ -148,6 +153,9 @@ abstract class Widget { assert(node.root is RenderObject); return node; } + + String toString() => '$runtimeType($key)'; + } @@ -175,6 +183,11 @@ abstract class TagNode extends Widget { super.remove(); } + void detachRoot() { + if (content != null) + content.detachRoot(); + } + } class ParentDataNode extends TagNode { @@ -305,6 +318,12 @@ abstract class Component extends Widget { super.remove(); } + void detachRoot() { + assert(_built != null); + assert(root != null); + _built.detachRoot(); + } + bool _retainStatefulNodeIfPossible(Widget old) { assert(!_disqualifiedFromEverAppearingAgain); @@ -466,13 +485,14 @@ abstract class RenderObjectWrapper extends Widget { RenderObject createNode(); - void insert(RenderObjectWrapper child, dynamic slot); - static final Map _nodeMap = new HashMap(); - static RenderObjectWrapper _getMounted(RenderObject node) => _nodeMap[node]; + RenderObjectWrapper _ancestor; + void insert(RenderObjectWrapper child, dynamic slot); + void detachChildRoot(RenderObjectWrapper child); + void _sync(Widget old, dynamic slot) { // TODO(abarth): We should split RenderObjectWrapper into two pieces so that // RenderViewObject doesn't need to inherit all this code it @@ -480,11 +500,14 @@ abstract class RenderObjectWrapper extends Widget { assert(parent != null || this is RenderViewWrapper); if (old == null) { _root = createNode(); - var ancestor = findAncestor(RenderObjectWrapper); - if (ancestor is RenderObjectWrapper) - ancestor.insert(this, slot); + _ancestor = findAncestor(RenderObjectWrapper); + if (_ancestor is RenderObjectWrapper) + _ancestor.insert(this, slot); + else + print("$this has no ancestor"); } else { _root = old.root; + _ancestor = old._ancestor; } assert(_root == root); // in case a subclass reintroduces it assert(root != null); @@ -518,6 +541,13 @@ abstract class RenderObjectWrapper extends Widget { _nodeMap.remove(root); super.remove(); } + + void detachRoot() { + assert(_ancestor != null); + assert(root != null); + _ancestor.detachChildRoot(this); + } + } abstract class OneChildRenderObjectWrapper extends RenderObjectWrapper { @@ -536,17 +566,17 @@ abstract class OneChildRenderObjectWrapper extends RenderObjectWrapper { void insert(RenderObjectWrapper child, dynamic slot) { final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer - assert(slot == null); assert(root is RenderObjectWithChildMixin); + assert(slot == null); root.child = child.root; assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer } - void removeChild(Widget node) { + void detachChildRoot(RenderObjectWrapper child) { final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer assert(root is RenderObjectWithChildMixin); + assert(root.child == child.root); root.child = null; - super.removeChild(node); assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer } @@ -579,12 +609,11 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer } - void removeChild(Widget node) { + void detachChildRoot(RenderObjectWrapper child) { final root = this.root; // TODO(ianh): Remove this once the analyzer is cleverer assert(root is ContainerRenderObjectMixin); - assert(node.root.parent == root); - root.remove(node.root); - super.removeChild(node); + assert(child.root.parent == root); + root.remove(child.root); assert(root == this.root); // TODO(ianh): Remove this once the analyzer is cleverer } @@ -647,6 +676,7 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { endIndex--; oldEndIndex--; sync(endIndex); + nextSibling = children[endIndex].root; } HashMap oldNodeIdMap = null; @@ -729,7 +759,7 @@ abstract class MultiChildRenderObjectWrapper extends RenderObjectWrapper { currentNode = null; while (oldStartIndex < oldEndIndex) { oldNode = oldChildren[oldStartIndex]; - removeChild(oldNode); + syncChild(null, oldNode, null); advanceOldStartIndex(); } diff --git a/tests/widgets/syncs1-expected.txt b/tests/widgets/syncs1-expected.txt new file mode 100644 index 000000000..03a94195d --- /dev/null +++ b/tests/widgets/syncs1-expected.txt @@ -0,0 +1,108 @@ +CONSOLE: TestRenderView enabled +CONSOLE: +PAINT FOR FRAME #1 ---------------------------------------------- +1 | TestDisplayList() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ +CONSOLE: +PAINT FOR FRAME #2 ---------------------------------------------- +2 | TestDisplayList() constructor: 800.0 x 600.0 +2 | paintChild RenderBlock at Point(0.0, 0.0) +2 | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | paintChild RenderConstrainedBox at Point(0.0, 0.0) +2 | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +2 | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 425.0, 250.0), Paint(Color(0xff00ffff))) +2 | | | paintChild RenderPadding at Point(425.0, 99.0) +2 | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +2 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +2 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +2 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | | | paintChild RenderParagraph at Point(0.0, 10.0) +2 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | paintChild RenderConstrainedBox at Point(0.0, 250.0) +2 | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +2 | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xff00ffff))) +2 | | | paintChild RenderPadding at Point(0.0, 0.0) +2 | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +2 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +2 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +2 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +2 | | | | | | paintChild RenderParagraph at Point(212.5, 109.0) +2 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ +CONSOLE: +PAINT FOR FRAME #3 ---------------------------------------------- +3 | TestDisplayList() constructor: 800.0 x 600.0 +3 | paintChild RenderBlock at Point(0.0, 0.0) +3 | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | paintChild RenderConstrainedBox at Point(0.0, 0.0) +3 | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +3 | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 549.0, 250.0), Paint(Color(0xefff9f00))) +3 | | | paintChild RenderPadding at Point(549.0, 99.0) +3 | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +3 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +3 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +3 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | | | paintChild RenderParagraph at Point(0.0, 10.0) +3 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | paintChild RenderConstrainedBox at Point(0.0, 250.0) +3 | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +3 | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xefff9f00))) +3 | | | paintChild RenderPadding at Point(0.0, 0.0) +3 | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +3 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +3 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +3 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +3 | | | | | | paintChild RenderParagraph at Point(274.5, 109.0) +3 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ +CONSOLE: +PAINT FOR FRAME #4 ---------------------------------------------- +4 | TestDisplayList() constructor: 800.0 x 600.0 +4 | paintChild RenderBlock at Point(0.0, 0.0) +4 | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | paintChild RenderConstrainedBox at Point(0.0, 0.0) +4 | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +4 | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 425.0, 250.0), Paint(Color(0xff00ffff))) +4 | | | paintChild RenderPadding at Point(425.0, 99.0) +4 | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +4 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +4 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +4 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | | | paintChild RenderParagraph at Point(0.0, 10.0) +4 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | paintChild RenderConstrainedBox at Point(0.0, 250.0) +4 | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | paintChild RenderDecoratedBox at Point(0.0, 0.0) +4 | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | drawRect(Rect.fromLTRB(0.0, 0.0, 800.0, 250.0), Paint(Color(0xff00ffff))) +4 | | | paintChild RenderPadding at Point(0.0, 0.0) +4 | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | paintChild RenderConstrainedBox at Point(8.0, 8.0) +4 | | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | | drawRRect(Instance of 'RRect', Paint(Color(0xffe0e0e0))) +4 | | | | | paintChild RenderPositionedBox at Point(8.0, 0.0) +4 | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +4 | | | | | | paintChild RenderParagraph at Point(212.5, 109.0) +4 | | | | | | | TestDisplayList() constructor: 800.0 x 600.0 +------------------------------------------------------------------------ +PAINTED 4 FRAMES diff --git a/tests/widgets/syncs1.dart b/tests/widgets/syncs1.dart new file mode 100644 index 000000000..14750eb39 --- /dev/null +++ b/tests/widgets/syncs1.dart @@ -0,0 +1,100 @@ +// Copyright 2015 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; + +import 'package:sky/widgets/basic.dart'; +import 'package:sky/widgets/raised_button.dart'; + +import '../resources/display_list.dart'; + +class FiddleApp extends App { + + bool arbitrarySetting = true; + + void toggle() { + setState(() { + arbitrarySetting = !arbitrarySetting; + }); + } + + Widget buildFlex1() { + return new Flex([ + new Flexible(child: new Container( + decoration: new BoxDecoration( + backgroundColor: const Color(0xFF00FFFF) + ) + )), + new RaisedButton(child: new Text('TAP ME TO CHANGE THE BACKGROUND COLOUR'), onPressed: toggle) + ]); + } + + Widget buildFlex2() { + return new Flex([ + new Flexible(child: new Container( + key: 'something-else', + decoration: new BoxDecoration( + backgroundColor: const Color(0xEFFF9F00) + ) + )), + new RaisedButton(child: new Text('PRESS ME TO CHANGE IT BACK'), onPressed: toggle) + ]); + } + + Widget buildStack1() { + return new Stack([ + new Positioned(child: new Container( + decoration: new BoxDecoration( + backgroundColor: const Color(0xFF00FFFF) + ) + )), + new RaisedButton(child: new Text('TAP ME TO CHANGE THE BACKGROUND COLOUR'), onPressed: toggle) + ]); + } + + Widget buildStack2() { + return new Stack([ + new Positioned(child: new Container( + key: 'something-else', + decoration: new BoxDecoration( + backgroundColor: const Color(0xEFFF9F00) + ) + )), + new RaisedButton(child: new Text('PRESS ME TO CHANGE IT BACK'), onPressed: toggle) + ]); + } + + Widget build() { + return new Block([ + new SizedBox( + key: 'flex-example', + height: 250.0, + child: arbitrarySetting ? buildFlex1() : buildFlex2() + ), + new SizedBox( + key: 'stack-example', + height: 250.0, + child: arbitrarySetting ? buildStack1() : buildStack2() + ) + ]); + } +} + +main() { + TestRenderView renderViewOverride = new TestRenderView(); + FiddleApp app = new FiddleApp(); + runApp(app, renderViewOverride: renderViewOverride); + new Future.microtask(() { + renderViewOverride.checkFrame(); + app.toggle(); + new Future.microtask(() { + renderViewOverride.checkFrame(); + app.toggle(); + new Future.microtask(() { + renderViewOverride.checkFrame(); + renderViewOverride.endTest(); + }); + }); + }); +} -- GitLab