From 09871cb29b5345d49929895b2a490360eee69244 Mon Sep 17 00:00:00 2001
From: Victor <victor@suumit.com>
Date: Mon, 17 Mar 2014 12:27:23 +0100
Subject: [PATCH] feat(ngRepeat): make use if the new change detection

closes #728
relates to #645
---
 lib/directive/ng_repeat.dart       | 219 ++++++++++++++---------------
 test/directive/ng_repeat_spec.dart | 173 +++++++++++++----------
 2 files changed, 206 insertions(+), 186 deletions(-)

diff --git a/lib/directive/ng_repeat.dart b/lib/directive/ng_repeat.dart
index 6d347d1e9..89e002fb2 100644
--- a/lib/directive/ng_repeat.dart
+++ b/lib/directive/ng_repeat.dart
@@ -83,12 +83,12 @@ class NgRepeatDirective {
   String _valueIdentifier;
   String _keyIdentifier;
   String _listExpr;
-  Map<dynamic, _Row> _rows = {};
-  Function _trackByIdFn = (key, value, index) => value;
+  List<_Row> _rows;
+  Function _generateId = (key, value, index) => value;
   Watch _watch;
 
   NgRepeatDirective(this._viewPort, this._boundViewFactory, this._scope,
-                    this._parser, this._astParser, this.filters);
+                    this._parser, this.filters);
 
   set expression(value) {
     assert(value != null);
@@ -106,14 +106,14 @@ class NgRepeatDirective {
     var trackByExpr = match.group(3);
     if (trackByExpr != null) {
       Expression trackBy = _parser(trackByExpr);
-      _trackByIdFn = ((key, value, index) {
-        final trackByLocals = <String, Object>{};
-        if (_keyIdentifier != null) trackByLocals[_keyIdentifier] = key;
-        trackByLocals..[_valueIdentifier] = value
-                     ..[r'$index'] = index
-                     ..[r'$id'] = (obj) => obj;
+      _generateId = ((key, value, index) {
+        final context = <String, Object>{}
+            ..[_valueIdentifier] = value
+            ..[r'$index'] = index
+            ..[r'$id'] = (obj) => obj;
+        if (_keyIdentifier != null) context[_keyIdentifier] = key;
         return relaxFnArgs(trackBy.eval)(new ScopeLocals(_scope.context,
-            trackByLocals));
+            context));
       });
     }
 
@@ -130,116 +130,113 @@ class NgRepeatDirective {
     _keyIdentifier = match.group(2);
 
     _watch = _scope.watch(
-        _astParser(_listExpr, collection: true, filters: filters),
-        (CollectionChangeRecord changeRecord, _) {
-          //TODO(misko): we should take advantage of the CollectionChangeRecord!
-          if (changeRecord == null) return;
-          _onCollectionChange(changeRecord.iterable);
-
-        }
+        _listExpr,
+        (CollectionChangeRecord changes, _) {
+          if (changes is! CollectionChangeRecord) return;
+          _onChange(changes);
+        },
+        collection: true,
+        filters: filters
     );
   }
 
-
-  // todo -> collection
-  List<_Row> _computeNewRows(Iterable collection, trackById) {
-    final newRowOrder = new List<_Row>(collection.length);
-    // Same as lastViewMap but it has the current state. It will become the
-    // lastViewMap on the next iteration.
-    final newRows = <dynamic, _Row>{};
-    // locate existing items
-    for (var index = 0; index < newRowOrder.length; index++) {
-      var value = collection.elementAt(index);
-      trackById = _trackByIdFn(index, value, index);
-      if (_rows.containsKey(trackById)) {
-        var row = _rows.remove(trackById);
-        newRows[trackById] = row;
-        newRowOrder[index] = row;
-      } else if (newRows.containsKey(trackById)) {
-        // restore lastViewMap
-        newRowOrder.forEach((row) {
-          if (row != null && row.startNode != null) _rows[row.id] = row;
-        });
-        // This is a duplicate and we need to throw an error
-        throw "[NgErr50] ngRepeat error! Duplicates in a repeater are not "
-            "allowed. Use 'track by' expression to specify unique keys. "
-            "Repeater: $_expression, Duplicate key: $trackById";
-      } else {
-        // new never before seen row
-        newRowOrder[index] = new _Row(trackById);
-        newRows[trackById] = null;
+  // Computes and executes DOM changes when the item list changes
+  void _onChange(CollectionChangeRecord changes) {
+    final int length = changes.iterable.length;
+    final rows = new List<_Row>(length);
+    final changeFunctions = new List<Function>(length);
+    final removedIndexes = <int>[];
+    final int domLength = _rows == null ? 0 : _rows.length;
+    final leftInDom = new List.generate(domLength, (i) => domLength - 1 - i);
+    var domIndex;
+
+    var addRow = (int index, value, View previousView) {
+      var childContext = _updateContext(new PrototypeMap(_scope.context), index,
+          length)..[_valueIdentifier] = value;
+      var childScope = _scope.createChild(childContext);
+      var view = _boundViewFactory(childScope);
+      var nodes = view.nodes;
+      rows[index] = new _Row(_generateId(index, value, index))
+          ..view = view
+          ..scope = childScope
+          ..nodes = nodes
+          ..startNode = nodes.first
+          ..endNode = nodes.last;
+      _viewPort.insert(view, insertAfter: previousView);
+    };
+
+    if (_rows == null) {
+      _rows = new List<_Row>(length);
+      for (var i = 0; i < length; i++) {
+        changeFunctions[i] = (index, previousView) {
+          addRow(index, changes.iterable.elementAt(i), previousView);
+        };
       }
+    } else {
+      changes.forEachRemoval((removal) {
+        var index = removal.previousIndex;
+        var row = _rows[index];
+        row.scope.destroy();
+        _viewPort.remove(row.view);
+        leftInDom.removeAt(domLength - 1 - index);
+      });
+
+      changes.forEachAddition((addition) {
+        changeFunctions[addition.currentIndex] = (index, previousView) {
+          addRow(index, addition.item, previousView);
+        };
+      });
+
+      changes.forEachMove((move) {
+        var previousIndex = move.previousIndex;
+        var value = move.item;
+        changeFunctions[move.currentIndex] = (index, previousView) {
+          var previousRow = _rows[previousIndex];
+          var childScope = previousRow.scope;
+          var childContext = _updateContext(childScope.context, index, length);
+          if (!identical(childScope.context[_valueIdentifier], value)) {
+            childContext[_valueIdentifier] = value;
+          }
+          rows[index] = _rows[previousIndex];
+          // Only move the DOM node when required
+          if (domIndex < 0 || leftInDom[domIndex] != previousIndex) {
+            _viewPort.move(previousRow.view, moveAfter: previousView);
+            leftInDom.remove(previousIndex);
+          }
+          domIndex--;
+        };
+      });
     }
-    // remove existing items
-    _rows.forEach((key, row) {
-      _viewPort.remove(row.view);
-      row.scope.destroy();
-    });
-    _rows = newRows;
-    return newRowOrder;
-  }
 
-  void _onCollectionChange(Iterable collection) {
-    // current position of the node
-    dom.Node previousNode = _viewPort.placeholder;
-    dom.Node nextNode;
-    Scope childScope;
-    Map childContext;
-    Scope trackById;
-    View cursor;
-
-    List<_Row> newRowOrder = _computeNewRows(collection, trackById);
-
-    for (var index = 0; index < collection.length; index++) {
-      var value = collection.elementAt(index);
-      _Row row = newRowOrder[index];
-
-      if (row.startNode != null) {
-        // if we have already seen this object, then we need to reuse the
-        // associated scope/element
-        childScope = row.scope;
-        childContext = childScope.context as Map;
-
-        nextNode = previousNode;
-        do {
-          nextNode = nextNode.nextNode;
-        } while (nextNode != null);
-
-        if (row.startNode != nextNode) {
-          // existing item which got moved
-          _viewPort.move(row.view, moveAfter: cursor);
-        }
-        previousNode = row.endNode;
+    var previousView = null;
+    domIndex = leftInDom.length - 1;
+    for(var targetIndex = 0; targetIndex < length; targetIndex++) {
+      var changeFn = changeFunctions[targetIndex];
+      if (changeFn == null) {
+        rows[targetIndex] = _rows[targetIndex];
+        domIndex--;
+        // The element has not moved but `$last` and `$middle` might still need
+        // to be updated
+        _updateContext(rows[targetIndex].scope.context, targetIndex, length);
       } else {
-        // new item which we don't know about
-        childScope = _scope.createChild(childContext =
-            new PrototypeMap(_scope.context));
-      }
-
-      if (!identical(childScope.context[_valueIdentifier], value)) {
-        childContext[_valueIdentifier] = value;
-      }
-      var first = (index == 0);
-      var last = (index == collection.length - 1);
-      childContext..[r'$index'] = index
-                  ..[r'$first'] = first
-                  ..[r'$last'] = last
-                  ..[r'$middle'] = !first && !last
-                  ..[r'$odd'] = index & 1 == 1
-                  ..[r'$even'] = index & 1 == 0;
-
-      if (row.startNode == null) {
-        var view = _boundViewFactory(childScope);
-        _rows[row.id] = row
-            ..view = view
-            ..scope = childScope
-            ..nodes = view.nodes
-            ..startNode = row.nodes.first
-            ..endNode = row.nodes.last;
-        _viewPort.insert(view, insertAfter: cursor);
+        changeFn(targetIndex, previousView);
       }
-      cursor = row.view;
+      previousView = rows[targetIndex].view;
     }
+
+    _rows = rows;
+  }
+
+  PrototypeMap _updateContext(PrototypeMap context, int index, int length) {
+    var first = (index == 0);
+    var last = (index == length - 1);
+    return context
+        ..[r'$index'] = index
+        ..[r'$first'] = first
+        ..[r'$last'] = last
+        ..[r'$middle'] = !(first || last)
+        ..[r'$odd'] = index.isOdd
+        ..[r'$even'] = index.isEven;
   }
 }
 
diff --git a/test/directive/ng_repeat_spec.dart b/test/directive/ng_repeat_spec.dart
index 0e81bcebe..6e624791c 100644
--- a/test/directive/ng_repeat_spec.dart
+++ b/test/directive/ng_repeat_spec.dart
@@ -2,6 +2,14 @@ library ng_repeat_spec;
 
 import '../_specs.dart';
 
+// Mock animate instance that throws on move
+class MockAnimate extends NgAnimate {
+  Animation move(Iterable<Node> nodes, Node parent,
+                 {Node insertBefore}) {
+    throw "Move should not be called";
+  }
+}
+
 main() {
   describe('NgRepeater', () {
     Element element;
@@ -47,8 +55,8 @@ main() {
 
     it(r'should iterate over an array of objects', () {
       element = $compile(
-        '<ul>' +
-          '<li ng-repeat="item in items">{{item.name}};</li>' +
+        '<ul>'
+          '<li ng-repeat="item in items">{{item.name}};</li>'
         '</ul>');
 
       // INIT
@@ -74,10 +82,10 @@ main() {
 
     it(r'should gracefully handle nulls', () {
       element = $compile(
-        '<div>' +
-          '<ul>' +
-            '<li ng-repeat="item in null">{{item.name}};</li>' +
-          '</ul>' +
+        '<div>'
+          '<ul>'
+            '<li ng-repeat="item in null">{{item.name}};</li>'
+          '</ul>'
         '</div>');
       scope.apply();
       expect(element.querySelectorAll('ul').length).toEqual(1);
@@ -98,8 +106,8 @@ main() {
     describe('track by', () {
       it(r'should track using expression function', () {
         element = $compile(
-            '<ul>' +
-                '<li ng-repeat="item in items track by item.id">{{item.name}};</li>' +
+            '<ul>'
+                '<li ng-repeat="item in items track by item.id">{{item.name}};</li>'
             '</ul>');
         scope.context['items'] = [{"id": 'misko'}, {"id": 'igor'}];
         scope.apply();
@@ -115,8 +123,8 @@ main() {
 
       it(r'should track using build in $id function', () {
         element = $compile(
-            '<ul>' +
-                r'<li ng-repeat="item in items track by $id(item)">{{item.name}};</li>' +
+            '<ul>'
+                r'<li ng-repeat="item in items track by $id(item)">{{item.name}};</li>'
             '</ul>');
         scope.context['items'] = [{"name": 'misko'}, {"name": 'igor'}];
         scope.apply();
@@ -132,8 +140,8 @@ main() {
 
       it(r'should iterate over an array of primitives', () {
         element = $compile(
-            r'<ul>' +
-                r'<li ng-repeat="item in items track by $index">{{item}};</li>' +
+            r'<ul>'
+                r'<li ng-repeat="item in items track by $index">{{item}};</li>'
             r'</ul>');
 
         // INIT
@@ -215,15 +223,19 @@ main() {
 
     it(r'should error on wrong parsing of ngRepeat', () {
       expect(() {
-        $compile('<ul><li ng-repeat="i dont parse"></li></ul>');
-      }).toThrow("[NgErr7] ngRepeat error! Expected expression in form of '_item_ in _collection_[ track by _id_]' but got 'i dont parse'.");
+        $compile('<ul><li ng-repeat="i dont parse"></li></ul>')();
+      }).toThrow("[NgErr7] ngRepeat error! Expected expression in form of "
+                 "'_item_ in _collection_[ track by _id_]' but got "
+                 "'i dont parse'.");
     });
 
 
     it("should throw error when left-hand-side of ngRepeat can't be parsed", () {
         expect(() {
-          $compile('<ul><li ng-repeat="i dont parse in foo"></li></ul>');
-        }).toThrow("[NgErr8] ngRepeat error! '_item_' in '_item_ in _collection_' should be an identifier or '(_key_, _value_)' expression, but got 'i dont parse'.");
+          $compile('<ul><li ng-repeat="i dont parse in foo"></li></ul>')();
+        }).toThrow("[NgErr8] ngRepeat error! '_item_' in '_item_ in "
+                  "_collection_' should be an identifier or '(_key_, _value_)' "
+                  "expression, but got 'i dont parse'.");
     });
 
 
@@ -242,27 +254,30 @@ main() {
     it(r'should expose iterator position as $first, $middle and $last when iterating over arrays',
         () {
       element = $compile(
-        '<ul>' +
-          '<li ng-repeat="item in items">{{item}}:{{\$first}}-{{\$middle}}-{{\$last}}|</li>' +
+        '<ul>'
+          '<li ng-repeat="item in items">{{item}}:{{\$first}}-{{\$middle}}-{{\$last}}|</li>'
         '</ul>');
       scope.context['items'] = ['misko', 'shyam', 'doug'];
       scope.apply();
-      expect(element.text).
-          toEqual('misko:true-false-false|shyam:false-true-false|doug:false-false-true|');
+      expect(element.text)
+          .toEqual('misko:true-false-false|'
+                   'shyam:false-true-false|'
+                   'doug:false-false-true|');
 
       scope.context['items'].add('frodo');
       scope.apply();
-      expect(element.text).
-          toEqual('misko:true-false-false|' +
-                  'shyam:false-true-false|' +
-                  'doug:false-true-false|' +
-                  'frodo:false-false-true|');
+      expect(element.text)
+          .toEqual('misko:true-false-false|'
+                   'shyam:false-true-false|'
+                   'doug:false-true-false|'
+                   'frodo:false-false-true|');
 
       scope.context['items'].removeLast();
       scope.context['items'].removeLast();
       scope.apply();
-      expect(element.text).toEqual('misko:true-false-false|shyam:false-false-true|');
 
+      expect(element.text).toEqual('misko:true-false-false|'
+                                   'shyam:false-false-true|');
       scope.context['items'].removeLast();
       scope.apply();
       expect(element.text).toEqual('misko:true-false-true|');
@@ -270,16 +285,21 @@ main() {
 
     it(r'should report odd', () {
       element = $compile(
-        '<ul>' +
-          '<li ng-repeat="item in items">{{item}}:{{\$odd}}-{{\$even}}|</li>' +
+        '<ul>'
+          '<li ng-repeat="item in items">{{item}}:{{\$odd}}-{{\$even}}|</li>'
         '</ul>');
       scope.context['items'] = ['misko', 'shyam', 'doug'];
       scope.apply();
-      expect(element.text).toEqual('misko:false-true|shyam:true-false|doug:false-true|');
+      expect(element.text).toEqual('misko:false-true|'
+                                   'shyam:true-false|'
+                                   'doug:false-true|');
 
       scope.context['items'].add('frodo');
       scope.apply();
-      expect(element.text).toEqual('misko:false-true|shyam:true-false|doug:false-true|frodo:true-false|');
+      expect(element.text).toEqual('misko:false-true|'
+                                   'shyam:true-false|'
+                                   'doug:false-true|'
+                                   'frodo:true-false|');
 
       scope.context['items'].removeLast();
       scope.context['items'].removeLast();
@@ -310,8 +330,8 @@ main() {
 
       beforeEach(() {
         element = $compile(
-          '<ul>' +
-            '<li ng-repeat="item in items">{{key}}:{{val}}|></li>' +
+          '<ul>'
+            '<li ng-repeat="item in items">{{key}}:{{val}}|></li>'
           '</ul>');
         a = {};
         b = {};
@@ -330,50 +350,16 @@ main() {
         var newElements = element.querySelectorAll('li');
         expect(newElements[0]).toEqual(lis[0]);
         expect(newElements[1]).toEqual(lis[2]);
-        expect(newElements[2] == lis[1]).toEqual(false);
+        expect(newElements[2]).not.toEqual(lis[1]);
       });
 
-
-      it(r'should throw error on adding existing duplicates and recover', () {
+      it(r'should not throw an error on duplicates', () {
         scope.context['items'] = [a, a, a];
-        expect(() {
-          scope.apply();
-        }).toThrow("[NgErr50] ngRepeat error! Duplicates in a repeater are not allowed. Use 'track by' expression to specify unique keys. Repeater: item in items, Duplicate key: {}");
-
-        // recover
-        scope.context['items'] = [a];
-        scope.apply();
-        var newElements = element.querySelectorAll('li');
-        expect(newElements.length).toEqual(1);
-        expect(newElements[0]).toEqual(lis[0]);
-
-        scope.context['items'] = [];
-        scope.apply();
-        newElements = element.querySelectorAll('li');
-        expect(newElements.length).toEqual(0);
+        expect(() => scope.apply()).not.toThrow();
+        scope.context['items'].add(a);
+        expect(() => scope.apply()).not.toThrow();
       });
 
-
-      it(r'should throw error on new duplicates and recover', () {
-        scope.context['items'] = [d, d, d];
-        expect(() {
-          scope.apply();
-        }).toThrow("[NgErr50] ngRepeat error! Duplicates in a repeater are not allowed. Use 'track by' expression to specify unique keys. Repeater: item in items, Duplicate key: {}");
-
-        // recover
-        scope.context['items'] = [a];
-        scope.apply();
-        var newElements = element.querySelectorAll('li');
-        expect(newElements.length).toEqual(1);
-        expect(newElements[0]).toEqual(lis[0]);
-
-        scope.context['items'] = [];
-        scope.apply();
-        newElements = element.querySelectorAll('li');
-        expect(newElements.length).toEqual(0);
-      });
-
-
       it(r'should reverse items when the collection is reversed', () {
         scope.context['items'] = [a, b, c];
         scope.apply();
@@ -390,8 +376,8 @@ main() {
 
 
       it(r'should reuse elements even when model is composed of primitives', () {
-        // rebuilding repeater from scratch can be expensive, we should try to avoid it even for
-        // model that is composed of primitives.
+        // rebuilding repeater from scratch can be expensive, we should try to
+        // avoid it even for model that is composed of primitives.
 
         scope.context['items'] = ['hello', 'cau', 'ahoj'];
         scope.apply();
@@ -414,13 +400,50 @@ main() {
 
       var parentScope = scope.createChild(new PrototypeMap(scope.context));
       element = $compile(
-        '<ul>' +
-          '<li ng-repeat="item in items">{{item}}</li>' +
+        '<ul>'
+          '<li ng-repeat="item in items">{{item}}</li>'
         '</ul>', parentScope);
 
       parentScope.destroy();
       expect(scope.apply).not.toThrow();
     });
 
+    it(r'should not move blocks when elements only added or removed',
+    inject((Injector injector) {
+      var throwOnMove = new MockAnimate();
+      var child = injector.createChild(
+          [new Module()..value(NgAnimate, throwOnMove)]);
+
+      child.invoke((Injector injector, Scope $rootScope, Compiler compiler,
+                    DirectiveMap _directives) {
+        $exceptionHandler = injector.get(ExceptionHandler);
+        scope = $rootScope;
+        $compile = (html) {
+          element = e(html);
+          var viewFactory = compiler([element], _directives);
+          viewFactory(injector, [element]);
+          return element;
+        };
+        directives = _directives;
+      });
+
+      element = $compile(
+          '<ul>'
+            '<li ng-repeat="item in items">{{item}}</li>'
+          '</ul>');
+
+      scope..context['items'] = ['a', 'b', 'c']
+           ..apply()
+           // grow
+           ..context['items'].add('d')
+           ..apply()
+           // shrink
+           ..context['items'].removeLast()
+           ..apply()
+           ..context['items'].removeAt(0)
+           ..apply();
+
+      expect(element).toHaveText('bc');
+    }));
   });
 }