Merge pull request #1400 from abarth/reorder_update_children

updateChildren() needs to walk the list forward

packages/flutter/lib/src/rendering/object.dart

@@ -1294,59 +1294,62 @@ abstract class ContainerRenderObjectMixin<ChildType extends RenderObject, Parent
 
   ChildType _firstChild;
   ChildType _lastChild;
-  void _addToChildList(ChildType child, { ChildType before }) {
+  void _insertIntoChildList(ChildType child, { ChildType after }) {
     final ParentDataType childParentData = child.parentData;
     assert(childParentData.nextSibling == null);
     assert(childParentData.previousSibling == null);
     _childCount += 1;
     assert(_childCount > 0);
-    if (before == null) {
-      // append at the end (_lastChild)
-      childParentData.previousSibling = _lastChild;
-      if (_lastChild != null) {
-        final ParentDataType _lastChildParentData = _lastChild.parentData;
-        _lastChildParentData.nextSibling = child;
+    if (after == null) {
+      // insert at the start (_firstChild)
+      childParentData.nextSibling = _firstChild;
+      if (_firstChild != null) {
+        final ParentDataType _firstChildParentData = _firstChild.parentData;
+        _firstChildParentData.previousSibling = child;
       }
-      _lastChild = child;
-      if (_firstChild == null)
-        _firstChild = child;
+      _firstChild = child;
+      if (_lastChild == null)
+        _lastChild = child;
     } else {
       assert(_firstChild != null);
       assert(_lastChild != null);
-      assert(_debugUltimatePreviousSiblingOf(before, equals: _firstChild));
-      assert(_debugUltimateNextSiblingOf(before, equals: _lastChild));
-      final ParentDataType beforeParentData = before.parentData;
-      if (beforeParentData.previousSibling == null) {
-        // insert at the start (_firstChild); we'll end up with two or more children
-        assert(before == _firstChild);
-        childParentData.nextSibling = before;
-        beforeParentData.previousSibling = child;
-        _firstChild = child;
+      assert(_debugUltimatePreviousSiblingOf(after, equals: _firstChild));
+      assert(_debugUltimateNextSiblingOf(after, equals: _lastChild));
+      final ParentDataType afterParentData = after.parentData;
+      if (afterParentData.nextSibling == null) {
+        // insert at the end (_lastChild); we'll end up with two or more children
+        assert(after == _lastChild);
+        childParentData.previousSibling = after;
+        afterParentData.nextSibling = child;
+        _lastChild = child;
       } else {
         // insert in the middle; we'll end up with three or more children
         // set up links from child to siblings
-        childParentData.previousSibling = beforeParentData.previousSibling;
-        childParentData.nextSibling = before;
+        childParentData.nextSibling = afterParentData.nextSibling;
+        childParentData.previousSibling = after;
         // set up links from siblings to child
         final ParentDataType childPreviousSiblingParentData = childParentData.previousSibling.parentData;
         final ParentDataType childNextSiblingParentData = childParentData.nextSibling.parentData;
         childPreviousSiblingParentData.nextSibling = child;
         childNextSiblingParentData.previousSibling = child;
-        assert(beforeParentData.previousSibling == child);
+        assert(afterParentData.nextSibling == child);
       }
     }
   }
-  /// Insert child into this render object's child list before the given child.
-  ///
-  /// To insert a child at the end of the child list, omit the before parameter.
-  void add(ChildType child, { ChildType before }) {
+  /// Insert child into this render object's child list after the given child.
+  void insert(ChildType child, { ChildType after }) {
     assert(child != this);
-    assert(before != this);
-    assert(child != before);
+    assert(after != this);
+    assert(child != after);
     assert(child != _firstChild);
     assert(child != _lastChild);
     adoptChild(child);
-    _addToChildList(child, before: before);
+    _insertIntoChildList(child, after: after);
+  }
+
+  /// Append child to the end of this render object's child list.
+  void add(ChildType child) {
+    insert(child, after: _lastChild);
   }
 
   /// Add all the children to the end of this render object's child list.
@@ -1411,16 +1414,16 @@ abstract class ContainerRenderObjectMixin<ChildType extends RenderObject, Parent
   /// More efficient than removing and re-adding the child. Requires the child
   /// to already be in the child list at some position. Pass null for before to
   /// move the child to the end of the child list.
-  void move(ChildType child, { ChildType before }) {
+  void move(ChildType child, { ChildType after }) {
     assert(child != this);
-    assert(before != this);
-    assert(child != before);
+    assert(after != this);
+    assert(child != after);
     assert(child.parent == this);
     final ParentDataType childParentData = child.parentData;
-    if (childParentData.nextSibling == before)
+    if (childParentData.previousSibling == after)
       return;
     _removeFromChildList(child);
-    _addToChildList(child, before: before);
+    _insertIntoChildList(child, after: after);
   }
 
   void attach() {

packages/flutter/lib/src/widgets/framework.dart

@@ -1507,79 +1507,80 @@ abstract class RenderObjectElement<T extends RenderObjectWidget> extends Buildab
     // Widgets without keys might be synced but there is no guarantee.
 
     // The general approach is to sync the entire new list backwards, as follows:
-    // 1. Walk the lists from the top until you no longer have
-    //    matching nodes. We don't sync these yet, but we now know to
-    //    skip them below. We do this because at each sync we need to
-    //    pass the pointer to the new next widget as the slot, which
-    //    we can't do until we've synced the next child.
-    // 2. Walk the lists from the bottom, syncing nodes, until you no
-    //    longer have matching nodes.
+    // 1. Walk the lists from the top, syncing nodes, until you no longer have
+    //    matching nodes.
+    // 2. Walk the lists from the bottom, without syncing nodes, until you no
+    //    longer have matching nodes. We'll sync these nodes at the end. We
+    //    don't sync them now because we want to sync all the nodes in order
+    //    from beginning ot end.
     // At this point we narrowed the old and new lists to the point
     // where the nodes no longer match.
     // 3. Walk the narrowed part of the old list to get the list of
     //    keys and sync null with non-keyed items.
-    // 4. Walk the narrowed part of the new list backwards:
+    // 4. Walk the narrowed part of the new list forwards:
     //     * Sync unkeyed items with null
     //     * Sync keyed items with the source if it exists, else with null.
-    // 5. Walk the top list again but backwards, syncing the nodes.
+    // 5. Walk the bottom of the list again, syncing the nodes.
     // 6. Sync null with any items in the list of keys that are still
     //    mounted.
 
-    int childrenTop = 0;
+    int newChildrenTop = 0;
+    int oldChildrenTop = 0;
     int newChildrenBottom = newWidgets.length - 1;
     int oldChildrenBottom = oldChildren.length - 1;
 
-    // top of the lists
-    while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) {
-      Element oldChild = oldChildren[childrenTop];
-      Widget newWidget = newWidgets[childrenTop];
+    List<Element> newChildren = oldChildren.length == newWidgets.length ?
+        oldChildren : new List<Element>(newWidgets.length);
+
+    Element previousChild;
+
+    // Update the top of the list.
+    while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) {
+      Element oldChild = oldChildren[oldChildrenTop];
+      Widget newWidget = newWidgets[newChildrenTop];
       assert(oldChild._debugLifecycleState == _ElementLifecycle.active);
       if (!Widget.canUpdate(oldChild.widget, newWidget))
         break;
-      childrenTop += 1;
+      Element newChild = updateChild(oldChild, newWidget, previousChild);
+      assert(newChild._debugLifecycleState == _ElementLifecycle.active);
+      newChildren[newChildrenTop] = newChild;
+      previousChild = newChild;
+      newChildrenTop += 1;
+      oldChildrenTop += 1;
     }
 
-    List<Element> newChildren = oldChildren.length == newWidgets.length ?
-        oldChildren : new List<Element>(newWidgets.length);
-
-    Element nextSibling;
-
-    // bottom of the lists
-    while ((childrenTop <= oldChildrenBottom) && (childrenTop <= newChildrenBottom)) {
+    // Scan the bottom of the list.
+    while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) {
       Element oldChild = oldChildren[oldChildrenBottom];
       Widget newWidget = newWidgets[newChildrenBottom];
       assert(oldChild._debugLifecycleState == _ElementLifecycle.active);
       if (!Widget.canUpdate(oldChild.widget, newWidget))
         break;
-      Element newChild = updateChild(oldChild, newWidget, nextSibling);
-      assert(newChild._debugLifecycleState == _ElementLifecycle.active);
-      newChildren[newChildrenBottom] = newChild;
-      nextSibling = newChild;
       oldChildrenBottom -= 1;
       newChildrenBottom -= 1;
     }
 
-    // middle of the lists - old list
-    bool haveOldNodes = childrenTop <= oldChildrenBottom;
+    // Scan the old children in the middle of the list.
+    bool haveOldChildren = oldChildrenTop <= oldChildrenBottom;
     Map<Key, Element> oldKeyedChildren;
-    if (haveOldNodes) {
+    if (haveOldChildren) {
       oldKeyedChildren = new Map<Key, Element>();
-      while (childrenTop <= oldChildrenBottom) {
-        Element oldChild = oldChildren[oldChildrenBottom];
+      while (oldChildrenTop <= oldChildrenBottom) {
+        Element oldChild = oldChildren[oldChildrenTop];
         assert(oldChild._debugLifecycleState == _ElementLifecycle.active);
         if (oldChild.widget.key != null)
           oldKeyedChildren[oldChild.widget.key] = oldChild;
         else
           _deactivateChild(oldChild);
-        oldChildrenBottom -= 1;
+        oldChildrenTop += 1;
       }
     }
 
-    // middle of the lists - new list
-    while (childrenTop <= newChildrenBottom) {
+    // Update the middle of the list.
+    while (newChildrenTop <= newChildrenBottom) {
       Element oldChild;
-      Widget newWidget = newWidgets[newChildrenBottom];
-      if (haveOldNodes) {
+      Widget newWidget = newWidgets[newChildrenTop];
+      if (haveOldChildren) {
         Key key = newWidget.key;
         if (key != null) {
           oldChild = oldKeyedChildren[newWidget.key];
@@ -1596,32 +1597,38 @@ abstract class RenderObjectElement<T extends RenderObjectWidget> extends Buildab
         }
       }
       assert(oldChild == null || Widget.canUpdate(oldChild.widget, newWidget));
-      Element newChild = updateChild(oldChild, newWidget, nextSibling);
+      Element newChild = updateChild(oldChild, newWidget, previousChild);
       assert(newChild._debugLifecycleState == _ElementLifecycle.active);
       assert(oldChild == newChild || oldChild == null || oldChild._debugLifecycleState != _ElementLifecycle.active);
-      newChildren[newChildrenBottom] = newChild;
-      nextSibling = newChild;
-      newChildrenBottom -= 1;
+      newChildren[newChildrenTop] = newChild;
+      previousChild = newChild;
+      newChildrenTop += 1;
     }
-    assert(oldChildrenBottom == newChildrenBottom);
-    assert(childrenTop == newChildrenBottom + 1);
 
-    // now sync the top of the list
-    while (childrenTop > 0) {
-      childrenTop -= 1;
-      Element oldChild = oldChildren[childrenTop];
+    // We've scaned the whole list.
+    assert(oldChildrenTop == oldChildrenBottom + 1);
+    assert(newChildrenTop == newChildrenBottom + 1);
+    assert(newWidgets.length - newChildrenTop == oldChildren.length - oldChildrenTop);
+    newChildrenBottom = newWidgets.length - 1;
+    oldChildrenBottom = oldChildren.length - 1;
+
+    // Update the bottom of the list.
+    while ((oldChildrenTop <= oldChildrenBottom) && (newChildrenTop <= newChildrenBottom)) {
+      Element oldChild = oldChildren[oldChildrenTop];
       assert(oldChild._debugLifecycleState == _ElementLifecycle.active);
-      Widget newWidget = newWidgets[childrenTop];
+      Widget newWidget = newWidgets[newChildrenTop];
       assert(Widget.canUpdate(oldChild.widget, newWidget));
-      Element newChild = updateChild(oldChild, newWidget, nextSibling);
+      Element newChild = updateChild(oldChild, newWidget, previousChild);
       assert(newChild._debugLifecycleState == _ElementLifecycle.active);
       assert(oldChild == newChild || oldChild == null || oldChild._debugLifecycleState != _ElementLifecycle.active);
-      newChildren[childrenTop] = newChild;
-      nextSibling = newChild;
+      newChildren[newChildrenTop] = newChild;
+      previousChild = newChild;
+      newChildrenTop += 1;
+      oldChildrenTop += 1;
     }
 
     // clean up any of the remaining middle nodes from the old list
-    if (haveOldNodes && !oldKeyedChildren.isEmpty) {
+    if (haveOldChildren && !oldKeyedChildren.isEmpty) {
       for (Element oldChild in oldKeyedChildren.values)
         _deactivateChild(oldChild);
     }
@@ -1755,15 +1762,13 @@ class MultiChildRenderObjectElement<T extends MultiChildRenderObjectWidget> exte
 
   void insertChildRenderObject(RenderObject child, Element slot) {
     final ContainerRenderObjectMixin renderObject = this.renderObject;
-    final RenderObject nextSibling = slot?.renderObject;
-    renderObject.add(child, before: nextSibling);
+    renderObject.insert(child, after: slot?.renderObject);
     assert(renderObject == this.renderObject);
   }
 
   void moveChildRenderObject(RenderObject child, dynamic slot) {
     final ContainerRenderObjectMixin renderObject = this.renderObject;
-    final RenderObject nextSibling = slot?.renderObject;
-    renderObject.move(child, before: nextSibling);
+    renderObject.move(child, after: slot?.renderObject);
     assert(renderObject == this.renderObject);
   }
 
@@ -1797,7 +1802,7 @@ class MultiChildRenderObjectElement<T extends MultiChildRenderObjectWidget> exte
     super.mount(parent, newSlot);
     _children = new List<Element>(widget.children.length);
     Element previousChild;
-    for (int i = _children.length - 1; i >= 0; --i) {
+    for (int i = 0; i < _children.length; ++i) {
       Element newChild = _inflateWidget(widget.children[i], previousChild);
       _children[i] = newChild;
       previousChild = newChild;

packages/flutter/lib/src/widgets/homogeneous_viewport.dart

@@ -118,14 +118,12 @@ abstract class _ViewportBaseElement<T extends _ViewportBase> extends RenderObjec
   }
 
   void insertChildRenderObject(RenderObject child, Element slot) {
-    RenderObject nextSibling = slot?.renderObject;
-    renderObject.add(child, before: nextSibling);
+    renderObject.insert(child, after: slot?.renderObject);
   }
 
   void moveChildRenderObject(RenderObject child, Element slot) {
     assert(child.parent == renderObject);
-    RenderObject nextSibling = slot?.renderObject;
-    renderObject.move(child, before: nextSibling);
+    renderObject.move(child, after: slot?.renderObject);
   }
 
   void removeChildRenderObject(RenderObject child) {

packages/flutter/lib/src/widgets/mixed_viewport.dart

@@ -552,14 +552,12 @@ class _MixedViewportElement extends RenderObjectElement<MixedViewport> {
     if (haveChildren) {
       // Place all our children in our RenderObject.
       // All the children we are placing are in builtChildren and newChildren.
-      // We will walk them backwards so we can set the slots at the same time.
-      Element nextSibling = null;
-      while (index > startIndex) {
-        index -= 1;
-        final Element element = builtChildren[index];
-        if (element.slot != nextSibling)
-          updateSlotForChild(element, nextSibling);
-        nextSibling = element;
+      Element previousChild = null;
+      for (int i = startIndex; i < index; ++i) {
+        final Element element = builtChildren[i];
+        if (element.slot != previousChild)
+          updateSlotForChild(element, previousChild);
+        previousChild = element;
       }
     }
 
@@ -577,20 +575,19 @@ class _MixedViewportElement extends RenderObjectElement<MixedViewport> {
     if (slot == _omit)
       return;
     assert(slot == null || slot is Element);
-    RenderObject nextSibling = slot?.renderObject;
-    renderObject.add(child, before: nextSibling);
+    renderObject.insert(child, after: slot?.renderObject);
   }
 
   void moveChildRenderObject(RenderObject child, dynamic slot) {
     if (slot == _omit)
       return;
     assert(slot == null || slot is Element);
-    RenderObject nextSibling = slot?.renderObject;
-    assert(nextSibling == null || nextSibling.parent == renderObject);
+    RenderObject previousSibling = slot?.renderObject;
+    assert(previousSibling == null || previousSibling.parent == renderObject);
     if (child.parent == renderObject)
-      renderObject.move(child, before: nextSibling);
+      renderObject.move(child, after: previousSibling);
     else
-      renderObject.add(child, before: nextSibling);
+      renderObject.insert(child, after: previousSibling);
   }
 
   void removeChildRenderObject(RenderObject child) {

packages/flutter/lib/src/widgets/virtual_viewport.dart

@@ -149,14 +149,12 @@ abstract class VirtualViewportElement<T extends VirtualViewport> extends RenderO
   }
 
   void insertChildRenderObject(RenderObject child, Element slot) {
-    RenderObject nextSibling = slot?.renderObject;
-    renderObject.add(child, before: nextSibling);
+    renderObject.insert(child, after: slot?.renderObject);
   }
 
   void moveChildRenderObject(RenderObject child, Element slot) {
     assert(child.parent == renderObject);
-    RenderObject nextSibling = slot?.renderObject;
-    renderObject.move(child, before: nextSibling);
+    renderObject.move(child, after: slot?.renderObject);
   }
 
   void removeChildRenderObject(RenderObject child) {

packages/flutter/test/widget/focus_test.dart

@@ -38,17 +38,16 @@ void main() {
           key: keyFocus,
           child: new Column(
             children: <Widget>[
-              // reverse these when you fix https://github.com/flutter/engine/issues/1495
-              new TestFocusable(
-                key: keyB,
-                no: 'b',
-                yes: 'B FOCUSED'
-              ),
               new TestFocusable(
                 key: keyA,
                 no: 'a',
                 yes: 'A FOCUSED'
               ),
+              new TestFocusable(
+                key: keyB,
+                no: 'b',
+                yes: 'B FOCUSED'
+              ),
             ]
           )
         )