From 28a79bc21ef3a4a7b0824249bfa31a668275c76e Mon Sep 17 00:00:00 2001 From: Chirayu Krishnappa Date: Thu, 27 Mar 2014 13:50:15 -0700 Subject: [PATCH] fix(dirty-checking): handle simultaneous mutations and ref changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This applies to both maps and lists/iterables. Consider the case when one is watching an expression, say, `items`, that evaluates a list.  Now consider the case when one both mutates that list and also updates `items` to point to a different list.  In this case, the reported changes should be between the earlier observed collection and the newly observed collection.  The previous implementation is buggy.  It first observes that the original list was mutated and then observes that the reference to the list was updated to a different list.  The changes it reports are the differences between the mutated list and the new list which is incorrect. This commit fixes the bug by noticing the case when both a mutation of the previously tracked sequence and a ref change occurs and resets the change record to undo the recording of the mutation. --- lib/change_detection/change_detection.dart | 10 + .../dirty_checking_change_detector.dart | 146 ++++++- .../dirty_checking_change_detector_spec.dart | 380 +++++++++--------- test/change_detection/watch_group_spec.dart | 99 +++++ 4 files changed, 444 insertions(+), 191 deletions(-) diff --git a/lib/change_detection/change_detection.dart b/lib/change_detection/change_detection.dart index b29f1e173..fff3c43e7 100644 --- a/lib/change_detection/change_detection.dart +++ b/lib/change_detection/change_detection.dart @@ -114,6 +114,7 @@ abstract class MapChangeRecord { /// A list of [CollectionKeyValue]s which are in the iteration order. */ KeyValue get mapHead; + PreviousKeyValue get previousMapHead; /// A list of changed items. ChangedKeyValue get changesHead; /// A list of new added items. @@ -145,6 +146,10 @@ abstract class KeyValue extends MapKeyValue { KeyValue get nextKeyValue; } +abstract class PreviousKeyValue extends MapKeyValue { + PreviousKeyValue get previousNextKeyValue; +} + abstract class AddedKeyValue extends MapKeyValue { AddedKeyValue get nextAddedKeyValue; } @@ -172,6 +177,7 @@ abstract class CollectionChangeRecord { /** A list of [CollectionItem]s which are in the iteration order. */ CollectionItem get collectionHead; + PreviousCollectionItem get previousCollectionHead; /** A list of new [AddedItem]s. */ AddedItem get additionsHead; /** A list of [MovedItem]s. */ @@ -211,6 +217,10 @@ abstract class CollectionItem extends CollectionChangeItem { * A linked list of new items added to the collection. These items are always in * the iteration order of the collection. */ +abstract class PreviousCollectionItem extends CollectionChangeItem { + PreviousCollectionItem get previousNextItem; +} + abstract class AddedItem extends CollectionChangeItem { AddedItem get nextAddedItem; } diff --git a/lib/change_detection/dirty_checking_change_detector.dart b/lib/change_detection/dirty_checking_change_detector.dart index de5855e67..234dfcfea 100644 --- a/lib/change_detection/dirty_checking_change_detector.dart +++ b/lib/change_detection/dirty_checking_change_detector.dart @@ -426,16 +426,29 @@ class DirtyCheckingRecord implements Record, WatchRecord { _getter = null; if (obj is Map) { if (_mode != _MODE_MAP_) { - // Last one was collection as well, don't reset state. _mode = _MODE_MAP_; currentValue = new _MapChangeRecord(); } + if (currentValue.isDirty) { + // We're dirty because the mapping we tracked by reference mutated. + // In addition, our reference has now changed. We should compare the + // previous reported value of that mapping with the one from the + // new reference. + currentValue._revertToPreviousState(); + } + } else if (obj is Iterable) { if (_mode != _MODE_ITERABLE_) { - // Last one was collection as well, don't reset state. - _mode = _MODE_ITERABLE_; + _mode = _MODE_ITERABLE_; currentValue = new _CollectionChangeRecord(); } + if (currentValue.isDirty) { + // We're dirty because the collection we tracked by reference mutated. + // In addition, our reference has now changed. We should compare the + // previous reported value of that collection with the one from the + // new reference. + currentValue._revertToPreviousState(); + } } else { _mode = _MODE_IDENTITY_; } @@ -507,12 +520,14 @@ class _MapChangeRecord implements MapChangeRecord { final Map _records = new Map(); Map _map; KeyValueRecord _mapHead; + KeyValueRecord _previousMapHead; KeyValueRecord _changesHead, _changesTail; KeyValueRecord _additionsHead, _additionsTail; KeyValueRecord _removalsHead, _removalsTail; Map get map => _map; KeyValue get mapHead => _mapHead; + PreviousKeyValue get previousMapHead => _previousMapHead; ChangedKeyValue get changesHead => _changesHead; AddedKeyValue get additionsHead => _additionsHead; RemovedKeyValue get removalsHead => _removalsHead; @@ -521,6 +536,24 @@ class _MapChangeRecord implements MapChangeRecord { _changesHead != null || _removalsHead != null; + _revertToPreviousState() { + if (!isDirty) { + return; + } + KeyValueRecord record, prev; + int i = 0; + for (record = _mapHead = _previousMapHead; + record != null; + prev = record, record = record._previousNextKeyValue, ++i) { + record._currentValue = record._previousValue; + if (prev != null) { + prev._nextKeyValue = prev._previousNextKeyValue = record; + } + } + prev._nextKeyValue = null; + _undoDeltas(); + } + void forEachChange(void f(ChangedKeyValue change)) { KeyValueRecord record = _changesHead; while (record != null) { @@ -602,6 +635,18 @@ class _MapChangeRecord implements MapChangeRecord { } void _reset() { + if (isDirty) { + // Record the state of the mapping for a possible _revertToPreviousState() + for (KeyValueRecord record = _previousMapHead = _mapHead; + record != null; + record = record._nextKeyValue) { + record._previousNextKeyValue = record._nextKeyValue; + } + _undoDeltas(); + } + } + + void _undoDeltas() { var record = _changesHead; while (record != null) { record._previousValue = record._currentValue; @@ -748,14 +793,42 @@ class _MapChangeRecord implements MapChangeRecord { _changesTail = record; } } + + String toString() { + List itemsList = [], previousList = [], changesList = [], additionsList = [], removalsList = []; + KeyValueRecord record; + for (record = _mapHead; record != null; record = record._nextKeyValue) { + itemsList.add("$record"); + } + for (record = _previousMapHead; record != null; record = record._previousNextKeyValue) { + previousList.add("$record"); + } + for (record = _changesHead; record != null; record = record._nextChangedKeyValue) { + changesList.add("$record"); + } + for (record = _additionsHead; record != null; record = record._nextAddedKeyValue) { + additionsList.add("$record"); + } + for (record = _removalsHead; record != null; record = record._nextRemovedKeyValue) { + removalsList.add("$record"); + } + return """ +map: ${itemsList.join(", ")} +previous: ${previousList.join(", ")} +changes: ${changesList.join(", ")} +additions: ${additionsList.join(", ")} +removals: ${removalsList.join(", ")} +"""; + } } -class KeyValueRecord implements KeyValue, AddedKeyValue, - RemovedKeyValue, ChangedKeyValue { +class KeyValueRecord implements KeyValue, PreviousKeyValue, + AddedKeyValue, RemovedKeyValue, ChangedKeyValue { final K key; V _previousValue, _currentValue; KeyValueRecord _nextKeyValue; + KeyValueRecord _previousNextKeyValue; KeyValueRecord _nextAddedKeyValue; KeyValueRecord _nextRemovedKeyValue, _prevRemovedKeyValue; KeyValueRecord _nextChangedKeyValue; @@ -765,12 +838,13 @@ class KeyValueRecord implements KeyValue, AddedKeyValue, V get previousValue => _previousValue; V get currentValue => _currentValue; KeyValue get nextKeyValue => _nextKeyValue; + PreviousKeyValue get previousNextKeyValue => _previousNextKeyValue; AddedKeyValue get nextAddedKeyValue => _nextAddedKeyValue; RemovedKeyValue get nextRemovedKeyValue => _nextRemovedKeyValue; ChangedKeyValue get nextChangedKeyValue => _nextChangedKeyValue; String toString() => _previousValue == _currentValue - ? key + ? "$key" : '$key[$_previousValue -> $_currentValue]'; } @@ -785,16 +859,40 @@ class _CollectionChangeRecord implements CollectionChangeRecord { /** Used to keep track of removed items. */ DuplicateMap _removedItems = new DuplicateMap(); + ItemRecord _previousCollectionHead; ItemRecord _collectionHead, _collectionTail; ItemRecord _additionsHead, _additionsTail; ItemRecord _movesHead, _movesTail; ItemRecord _removalsHead, _removalsTail; + CollectionChangeItem get previousCollectionHead => _previousCollectionHead; CollectionChangeItem get collectionHead => _collectionHead; CollectionChangeItem get additionsHead => _additionsHead; CollectionChangeItem get movesHead => _movesHead; CollectionChangeItem get removalsHead => _removalsHead; + _revertToPreviousState() { + if (!isDirty) { + return; + } + _items.clear(); + ItemRecord record, prev; + int i = 0; + for (record = _collectionHead = _previousCollectionHead; + record != null; + prev = record, record = record._previousNextRec, ++i) { + record.currentIndex = record.previousIndex = i; + record._prevRec = prev; + if (prev != null) { + prev._nextRec = prev._previousNextRec = record; + } + _items.put(record); + } + prev._nextRec = null; + _collectionTail = prev; + _undoDeltas(); + } + void forEachAddition(void f(AddedItem addition)){ ItemRecord record = _additionsHead; while (record != null) { @@ -824,14 +922,15 @@ class _CollectionChangeRecord implements CollectionChangeRecord { bool _check(Iterable collection) { _reset(); - ItemRecord record = _collectionHead; - bool maybeDirty = false; if ((collection is UnmodifiableListView) && - identical(_iterable, collection)) { + identical(_iterable, collection)) { // Short circuit and assume that the list has not been modified. return false; } + ItemRecord record = _collectionHead; + bool maybeDirty = false; + if (collection is List) { List list = collection; _length = list.length; @@ -873,6 +972,18 @@ class _CollectionChangeRecord implements CollectionChangeRecord { * removals). */ void _reset() { + if (isDirty) { + // Record the state of the collection for a possible _revertToPreviousState() + for (ItemRecord record = _previousCollectionHead = _collectionHead; + record != null; + record = record._nextRec) { + record._previousNextRec = record._nextRec; + } + _undoDeltas(); + } + } + + void _undoDeltas() { ItemRecord record; record = _additionsHead; @@ -1157,6 +1268,13 @@ class _CollectionChangeRecord implements CollectionChangeRecord { record = record._nextRec; } + var previous = []; + record = _previousCollectionHead; + while (record != null) { + previous.add(record); + record = record._previousNextRec; + } + var additions = []; record = _additionsHead; while (record != null) { @@ -1180,6 +1298,7 @@ class _CollectionChangeRecord implements CollectionChangeRecord { return """ collection: ${list.join(", ")} +previous: ${previous.join(", ")} additions: ${additions.join(", ")} moves: ${moves.join(", ")} removals: ${removals.join(", ")} @@ -1187,17 +1306,20 @@ removals: ${removals.join(", ")} } } -class ItemRecord implements CollectionItem, AddedItem, MovedItem, +class ItemRecord implements PreviousCollectionItem, CollectionItem, AddedItem, MovedItem, RemovedItem { int previousIndex = null; int currentIndex = null; V item = _INITIAL_; + + ItemRecord _previousNextRec; ItemRecord _prevRec, _nextRec; ItemRecord _prevDupRec, _nextDupRec; ItemRecord _prevRemovedRec, _nextRemovedRec; ItemRecord _nextAddedRec, _nextMovedRec; + PreviousCollectionItem get previousNextItem => _previousNextRec; CollectionItem get nextCollectionItem => _nextRec; RemovedItem get nextRemovedItem => _nextRemovedRec; AddedItem get nextAddedItem => _nextAddedRec; @@ -1314,7 +1436,11 @@ class DuplicateMap { return record; } + bool get isEmpty => map.isEmpty; + void clear() { map.clear(); } + + String toString() => "$runtimeType($map)"; } diff --git a/test/change_detection/dirty_checking_change_detector_spec.dart b/test/change_detection/dirty_checking_change_detector_spec.dart index cda777dad..956e0583c 100644 --- a/test/change_detection/dirty_checking_change_detector_spec.dart +++ b/test/change_detection/dirty_checking_change_detector_spec.dart @@ -249,6 +249,53 @@ void main() { }); describe('list watching', () { + describe('previous state', () { + it('should store on addition', () { + var list = []; + var record = detector.watch(list, null, null); + expect(detector.collectChanges().moveNext()).toEqual(false); + var iterator; + + list.add('a'); + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( + collection: ['a[null -> 0]'], + previous: [], + additions: ['a[null -> 0]'], + moves: [], + removals: [])); + + list.add('b'); + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( + collection: ['a', 'b[null -> 1]'], + previous: ['a'], + additions: ['b[null -> 1]'], + moves: [], + removals: [])); + }); + + it('should handle swapping elements correctly', () { + var list = [1, 2]; + var record = detector.watch(list, null, null); + detector.collectChanges().moveNext(); + var iterator; + + // reverse the list. + list.setAll(0, list.reversed.toList()); + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( + collection: ['2[1 -> 0]', '1[0 -> 1]'], + previous: ['1[0 -> 1]', '2[1 -> 0]'], + additions: [], + moves: ['2[1 -> 0]', '1[0 -> 1]'], + removals: [])); + }); + }); + it('should detect changes in list', () { var list = []; var record = detector.watch(list, null, 'handler'); @@ -269,6 +316,7 @@ void main() { expect(iterator.moveNext()).toEqual(true); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b[null -> 1]'], + previous: ['a'], additions: ['b[null -> 1]'], moves: [], removals: [])); @@ -279,6 +327,7 @@ void main() { expect(iterator.moveNext()).toEqual(true); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'c[null -> 2]', 'd[null -> 3]'], + previous: ['a', 'b'], additions: ['c[null -> 2]', 'd[null -> 3]'], moves: [], removals: [])); @@ -289,6 +338,7 @@ void main() { expect(iterator.moveNext()).toEqual(true); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'd[3 -> 2]'], + previous: ['a', 'b', 'c[2 -> null]', 'd[3 -> 2]'], additions: [], moves: ['d[3 -> 2]'], removals: ['c[2 -> null]'])); @@ -299,6 +349,7 @@ void main() { expect(iterator.moveNext()).toEqual(true); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['d[2 -> 0]', 'c[null -> 1]', 'b[1 -> 2]', 'a[0 -> 3]'], + previous: ['a[0 -> 3]', 'b[1 -> 2]', 'd[2 -> 0]'], additions: ['c[null -> 1]'], moves: ['d[2 -> 0]', 'b[1 -> 2]', 'a[0 -> 3]'], removals: [])); @@ -330,6 +381,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'c[2 -> 1]'], + previous: ['a', 'b[1 -> null]', 'c[2 -> 1]'], additions: [], moves: ['c[2 -> 1]'], removals: ['b[1 -> null]'])); @@ -339,6 +391,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b[null -> 1]', 'c[1 -> 2]'], + previous: ['a', 'c[1 -> 2]'], additions: ['b[null -> 1]'], moves: ['c[1 -> 2]'], removals: [])); @@ -353,6 +406,7 @@ void main() { var iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'a', 'b[3 -> 2]', 'b[4 -> 3]'], + previous: ['a', 'a', 'a[2 -> null]', 'b[3 -> 2]', 'b[4 -> 3]'], additions: [], moves: ['b[3 -> 2]', 'b[4 -> 3]'], removals: ['a[2 -> null]'])); @@ -369,6 +423,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['b[2 -> 0]', 'a[0 -> 1]', 'a[1 -> 2]', 'b', 'b[null -> 4]'], + previous: ['a[0 -> 1]', 'a[1 -> 2]', 'b[2 -> 0]', 'b'], additions: ['b[null -> 4]'], moves: ['b[2 -> 0]', 'a[0 -> 1]', 'a[1 -> 2]'], removals: [])); @@ -411,6 +466,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['2[1 -> 0]', '3[2 -> 1]', '4[3 -> 2]'], + previous: ['1[0 -> null]', '2[1 -> 0]', '3[2 -> 1]', '4[3 -> 2]'], additions: [], moves: ['2[1 -> 0]', '3[2 -> 1]', '4[3 -> 2]'], removals: ['1[0 -> null]'])); @@ -419,6 +475,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['1[null -> 0]', '2[0 -> 1]', '3[1 -> 2]', '4[2 -> 3]'], + previous: ['2[0 -> 1]', '3[1 -> 2]', '4[2 -> 3]'], additions: ['1[null -> 0]'], moves: ['2[0 -> 1]', '3[1 -> 2]', '4[2 -> 3]'], removals: [])); @@ -442,6 +499,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['(1)a-a[1 -> 0]'], + previous: ['(0)a-a[0 -> null]', '(1)a-a[1 -> 0]'], additions: [], moves: ['(1)a-a[1 -> 0]'], removals: ['(0)a-a[0 -> null]'])); @@ -450,6 +508,7 @@ void main() { iterator = detector.collectChanges()..moveNext(); expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['(2)a-a[null -> 0]', '(1)a-a[0 -> 1]'], + previous: ['(1)a-a[0 -> 1]'], additions: ['(2)a-a[null -> 0]'], moves: ['(1)a-a[0 -> 1]'], removals: [])); @@ -457,6 +516,53 @@ void main() { }); describe('map watching', () { + describe('previous state', () { + it('should store on insertion', () { + var map = {}; + var record = detector.watch(map, null, null); + expect(detector.collectChanges().moveNext()).toEqual(false); + var iterator; + + map['a'] = 1; + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualMapRecord( + map: ['a[null -> 1]'], + previous: [], + additions: ['a[null -> 1]'], + changes: [], + removals: [])); + + map['b'] = 2; + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualMapRecord( + map: ['a', 'b[null -> 2]'], + previous: ['a'], + additions: ['b[null -> 2]'], + changes: [], + removals: [])); + }); + + it('should handle changing key/values correctly', () { + var map = {1: 10, 2: 20}; + var record = detector.watch(map, null, null); + detector.collectChanges().moveNext(); + var iterator; + + map[1] = 20; + map[2] = 10; + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualMapRecord( + map: ['1[10 -> 20]', '2[20 -> 10]'], + previous: ['1[10 -> 20]', '2[20 -> 10]'], + additions: [], + changes: ['1[10 -> 20]', '2[20 -> 10]'], + removals: [])); + }); + }); + it('should do basic map watching', () { var map = {}; var record = detector.watch(map, null, 'handler'); @@ -468,6 +574,7 @@ void main() { expect(changeIterator.moveNext()).toEqual(true); expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a[null -> A]'], + previous: [], additions: ['a[null -> A]'], changes: [], removals: [])); @@ -477,6 +584,7 @@ void main() { expect(changeIterator.moveNext()).toEqual(true); expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'b[null -> B]'], + previous: ['a'], additions: ['b[null -> B]'], changes: [], removals: [])); @@ -487,6 +595,7 @@ void main() { expect(changeIterator.moveNext()).toEqual(true); expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'b[B -> BB]', 'd[null -> D]'], + previous: ['a', 'b[B -> BB]'], additions: ['d[null -> D]'], changes: ['b[B -> BB]'], removals: [])); @@ -497,6 +606,7 @@ void main() { expect(changeIterator.moveNext()).toEqual(true); expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'd'], + previous: ['a', 'b[BB -> null]', 'd'], additions: [], changes: [], removals: ['b[BB -> null]'])); @@ -506,6 +616,7 @@ void main() { expect(changeIterator.moveNext()).toEqual(true); expect(changeIterator.current.currentValue, toEqualMapRecord( map: [], + previous: ['a[A -> null]', 'd[D -> null]'], additions: [], changes: [], removals: ['a[A -> null]', 'd[D -> null]'])); @@ -577,12 +688,12 @@ class _User { _User([this.first, this.last, this.age]); } -Matcher toEqualCollectionRecord({collection, additions, moves, removals}) => - new CollectionRecordMatcher(collection:collection, additions:additions, - moves:moves, removals:removals); -Matcher toEqualMapRecord({map, additions, changes, removals}) => - new MapRecordMatcher(map:map, additions:additions, - changes:changes, removals:removals); +Matcher toEqualCollectionRecord({collection, previous, additions, moves, removals}) => + new CollectionRecordMatcher(collection:collection, previous: previous, + additions:additions, moves:moves, removals:removals); +Matcher toEqualMapRecord({map, previous, additions, changes, removals}) => + new MapRecordMatcher(map:map, previous: previous, + additions:additions, changes:changes, removals:removals); Matcher toEqualChanges(List changes) => new ChangeMatcher(changes); class ChangeMatcher extends Matcher { @@ -612,14 +723,53 @@ class ChangeMatcher extends Matcher { } } -class CollectionRecordMatcher extends Matcher { +abstract class _CollectionMatcher extends Matcher { + List _getList(T item, T next(T)) { + var result = []; + for (; item != null; item = next(item)) { + result.add(item); + } + return result; + } + + bool _compareLists(String tag, List expected, List actual, List diffs) { + var equals = true; + Iterator iActual = actual.iterator; + iActual.moveNext(); + T actualItem = iActual.current; + if (expected == null) { + expected = []; + } + for (String expectedItem in expected) { + if (actualItem == null) { + equals = false; + diffs.add('$tag too short: $expectedItem'); + } else { + if ("$actualItem" != expectedItem) { + equals = false; + diffs.add('$tag mismatch: $actualItem != $expectedItem'); + } + iActual.moveNext(); + actualItem = iActual.current; + } + } + if (actualItem != null) { + diffs.add('$tag too long: $actualItem'); + equals = false; + } + return equals; + } +} + +class CollectionRecordMatcher extends _CollectionMatcher { final List collection; + final List previous; final List additions; final List moves; final List removals; - CollectionRecordMatcher({this.collection, this.additions, this.moves, - this.removals}); + CollectionRecordMatcher({this.collection, this.previous, + this.additions, this.moves, this.removals}); Description describeMismatch(changes, Description mismatchDescription, Map matchState, bool verbose) { @@ -636,6 +786,7 @@ class CollectionRecordMatcher extends Matcher { } add('collection', collection); + add('previous', previous); add('additions', additions); add('moves', moves); add('removals', removals); @@ -645,122 +796,52 @@ class CollectionRecordMatcher extends Matcher { bool matches(CollectionChangeRecord changeRecord, Map matchState) { var diffs = matchState['diffs'] = []; return checkCollection(changeRecord, diffs) && + checkPrevious(changeRecord, diffs) && checkAdditions(changeRecord, diffs) && checkMoves(changeRecord, diffs) && checkRemovals(changeRecord, diffs); } bool checkCollection(CollectionChangeRecord changeRecord, List diffs) { - var equals = true; - int count = 0; - if (collection != null) { - CollectionItem collectionItem = changeRecord.collectionHead; - for (var item in collection) { - count++; - if (collectionItem == null) { - equals = false; - diffs.add('collection too short: $item'); - } else { - if (collectionItem.toString() != item) { - equals = false; - diffs.add('collection mismatch: $collectionItem != $item'); - } - collectionItem = collectionItem.nextCollectionItem; - } - } - if (collectionItem != null) { - diffs.add('collection too long: $collectionItem'); - equals = false; - } - } - var iterableLength = changeRecord.iterable.toList().length; - if (iterableLength != count) { - diffs.add('collection length mismatched: $iterableLength != $count'); + List items = _getList(changeRecord.collectionHead, (r) => r.nextCollectionItem); + bool equals = _compareLists("collection", collection, items, diffs); + int iterableLength = changeRecord.iterable.toList().length; + if (iterableLength != items.length) { + diffs.add('collection length mismatched: $iterableLength != ${items.length}'); equals = false; } return equals; } + bool checkPrevious(CollectionChangeRecord changeRecord, List diffs) { + List items = _getList(changeRecord.previousCollectionHead, (r) => r.previousNextItem); + return _compareLists("previous", previous, items, diffs); + } + bool checkAdditions(CollectionChangeRecord changeRecord, List diffs) { - var equals = true; - if (additions != null) { - AddedItem addedItem = changeRecord.additionsHead; - for (var item in additions) { - if (addedItem == null) { - equals = false; - diffs.add('additions too short: $item'); - } else { - if (addedItem.toString() != item) { - equals = false; - diffs.add('additions mismatch: $addedItem != $item'); - } - addedItem = addedItem.nextAddedItem; - } - } - if (addedItem != null) { - equals = false; - diffs.add('additions too long: $addedItem'); - } - } - return equals; + List items = _getList(changeRecord.additionsHead, (r) => r.nextAddedItem); + return _compareLists("additions", additions, items, diffs); } bool checkMoves(CollectionChangeRecord changeRecord, List diffs) { - var equals = true; - if (moves != null) { - MovedItem movedItem = changeRecord.movesHead; - for (var item in moves) { - if (movedItem == null) { - equals = false; - diffs.add('moves too short: $item'); - } else { - if (movedItem.toString() != item) { - equals = false; - diffs.add('moves too mismatch: $movedItem != $item'); - } - movedItem = movedItem.nextMovedItem; - } - } - if (movedItem != null) { - equals = false; - diffs.add('moves too long: $movedItem'); - } - } - return equals; + List items = _getList(changeRecord.movesHead, (r) => r.nextMovedItem); + return _compareLists("moves", moves, items, diffs); } bool checkRemovals(CollectionChangeRecord changeRecord, List diffs) { - var equals = true; - if (removals != null) { - RemovedItem removedItem = changeRecord.removalsHead; - for (var item in removals) { - if (removedItem == null) { - equals = false; - diffs.add('removes too short: $item'); - } else { - if (removedItem.toString() != item) { - equals = false; - diffs.add('removes too mismatch: $removedItem != $item'); - } - removedItem = removedItem.nextRemovedItem; - } - } - if (removedItem != null) { - equals = false; - diffs.add('removes too long: $removedItem'); - } - } - return equals; + List items = _getList(changeRecord.removalsHead, (r) => r.nextRemovedItem); + return _compareLists("removes", removals, items, diffs); } } -class MapRecordMatcher extends Matcher { +class MapRecordMatcher extends _CollectionMatcher { final List map; + final List previous; final List additions; final List changes; final List removals; - MapRecordMatcher({this.map, this.additions, this.changes, this.removals}); + MapRecordMatcher({this.map, this.previous, this.additions, this.changes, this.removals}); Description describeMismatch(changes, Description mismatchDescription, Map matchState, bool verbose) { @@ -777,6 +858,7 @@ class MapRecordMatcher extends Matcher { } add('map', map); + add('previous', previous); add('additions', additions); add('changes', changes); add('removals', removals); @@ -786,105 +868,41 @@ class MapRecordMatcher extends Matcher { bool matches(MapChangeRecord changeRecord, Map matchState) { var diffs = matchState['diffs'] = []; return checkMap(changeRecord, diffs) && + checkPrevious(changeRecord, diffs) && checkAdditions(changeRecord, diffs) && checkChanges(changeRecord, diffs) && checkRemovals(changeRecord, diffs); } bool checkMap(MapChangeRecord changeRecord, List diffs) { - var equals = true; - if (map != null) { - KeyValue mapKeyValue = changeRecord.mapHead; - for (var item in map) { - if (mapKeyValue == null) { - equals = false; - diffs.add('map too short: $item'); - } else { - if (mapKeyValue.toString() != item) { - equals = false; - diffs.add('map mismatch: $mapKeyValue != $item'); - } - mapKeyValue = mapKeyValue.nextKeyValue; - } - } - if (mapKeyValue != null) { - diffs.add('map too long: $mapKeyValue'); - equals = false; - } + List items = _getList(changeRecord.mapHead, (r) => r.nextKeyValue); + bool equals = _compareLists("map", map, items, diffs); + int mapLength = changeRecord.map.length; + if (mapLength != items.length) { + diffs.add('map length mismatched: $mapLength != ${items.length}'); + equals = false; } return equals; } + bool checkPrevious(MapChangeRecord changeRecord, List diffs) { + List items = _getList(changeRecord.previousMapHead, (r) => r.previousNextKeyValue); + return _compareLists("previous", previous, items, diffs); + } + bool checkAdditions(MapChangeRecord changeRecord, List diffs) { - var equals = true; - if (additions != null) { - AddedKeyValue addedKeyValue = changeRecord.additionsHead; - for (var item in additions) { - if (addedKeyValue == null) { - equals = false; - diffs.add('additions too short: $item'); - } else { - if (addedKeyValue.toString() != item) { - equals = false; - diffs.add('additions mismatch: $addedKeyValue != $item'); - } - addedKeyValue = addedKeyValue.nextAddedKeyValue; - } - } - if (addedKeyValue != null) { - equals = false; - diffs.add('additions too long: $addedKeyValue'); - } - } - return equals; + List items = _getList(changeRecord.additionsHead, (r) => r.nextAddedKeyValue); + return _compareLists("additions", additions, items, diffs); } bool checkChanges(MapChangeRecord changeRecord, List diffs) { - var equals = true; - if (changes != null) { - ChangedKeyValue movedKeyValue = changeRecord.changesHead; - for (var item in changes) { - if (movedKeyValue == null) { - equals = false; - diffs.add('changes too short: $item'); - } else { - if (movedKeyValue.toString() != item) { - equals = false; - diffs.add('changes too mismatch: $movedKeyValue != $item'); - } - movedKeyValue = movedKeyValue.nextChangedKeyValue; - } - } - if (movedKeyValue != null) { - equals = false; - diffs.add('changes too long: $movedKeyValue'); - } - } - return equals; + List items = _getList(changeRecord.changesHead, (r) => r.nextChangedKeyValue); + return _compareLists("changes", changes, items, diffs); } bool checkRemovals(MapChangeRecord changeRecord, List diffs) { - var equals = true; - if (removals != null) { - RemovedKeyValue removedKeyValue = changeRecord.removalsHead; - for (var item in removals) { - if (removedKeyValue == null) { - equals = false; - diffs.add('rechanges too short: $item'); - } else { - if (removedKeyValue.toString() != item) { - equals = false; - diffs.add('rechanges too mismatch: $removedKeyValue != $item'); - } - removedKeyValue = removedKeyValue.nextRemovedKeyValue; - } - } - if (removedKeyValue != null) { - equals = false; - diffs.add('rechanges too long: $removedKeyValue'); - } - } - return equals; + List items = _getList(changeRecord.removalsHead, (r) => r.nextRemovedKeyValue); + return _compareLists("removals", removals, items, diffs); } } diff --git a/test/change_detection/watch_group_spec.dart b/test/change_detection/watch_group_spec.dart index 83e60801a..66d28f9e5 100644 --- a/test/change_detection/watch_group_spec.dart +++ b/test/change_detection/watch_group_spec.dart @@ -1,6 +1,7 @@ library watch_group_spec; import '../_specs.dart'; +import 'dart:collection'; import 'package:angular/change_detection/watch_group.dart'; import 'package:angular/change_detection/dirty_checking_change_detector.dart'; import 'package:angular/change_detection/dirty_checking_change_detector_dynamic.dart'; @@ -112,6 +113,103 @@ void main() { expect(logger).toEqual(['hello', 'bye']); }); + describe('sequence mutations and ref changes', () { + it('should handle a simultaneous map mutation and reference change', () { + context['a'] = context['b'] = {1: 10, 2: 20}; + var watchA = watchGrp.watch(new CollectionAST(parse('a')), (v, p) => logger(v)); + var watchB = watchGrp.watch(new CollectionAST(parse('b')), (v, p) => logger(v)); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(2); + expect(logger[0], toEqualMapRecord( + map: ['1', '2'], + previous: ['1', '2'])); + expect(logger[1], toEqualMapRecord( + map: ['1', '2'], + previous: ['1', '2'])); + logger.clear(); + + // context['a'] is set to a copy with an addition. + context['a'] = new Map.from(context['a'])..[3] = 30; + // context['b'] still has the original collection. We'll mutate it. + context['b'].remove(1); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(2); + expect(logger[0], toEqualMapRecord( + map: ['1', '2', '3[null -> 30]'], + previous: ['1', '2'], + additions: ['3[null -> 30]'])); + expect(logger[1], toEqualMapRecord( + map: ['2'], + previous: ['1[10 -> null]', '2'], + removals: ['1[10 -> null]'])); + logger.clear(); + }); + + it('should handle a simultaneous list mutation and reference change', () { + context['a'] = context['b'] = [0, 1]; + var watchA = watchGrp.watch(new CollectionAST(parse('a')), (v, p) => logger(v)); + var watchB = watchGrp.watch(new CollectionAST(parse('b')), (v, p) => logger(v)); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(2); + expect(logger[0], toEqualCollectionRecord( + collection: ['0', '1'], + previous: ['0', '1'], + additions: [], moves: [], removals: [])); + expect(logger[1], toEqualCollectionRecord( + collection: ['0', '1'], + previous: ['0', '1'], + additions: [], moves: [], removals: [])); + logger.clear(); + + // context['a'] is set to a copy with an addition. + context['a'] = context['a'].toList()..add(2); + // context['b'] still has the original collection. We'll mutate it. + context['b'].remove(0); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(2); + expect(logger[0], toEqualCollectionRecord( + collection: ['0', '1', '2[null -> 2]'], + previous: ['0', '1'], + additions: ['2[null -> 2]'], + moves: [], + removals: [])); + expect(logger[1], toEqualCollectionRecord( + collection: ['1[1 -> 0]'], + previous: ['0[0 -> null]', '1[1 -> 0]'], + additions: [], + moves: ['1[1 -> 0]'], + removals: ['0[0 -> null]'])); + logger.clear(); + }); + + it('should work correctly with UnmodifiableListView', () { + context['a'] = new UnmodifiableListView([0, 1]); + var watch = watchGrp.watch(new CollectionAST(parse('a')), (v, p) => logger(v)); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(1); + expect(logger[0], toEqualCollectionRecord( + collection: ['0', '1'], + previous: ['0', '1'])); + logger.clear(); + + context['a'] = new UnmodifiableListView([1, 0]); + + watchGrp.detectChanges(); + expect(logger.length).toEqual(1); + expect(logger[0], toEqualCollectionRecord( + collection: ['1[1 -> 0]', '0[0 -> 1]'], + previous: ['0[0 -> 1]', '1[1 -> 0]'], + moves: ['1[1 -> 0]', '0[0 -> 1]'])); + logger.clear(); + }); + + }); + it('should read property chain', () { context['a'] = {'b': 'hello'}; @@ -569,6 +667,7 @@ void main() { watchGrp.detectChanges(); expect(logger[0], toEqualCollectionRecord( collection: ['2[null -> 0]'], + previous: ['1[0 -> null]'], additions: ['2[null -> 0]'], moves: [], removals: ['1[0 -> null]']));