diff --git a/lib/core/parser/syntax.dart b/lib/core/parser/syntax.dart index 63777cbf2..2509a3427 100644 --- a/lib/core/parser/syntax.dart +++ b/lib/core/parser/syntax.dart @@ -28,8 +28,8 @@ abstract class Visitor { visitPrefix(Prefix expression) => visitExpression(expression); visitLiteral(Literal expression) => visitExpression(expression); - visitLiteralPrimitive(LiteralPrimitive expression) - => visitLiteral(expression); + visitLiteralPrimitive(LiteralPrimitive expression) => + visitLiteral(expression); visitLiteralString(LiteralString expression) => visitLiteral(expression); visitLiteralArray(LiteralArray expression) => visitLiteral(expression); visitLiteralObject(LiteralObject expression) => visitLiteral(expression); @@ -39,12 +39,12 @@ abstract class Expression { bool get isAssignable => false; bool get isChain => false; - eval(scope, [FilterMap filters = defaultFilterMap]) - => throw new EvalError("Cannot evaluate $this"); - assign(scope, value) - => throw new EvalError("Cannot assign to $this"); - bind(context, [LocalsWrapper wrapper]) - => new BoundExpression(this, context, wrapper); + eval(scope, [FilterMap filters = defaultFilterMap]) => + throw new EvalError("Cannot evaluate $this"); + assign(scope, value) => + throw new EvalError("Cannot assign to $this"); + bind(context, [LocalsWrapper wrapper]) => + new BoundExpression(this, context, wrapper); accept(Visitor visitor); String toString() => Unparser.unparse(this); diff --git a/lib/core_dom/view.dart b/lib/core_dom/view.dart index 021e9f2e9..24b0731ff 100644 --- a/lib/core_dom/view.dart +++ b/lib/core_dom/view.dart @@ -29,7 +29,7 @@ class View { class ViewPort { final dom.Node placeholder; final NgAnimate _animate; - final List _views = []; + final _views = []; ViewPort(this.placeholder, this._animate); @@ -56,12 +56,12 @@ class ViewPort { } void _viewsInsertAfter(View view, View insertAfter) { - int index = (insertAfter != null) ? _views.indexOf(insertAfter) : -1; - _views.insert(index + 1, view); + int index = insertAfter == null ? 0 : _views.indexOf(insertAfter) + 1; + _views.insert(index, view); } dom.Node _lastNode(View insertAfter) => insertAfter == null ? placeholder - : insertAfter.nodes[insertAfter.nodes.length - 1]; + : insertAfter.nodes.last; } diff --git a/lib/directive/ng_repeat.dart b/lib/directive/ng_repeat.dart index 087900be9..86601ed42 100644 --- a/lib/directive/ng_repeat.dart +++ b/lib/directive/ng_repeat.dart @@ -1,16 +1,5 @@ part of angular.directive; -class _Row { - var id; - Scope scope; - View view; - dom.Element startNode; - dom.Element endNode; - List nodes; - - _Row(this.id); -} - /** * The `ngRepeat` directive instantiates a template once per item from a * collection. Each template instance gets its own scope, where the given loop @@ -20,15 +9,15 @@ class _Row { * Special properties are exposed on the local scope of each template instance, * including: * - * - * - * - * - * - * - * - * - *
Variable Type Details
`$index` [num] iterator offset of the repeated element (0..length-1)
`$first` [bool] true if the repeated element is first in the iterator.
`$middle` [bool] true if the repeated element is between the first and last in the iterator.
`$last` [bool] true if the repeated element is last in the iterator.
`$even` [bool] true if the iterator position `$index` is even (otherwise false).
`$odd` [bool] true if the iterator position `$index` is odd (otherwise false).
+ * * `$index` ([:num:]) the iterator offset of the repeated element + * (0..length-1) + * * `$first` ([:bool:]) whether the repeated element is first in the + * iterator. + * * `$middle` ([:bool:]) whether the repeated element is between the first + * and last in the iterator. + * * `$last` ([:bool:]) whether the repeated element is last in the iterator. + * * `$even` ([:bool:]) whether the iterator position `$index` is even. + * * `$odd` ([:bool:]) whether the iterator position `$index` is odd. * * * [repeat_expression] ngRepeat The expression indicating how to enumerate a @@ -57,7 +46,7 @@ class _Row { * function can be used to assign a unique `$$hashKey` property to each item * in the array. This property is then used as a key to associated DOM * elements with the corresponding item in the array by identity. Moving the - * same object in array would move the DOM element in the same way ian the + * same object in array would move the DOM element in the same way in the * DOM. * * For example: `item in items track by item.id` is a typical pattern when @@ -81,8 +70,8 @@ class _Row { selector: '[ng-repeat]', map: const {'.': '@expression'}) class NgRepeatDirective { - static RegExp _SYNTAX = new RegExp(r'^\s*(.+)\s+in\s+(.*?)\s*(\s+track\s+by\s+(.+)\s*)?(\s+lazily\s*)?$'); - static RegExp _LHS_SYNTAX = new RegExp(r'^(?:([\$\w]+)|\(([\$\w]+)\s*,\s*([\$\w]+)\))$'); + static RegExp _SYNTAX = new RegExp(r'^\s*(.+)\s+in\s+(.*?)\s*(?:track\s+by\s+(.+)\s*)?(\s+lazily\s*)?$'); + static RegExp _LHS_SYNTAX = new RegExp(r'^(?:([$\w]+)|\(([$\w]+)\s*,\s*([$\w]+)\))$'); final ViewPort _viewPort; final BoundViewFactory _boundViewFactory; @@ -95,38 +84,40 @@ class NgRepeatDirective { String _valueIdentifier; String _keyIdentifier; String _listExpr; - Map _rows = {}; - Function _trackByIdFn = (key, value, index) => value; - Watch _watch = null; - Iterable _lastCollection; + List<_Row> _rows; + Function _generateId = (key, value, index) => value; + Watch _watch; - NgRepeatDirective(this._viewPort, this._boundViewFactory, - this._scope, this._parser, this._astParser, - this.filters); + NgRepeatDirective(this._viewPort, this._boundViewFactory, this._scope, + this._parser, this._astParser, this.filters); set expression(value) { assert(value != null); _expression = value; if (_watch != null) _watch.remove(); + Match match = _SYNTAX.firstMatch(_expression); if (match == null) { throw "[NgErr7] ngRepeat error! Expected expression in form of '_item_ " "in _collection_[ track by _id_]' but got '$_expression'."; } + _listExpr = match.group(2); - var trackByExpr = match.group(4); + + var trackByExpr = match.group(3); if (trackByExpr != null) { Expression trackBy = _parser(trackByExpr); - _trackByIdFn = ((key, value, index) { - final trackByLocals = {}; - if (_keyIdentifier != null) trackByLocals[_keyIdentifier] = key; - trackByLocals + _generateId = ((key, value, index) { + final context = {} ..[_valueIdentifier] = value ..[r'$index'] = index ..[r'$id'] = (obj) => obj; - return relaxFnArgs(trackBy.eval)(new ScopeLocals(_scope.context, trackByLocals)); + if (_keyIdentifier != null) context[_keyIdentifier] = key; + return relaxFnArgs(trackBy.eval)(new ScopeLocals(_scope.context, + context)); }); } + var assignExpr = match.group(1); match = _LHS_SYNTAX.firstMatch(assignExpr); if (match == null) { @@ -134,117 +125,127 @@ class NgRepeatDirective { "should be an identifier or '(_key_, _value_)' expression, but got " "'$assignExpr'."; } + _valueIdentifier = match.group(3); if (_valueIdentifier == null) _valueIdentifier = match.group(1); _keyIdentifier = match.group(2); _watch = _scope.watch( _astParser(_listExpr, collection: true, filters: filters), - (CollectionChangeRecord collection, _) { - //TODO(misko): we should take advantage of the CollectionChangeRecord! - _onCollectionChange(collection == null ? [] : collection.iterable); + (CollectionChangeRecord changes, _) { + if (changes is! CollectionChangeRecord) return; + _onChange(changes); } ); } - 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 = {}; - // 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[trackById]; - _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(length); + final removedIndexes = []; + 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; - } - _onCollectionChange(Iterable collection) { - dom.Node previousNode = _viewPort.placeholder; // current position of the - // node - 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; + changeFn(targetIndex, previousView); } - 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[0] - ..endNode = row.nodes[row.nodes.length - 1]; - _viewPort.insert(view, insertAfter: cursor); - } - 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; + } +} + +class _Row { + final id; + Scope scope; + View view; + dom.Element startNode; + dom.Element endNode; + List nodes; + + _Row(this.id); } diff --git a/test/change_detection/dirty_checking_change_detector_spec.dart b/test/change_detection/dirty_checking_change_detector_spec.dart index 3dcb6335a..77415710f 100644 --- a/test/change_detection/dirty_checking_change_detector_spec.dart +++ b/test/change_detection/dirty_checking_change_detector_spec.dart @@ -27,10 +27,9 @@ void main() { var user = new _User('', ''); Iterator changeIterator; - detector - ..watch(user, 'first', null) - ..watch(user, 'last', null) - ..collectChanges(); // throw away first set + detector..watch(user, 'first', null) + ..watch(user, 'last', null) + ..collectChanges(); // throw away first set changeIterator = detector.collectChanges(); expect(changeIterator.moveNext()).toEqual(false); 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 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( - '
    ' + - '
  • {{item.name}};
    • ' + + '
      ' + '
    • {{item.name}};
      • ' '
    '); // INIT @@ -74,10 +82,10 @@ main() { it(r'should gracefully handle nulls', () { element = $compile( - '
    ' + - '
      ' + - '
    • {{item.name}};
      • ' + - '
    ' + + '
    ' + '
      ' + '
    • {{item.name}};
      • ' + '
    ' '
    '); 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( - '
      ' + - '
    • {{item.name}};
      • ' + + '
        ' + '
      • {{item.name}};
        • ' '
      '); 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( - '
        ' + - r'
      • {{item.name}};
        • ' + + '
          ' + r'
        • {{item.name}};
          • ' '
        '); 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'
          ' + - r'
        • {{item}};
          • ' + + r'
            ' + r'
          • {{item}};
            • ' r'
          '); // INIT @@ -215,15 +223,19 @@ main() { it(r'should error on wrong parsing of ngRepeat', () { expect(() { - $compile('
          '); - }).toThrow("[NgErr7] ngRepeat error! Expected expression in form of '_item_ in _collection_[ track by _id_]' but got 'i dont parse'."); + $compile('
          ')(); + }).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('
          '); - }).toThrow("[NgErr8] ngRepeat error! '_item_' in '_item_ in _collection_' should be an identifier or '(_key_, _value_)' expression, but got 'i dont parse'."); + $compile('
          ')(); + }).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( - '
            ' + - '
          • {{item}}:{{\$first}}-{{\$middle}}-{{\$last}}|
            • ' + + '
              ' + '
            • {{item}}:{{\$first}}-{{\$middle}}-{{\$last}}|
              • ' '
            '); 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( - '
              ' + - '
            • {{item}}:{{\$odd}}-{{\$even}}|
              • ' + + '
                ' + '
              • {{item}}:{{\$odd}}-{{\$even}}|
                • ' '
              '); 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( - '
                ' + - '
              • {{key}}:{{val}}|>
                • ' + + '
                  ' + '
                • {{key}}:{{val}}|>
                  • ' '
                '); 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( - '
                  ' + - '
                • {{item}}
                  • ' + + '
                    ' + '
                  • {{item}}
                    • ' '
                  ', 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( + '
                    ' + '
                  • {{item}}
                    • ' + '
                  '); + + 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'); + })); }); }