From 75fbded7ad2691eb4391c56a595ab488842a85ed Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Wed, 5 Mar 2014 12:27:00 -0800 Subject: [PATCH] fix(change-detection): remove memory leak, use iterator When the change detection finds change it creates a linked list of records which have changed. The linked list is not bound to groups but rather crosses groups, this implies that if we remove a group it is possible that a record in a live group retains a pointer to a record in the now deleted record. While technically not a leak, the memory will not be realized until that record detects another change and gets re-added to the dirty list. If the record never fires than the memory is retained forever. --- lib/change_detection/ast.dart | 6 +- lib/change_detection/change_detection.dart | 24 +-- .../dirty_checking_change_detector.dart | 60 ++++-- lib/change_detection/linked_list.dart | 24 +-- lib/change_detection/watch_group.dart | 69 +++--- .../dirty_checking_change_detector_spec.dart | 201 +++++++++++------- 6 files changed, 223 insertions(+), 161 deletions(-) diff --git a/lib/change_detection/ast.dart b/lib/change_detection/ast.dart index 66c55fba3..0ff5e4b1e 100644 --- a/lib/change_detection/ast.dart +++ b/lib/change_detection/ast.dart @@ -123,8 +123,8 @@ String _argList(List items) => items.join(', '); /** * The name is a bit oxymoron, but it is essentially the NullObject pattern. * - * This allows children to set a handler on this ChangeRecord and then let it write the initial - * constant value to the forwarding ChangeRecord. + * This allows children to set a handler on this Record and then let it write the initial + * constant value to the forwarding Record. */ class _ConstantWatchRecord extends WatchRecord<_Handler> { final currentValue; @@ -134,7 +134,7 @@ class _ConstantWatchRecord extends WatchRecord<_Handler> { : currentValue = currentValue, handler = new _ConstantHandler(watchGroup, expression, currentValue); - ChangeRecord<_Handler> check() => null; + bool check() => false; void remove() => null; get field => null; diff --git a/lib/change_detection/change_detection.dart b/lib/change_detection/change_detection.dart index 5db6300cd..c3394708a 100644 --- a/lib/change_detection/change_detection.dart +++ b/lib/change_detection/change_detection.dart @@ -23,7 +23,7 @@ abstract class ChangeDetectorGroup { * Parameters: * - [object] to watch. * - [field] to watch on the [object]. - * - [handler] an opaque object passed on to [ChangeRecord]. + * - [handler] an opaque object passed on to [Record]. */ WatchRecord watch(Object object, String field, H handler); @@ -43,7 +43,7 @@ abstract class ChangeDetectorGroup { * predictable performance, and the developer can implement `.equals()` on top * of identity checks. * - * - [H] A [ChangeRecord] has associated handler object. The handler object is + * - [H] A [Record] has associated handler object. The handler object is * opaque to the [ChangeDetector] but it is meaningful to the code which * registered the watcher. It can be a data structure, an object, or a function. * It is up to the developer to attach meaning to it. @@ -51,10 +51,10 @@ abstract class ChangeDetectorGroup { abstract class ChangeDetector extends ChangeDetectorGroup { /** * This method does the work of collecting the changes and returns them as a - * linked list of [ChangeRecord]s. The [ChangeRecord]s are returned in the + * linked list of [Record]s. The [Record]s are returned in the * same order as they were registered. */ - ChangeRecord collectChanges({ EvalExceptionHandler exceptionHandler, + Iterator> collectChanges({ EvalExceptionHandler exceptionHandler, AvgStopwatch stopwatch }); } @@ -93,24 +93,14 @@ abstract class WatchRecord extends Record { set object(value); /** - * Check to see if the field on the object has changed. Returns [null] if no - * change, or a [ChangeRecord] if a change has been detected. + * Check to see if the field on the object has changed. Returns [true] if + * change has been detected. */ - ChangeRecord check(); + bool check(); void remove(); } -/** - * Provides information about the changes which were detected in objects. - * - * It exposes a `nextChange` method for traversing all of the changes. - */ -abstract class ChangeRecord extends Record { - /** Next [ChangeRecord] */ - ChangeRecord get nextChange; -} - /** * If the [ChangeDetector] is watching a [Map] then the [currentValue] of * [Record] will contain an instance of this object. A [MapChangeRecord] diff --git a/lib/change_detection/dirty_checking_change_detector.dart b/lib/change_detection/dirty_checking_change_detector.dart index 8b32e3b78..48967fda3 100644 --- a/lib/change_detection/dirty_checking_change_detector.dart +++ b/lib/change_detection/dirty_checking_change_detector.dart @@ -37,13 +37,6 @@ class GetterCache { * Each property to be watched is recorded as a [DirtyCheckingRecord] and kept * in a linked list. Linked list are faster than Arrays for iteration. They also * allow removal of large blocks of watches in an efficient manner. - * - * [ChangeRecord] - * - * When the results are delivered they are a linked list of [ChangeRecord]s. For - * efficiency reasons the [DirtyCheckingRecord] and [ChangeRecord] are two - * different interfaces for the same underlying object this makes reporting - * efficient since no additional memory allocation is performed. */ class DirtyCheckingChangeDetectorGroup implements ChangeDetectorGroup { /** @@ -177,6 +170,10 @@ class DirtyCheckingChangeDetectorGroup implements ChangeDetectorGroup { nextGroup._prev = prevGroup; } _parent = null; + _prev = _next = null; + _recordHead._prevRecord = null; + _recordTail._nextRecord = null; + _recordHead = _recordTail = null; assert(root._assertRecordsOk()); } @@ -281,7 +278,7 @@ class DirtyCheckingChangeDetector extends DirtyCheckingChangeDetectorGroup return true; } - DirtyCheckingRecord collectChanges({ EvalExceptionHandler exceptionHandler, + Iterator> collectChanges({ EvalExceptionHandler exceptionHandler, AvgStopwatch stopwatch}) { if (stopwatch != null) stopwatch.start(); DirtyCheckingRecord changeHead = null; @@ -291,11 +288,11 @@ class DirtyCheckingChangeDetector extends DirtyCheckingChangeDetectorGroup int count = 0; while (current != null) { try { - if (current.check() != null) { + if (current.check()) { if (changeHead == null) { changeHead = changeTail = current; } else { - changeTail = changeTail.nextChange = current; + changeTail = changeTail._nextChange = current; } } if (stopwatch != null) count++; @@ -308,9 +305,9 @@ class DirtyCheckingChangeDetector extends DirtyCheckingChangeDetectorGroup } current = current._nextRecord; } - if (changeTail != null) changeTail.nextChange = null; + if (changeTail != null) changeTail._nextChange = null; if (stopwatch != null) stopwatch..stop()..increment(count); - return changeHead; + return new _ChangeIterator(changeHead); } void remove() { @@ -318,6 +315,29 @@ class DirtyCheckingChangeDetector extends DirtyCheckingChangeDetectorGroup } } +class _ChangeIterator implements Iterator>{ + DirtyCheckingRecord _current; + DirtyCheckingRecord _next; + DirtyCheckingRecord get current => _current; + + _ChangeIterator(this._next); + + bool moveNext() { + _current = _next; + if (_next != null) { + _next = _current._nextChange; + /* + * This is important to prevent memory leaks. If we don't reset then + * a record maybe pointing to a deleted change detector group and it + * will not release the reference until it fires again. So we have + * to be eager about releasing references. + */ + _current._nextChange = null; + } + return _current != null; + } +} + /** * [DirtyCheckingRecord] represents as single item to check. The heart of the * [DirtyCheckingRecord] is a the [check] method which can read the @@ -327,7 +347,7 @@ class DirtyCheckingChangeDetector extends DirtyCheckingChangeDetectorGroup * removing efficient. [DirtyCheckingRecord] also has a [nextChange] field which * creates a single linked list of all of the changes for efficient traversal. */ -class DirtyCheckingRecord implements ChangeRecord, WatchRecord { +class DirtyCheckingRecord implements Record, WatchRecord { static const List _MODE_NAMES = const ['MARKER', 'IDENT', 'REFLECT', 'GETTER', 'MAP[]', 'ITERABLE', 'MAP']; static const int _MODE_MARKER_ = 0; @@ -350,7 +370,7 @@ class DirtyCheckingRecord implements ChangeRecord, WatchRecord { var currentValue; DirtyCheckingRecord _nextRecord; DirtyCheckingRecord _prevRecord; - ChangeRecord nextChange; + Record _nextChange; var _object; InstanceMirror _instanceMirror; @@ -416,12 +436,12 @@ class DirtyCheckingRecord implements ChangeRecord, WatchRecord { } } - ChangeRecord check() { + bool check() { assert(_mode != null); var current; switch (_mode) { case _MODE_MARKER_: - return null; + return false; case _MODE_REFLECT_: current = _instanceMirror.getField(_symbol).reflectee; break; @@ -435,9 +455,9 @@ class DirtyCheckingRecord implements ChangeRecord, WatchRecord { current = object; break; case _MODE_MAP_: - return (currentValue as _MapChangeRecord)._check(object) ? this : null; + return (currentValue as _MapChangeRecord)._check(object); case _MODE_ITERABLE_: - return (currentValue as _CollectionChangeRecord)._check(object) ? this : null; + return (currentValue as _CollectionChangeRecord)._check(object); default: assert(false); } @@ -454,10 +474,10 @@ class DirtyCheckingRecord implements ChangeRecord, WatchRecord { } else { previousValue = last; currentValue = current; - return this; + return true; } } - return null; + return false; } diff --git a/lib/change_detection/linked_list.dart b/lib/change_detection/linked_list.dart index 1c31c5657..9376ab811 100644 --- a/lib/change_detection/linked_list.dart +++ b/lib/change_detection/linked_list.dart @@ -92,19 +92,19 @@ abstract class _EvalWatchList { static _EvalWatchRecord _add(_EvalWatchList list, _EvalWatchRecord item) { assert(item._nextEvalWatch == null); - assert(item._previousEvalWatch == null); + assert(item._prevEvalWatch == null); var prev = list._evalWatchTail; var next = prev._nextEvalWatch; if (prev == list._marker) { list._evalWatchHead = list._evalWatchTail = item; - prev = prev._previousEvalWatch; + prev = prev._prevEvalWatch; } item._nextEvalWatch = next; - item._previousEvalWatch = prev; + item._prevEvalWatch = prev; if (prev != null) prev._nextEvalWatch = item; - if (next != null) next._previousEvalWatch = item; + if (next != null) next._prevEvalWatch = item; return list._evalWatchTail = item; } @@ -113,21 +113,21 @@ abstract class _EvalWatchList { static void _remove(_EvalWatchList list, _EvalWatchRecord item) { assert(item.watchGrp == list); - var prev = item._previousEvalWatch; + var prev = item._prevEvalWatch; var next = item._nextEvalWatch; if (list._evalWatchHead == list._evalWatchTail) { list._evalWatchHead = list._evalWatchTail = list._marker; list._marker .._nextEvalWatch = next - .._previousEvalWatch = prev; + .._prevEvalWatch = prev; if (prev != null) prev._nextEvalWatch = list._marker; - if (next != null) next._previousEvalWatch = list._marker; + if (next != null) next._prevEvalWatch = list._marker; } else { if (item == list._evalWatchHead) list._evalWatchHead = next; if (item == list._evalWatchTail) list._evalWatchTail = prev; if (prev != null) prev._nextEvalWatch = next; - if (next != null) next._previousEvalWatch = prev; + if (next != null) next._prevEvalWatch = prev; } } } @@ -137,11 +137,11 @@ class _WatchGroupList { static WatchGroup _add(_WatchGroupList list, WatchGroup item) { assert(item._nextWatchGroup == null); - assert(item._previousWatchGroup == null); + assert(item._prevWatchGroup == null); if (list._watchGroupTail == null) { list._watchGroupHead = list._watchGroupTail = item; } else { - item._previousWatchGroup = list._watchGroupTail; + item._prevWatchGroup = list._watchGroupTail; list._watchGroupTail._nextWatchGroup = item; list._watchGroupTail = item; } @@ -151,10 +151,10 @@ class _WatchGroupList { static bool _isEmpty(_WatchGroupList list) => list._watchGroupHead == null; static void _remove(_WatchGroupList list, WatchGroup item) { - var previous = item._previousWatchGroup; + var previous = item._prevWatchGroup; var next = item._nextWatchGroup; if (previous == null) list._watchGroupHead = next; else previous._nextWatchGroup = next; - if (next == null) list._watchGroupTail = previous; else next._previousWatchGroup = previous; + if (next == null) list._watchGroupTail = previous; else next._prevWatchGroup = previous; } } diff --git a/lib/change_detection/watch_group.dart b/lib/change_detection/watch_group.dart index d39b17b5d..501133733 100644 --- a/lib/change_detection/watch_group.dart +++ b/lib/change_detection/watch_group.dart @@ -92,9 +92,9 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { int _nextChildId = 0; _EvalWatchRecord _evalWatchHead, _evalWatchTail; /// Pointer for creating tree of [WatchGroup]s. - WatchGroup _watchGroupHead, _watchGroupTail, _previousWatchGroup, - _nextWatchGroup; WatchGroup _parentWatchGroup; + WatchGroup _watchGroupHead, _watchGroupTail; + WatchGroup _prevWatchGroup, _nextWatchGroup; WatchGroup._child(_parentWatchGroup, this._changeDetector, this.context, this._cache, this._rootGroup) @@ -143,7 +143,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { WatchRecord<_Handler> addFieldWatch(AST lhs, String name, String expression) { var fieldHandler = new _FieldHandler(this, expression); - // Create a ChangeRecord for the current field and assign the change record + // Create a Record for the current field and assign the change record // to the handler. var watchRecord = _changeDetector.watch(null, name, fieldHandler); _fieldCost++; @@ -257,15 +257,15 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { this, _changeDetector.newGroup(), context == null ? this.context : context, - context == null ? this._cache: >{}, + >{}, _rootGroup == null ? this : _rootGroup); _WatchGroupList._add(this, childGroup); var marker = childGroup._marker; - marker._previousEvalWatch = prev; + marker._prevEvalWatch = prev; marker._nextEvalWatch = next; prev._nextEvalWatch = marker; - if (next != null) next._previousEvalWatch = marker; + if (next != null) next._prevEvalWatch = marker; return childGroup; } @@ -276,9 +276,9 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { void remove() { // TODO:(misko) This code is not right. // 1) It fails to release [ChangeDetector] [WatchRecord]s. - // 2) it needs to cleanup caches if the cache is being shared. _WatchGroupList._remove(_parentWatchGroup, this); + _nextWatchGroup = _prevWatchGroup = null; _changeDetector.remove(); _rootGroup._removeCount++; _parentWatchGroup = null; @@ -287,10 +287,13 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { _EvalWatchRecord firstEvalWatch = _evalWatchHead; _EvalWatchRecord lastEvalWatch = (_watchGroupTail == null ? this : _watchGroupTail)._evalWatchTail; - _EvalWatchRecord previous = firstEvalWatch._previousEvalWatch; + _EvalWatchRecord previous = firstEvalWatch._prevEvalWatch; _EvalWatchRecord next = lastEvalWatch._nextEvalWatch; if (previous != null) previous._nextEvalWatch = next; - if (next != null) next._previousEvalWatch = previous; + if (next != null) next._prevEvalWatch = previous; + _evalWatchHead._prevEvalWatch = null; + _evalWatchTail._nextEvalWatch = null; + _evalWatchHead = _evalWatchTail = null; } toString() { @@ -301,7 +304,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList { var prev = null; while (watch != null) { allWatches.add(watch.toString()); - assert(watch._previousEvalWatch == prev); + assert(watch._prevEvalWatch == prev); prev = watch; watch = watch._nextEvalWatch; } @@ -363,18 +366,18 @@ class RootWatchGroup extends WatchGroup { AvgStopwatch fieldStopwatch, AvgStopwatch evalStopwatch, AvgStopwatch processStopwatch}) { - // Process the ChangeRecords from the change detector - ChangeRecord<_Handler> changeRecord = + // Process the Records from the change detector + Iterator> changedRecordIterator = (_changeDetector as ChangeDetector<_Handler>).collectChanges( exceptionHandler:exceptionHandler, stopwatch: fieldStopwatch); if (processStopwatch != null) processStopwatch.start(); - while (changeRecord != null) { - if (changeLog != null) changeLog(changeRecord.handler.expression, - changeRecord.currentValue, - changeRecord.previousValue); - changeRecord.handler.onChange(changeRecord); - changeRecord = changeRecord.nextChange; + while (changedRecordIterator.moveNext()) { + var record = changedRecordIterator.current; + if (changeLog != null) changeLog(record.handler.expression, + record.currentValue, + record.previousValue); + record.handler.onChange(record); } if (processStopwatch != null) processStopwatch.stop(); @@ -385,8 +388,7 @@ class RootWatchGroup extends WatchGroup { while (evalRecord != null) { try { if (evalStopwatch != null) evalCount++; - var change = evalRecord.check(); - if (change != null && changeLog != null) { + if (evalRecord.check() && changeLog != null) { changeLog(evalRecord.handler.expression, evalRecord.currentValue, evalRecord.previousValue); @@ -492,6 +494,7 @@ class Watch { * changes detected at one handler are propagated to the next handler. */ abstract class _Handler implements _LinkedList, _LinkedListItem, _WatchList { + // Used for forwarding changes to delegates _Handler _head, _tail; _Handler _next, _previous; Watch _watchHead, _watchTail; @@ -543,7 +546,7 @@ abstract class _Handler implements _LinkedList, _LinkedListItem, _WatchList { } acceptValue(object) => null; - void onChange(ChangeRecord<_Handler> record) { + void onChange(Record<_Handler> record) { assert(_next != this); // verify we are not detached // If we have reaction functions than queue them up for asynchronous // processing. @@ -579,8 +582,7 @@ class _FieldHandler extends _Handler { */ void acceptValue(object) { watchRecord.object = object; - var changeRecord = watchRecord.check(); - if (changeRecord != null) onChange(changeRecord); + if (watchRecord.check()) onChange(watchRecord); } } @@ -592,8 +594,7 @@ class _CollectionHandler extends _Handler { */ void acceptValue(object) { watchRecord.object = object; - var changeRecord = watchRecord.check(); - if (changeRecord != null) onChange(changeRecord); + if (watchRecord.check()) onChange(watchRecord); } void _releaseWatch() { @@ -631,7 +632,7 @@ class _InvokeHandler extends _Handler implements _ArgHandlerList { watchRecord.object = object; } - void onChange(ChangeRecord<_Handler> record) { + void onChange(Record<_Handler> record) { super.onChange(record); } @@ -650,7 +651,7 @@ class _InvokeHandler extends _Handler implements _ArgHandlerList { } -class _EvalWatchRecord implements WatchRecord<_Handler>, ChangeRecord<_Handler> { +class _EvalWatchRecord implements WatchRecord<_Handler>, Record<_Handler> { static const int _MODE_DELETED_ = -1; static const int _MODE_MARKER_ = 0; static const int _MODE_FUNCTION_ = 1; @@ -670,7 +671,7 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, ChangeRecord<_Handler> bool dirtyArgs = true; dynamic currentValue, previousValue, _object; - _EvalWatchRecord _previousEvalWatch, _nextEvalWatch; + _EvalWatchRecord _prevEvalWatch, _nextEvalWatch; _EvalWatchRecord(this.watchGrp, this.handler, this.fn, name, int arity) : args = new List(arity), @@ -730,19 +731,19 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, ChangeRecord<_Handler> } } - ChangeRecord<_Handler> check() { + bool check() { var value; switch (mode) { case _MODE_MARKER_: case _MODE_NULL_: - return null; + return false; case _MODE_FUNCTION_: - if (!dirtyArgs) return null; + if (!dirtyArgs) return false; value = Function.apply(fn, args); dirtyArgs = false; break; case _MODE_FUNCTION_APPLY_: - if (!dirtyArgs) return null; + if (!dirtyArgs) return false; value = (fn as FunctionApply).apply(args); dirtyArgs = false; break; @@ -770,10 +771,10 @@ class _EvalWatchRecord implements WatchRecord<_Handler>, ChangeRecord<_Handler> previousValue = current; currentValue = value; handler.onChange(this); - return this; + return true; } } - return null; + return false; } get nextChange => null; diff --git a/test/change_detection/dirty_checking_change_detector_spec.dart b/test/change_detection/dirty_checking_change_detector_spec.dart index 765776ab1..8f54dca37 100644 --- a/test/change_detection/dirty_checking_change_detector_spec.dart +++ b/test/change_detection/dirty_checking_change_detector_spec.dart @@ -20,42 +20,45 @@ void main() { describe('object field', () { it('should detect nothing', () { var changes = detector.collectChanges(); - expect(changes).toEqual(null); + expect(changes.moveNext()).toEqual(false); }); it('should detect field changes', () { var user = new _User('', ''); - var change; + Iterator changeIterator; detector ..watch(user, 'first', null) ..watch(user, 'last', null) ..collectChanges(); // throw away first set - change = detector.collectChanges(); - expect(change).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(false); user..first = 'misko' ..last = 'hevery'; - change = detector.collectChanges(); - expect(change.currentValue).toEqual('misko'); - expect(change.previousValue).toEqual(''); - expect(change.nextChange.currentValue).toEqual('hevery'); - expect(change.nextChange.previousValue).toEqual(''); - expect(change.nextChange.nextChange).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue).toEqual('misko'); + expect(changeIterator.current.previousValue).toEqual(''); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue).toEqual('hevery'); + expect(changeIterator.current.previousValue).toEqual(''); + expect(changeIterator.moveNext()).toEqual(false); // force different instance user.first = 'mis'; user.first += 'ko'; - change = detector.collectChanges(); - expect(change).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(false); user.last = 'Hevery'; - change = detector.collectChanges(); - expect(change.currentValue).toEqual('Hevery'); - expect(change.previousValue).toEqual('hevery'); - expect(change.nextChange).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue).toEqual('Hevery'); + expect(changeIterator.current.previousValue).toEqual('hevery'); + expect(changeIterator.moveNext()).toEqual(false); }); it('should ignore NaN != NaN', () { @@ -63,14 +66,15 @@ void main() { user.age = double.NAN; detector..watch(user, 'age', null)..collectChanges(); // throw away first set - var changes = detector.collectChanges(); - expect(changes).toEqual(null); + var changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(false); user.age = 123; - changes = detector.collectChanges(); - expect(changes.currentValue).toEqual(123); - expect(changes.previousValue.isNaN).toEqual(true); - expect(changes.nextChange).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue).toEqual(123); + expect(changeIterator.current.previousValue.isNaN).toEqual(true); + expect(changeIterator.moveNext()).toEqual(false); }); it('should treat map field dereference as []', () { @@ -79,9 +83,10 @@ void main() { detector.collectChanges(); // throw away first set obj['name'] = 'Misko'; - var changes = detector.collectChanges(); - expect(changes.currentValue).toEqual('Misko'); - expect(changes.previousValue).toEqual('misko'); + var changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue).toEqual('Misko'); + expect(changeIterator.current.previousValue).toEqual('misko'); }); }); @@ -92,21 +97,24 @@ void main() { var b = detector.watch(obj, 'b', 'b'); obj['a'] = obj['b'] = 1; - var changes = detector.collectChanges(); - expect(changes.handler).toEqual('a'); - expect(changes.nextChange.handler).toEqual('b'); - expect(changes.nextChange.nextChange).toEqual(null); + var changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.handler).toEqual('a'); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.handler).toEqual('b'); + expect(changeIterator.moveNext()).toEqual(false); obj['a'] = obj['b'] = 2; a.remove(); - changes = detector.collectChanges(); - expect(changes.handler).toEqual('b'); - expect(changes.nextChange).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.handler).toEqual('b'); + expect(changeIterator.moveNext()).toEqual(false); obj['a'] = obj['b'] = 3; b.remove(); - changes = detector.collectChanges(); - expect(changes).toEqual(null); + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(false); }); it('should remove all watches in group and group\'s children', () { @@ -121,6 +129,7 @@ void main() { child1a.watch(obj,'a', '1A'); child2.watch(obj,'a', '2A'); + var iterator; obj['a'] = 1; expect(detector.collectChanges(), toEqualChanges(['0a', '0A', '1a', '1A', '2A', '1b'])); @@ -135,6 +144,7 @@ void main() { var ra = detector.watch(obj, 'a', 'a'); var child = detector.newGroup(); var cb = child.watch(obj,'b', 'b'); + var iterotar; obj['a'] = obj['b'] = 1; expect(detector.collectChanges(), toEqualChanges(['a', 'b'])); @@ -166,17 +176,22 @@ void main() { it('should detect changes in list', () { var list = []; var record = detector.watch(list, null, 'handler'); - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); + var iterator; list.add('a'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a[null -> 0]'], additions: ['a[null -> 0]'], moves: [], removals: [])); list.add('b'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b[null -> 1]'], additions: ['b[null -> 1]'], moves: [], @@ -184,7 +199,9 @@ void main() { list.add('c'); list.add('d'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'c[null -> 2]', 'd[null -> 3]'], additions: ['c[null -> 2]', 'd[null -> 3]'], moves: [], @@ -192,7 +209,9 @@ void main() { list.remove('c'); expect(list).toEqual(['a', 'b', 'd']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'd[3 -> 2]'], additions: [], moves: ['d[3 -> 2]'], @@ -200,7 +219,9 @@ void main() { list.clear(); list.addAll(['d', 'c', 'b', 'a']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges(); + expect(iterator.moveNext()).toEqual(true); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['d[2 -> 0]', 'c[null -> 1]', 'b[1 -> 2]', 'a[0 -> 3]'], additions: ['c[null -> 1]'], moves: ['d[2 -> 0]', 'b[1 -> 2]', 'a[0 -> 3]'], @@ -210,17 +231,20 @@ void main() { it('should detect changes in list', () { var list = []; var record = detector.watch(list.map((i) => i), null, 'handler'); - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); + var iterator; list.add('a'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a[null -> 0]'], additions: ['a[null -> 0]'], moves: [], removals: [])); list.add('b'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b[null -> 1]'], additions: ['b[null -> 1]'], moves: [], @@ -228,7 +252,8 @@ void main() { list.add('c'); list.add('d'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'c[null -> 2]', 'd[null -> 3]'], additions: ['c[null -> 2]', 'd[null -> 3]'], moves: [], @@ -236,7 +261,8 @@ void main() { list.remove('c'); expect(list).toEqual(['a', 'b', 'd']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b', 'd[3 -> 2]'], additions: [], moves: ['d[3 -> 2]'], @@ -244,7 +270,8 @@ void main() { list.clear(); list.addAll(['d', 'c', 'b', 'a']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['d[2 -> 0]', 'c[null -> 1]', 'b[1 -> 2]', 'a[0 -> 3]'], additions: ['c[null -> 1]'], moves: ['d[2 -> 0]', 'b[1 -> 2]', 'a[0 -> 3]'], @@ -257,23 +284,25 @@ void main() { list[1] = 'b' + 'oo'; - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); it('should ignore [NaN] != [NaN]', () { var list = [double.NAN]; var record = detector..watch(list, null, null)..collectChanges(); - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); it('should remove and add same item', () { var list = ['a', 'b', 'c']; var record = detector.watch(list, null, 'handler'); + var iterator; detector.collectChanges(); list.remove('b'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'c[2 -> 1]'], additions: [], moves: ['c[2 -> 1]'], @@ -281,7 +310,8 @@ void main() { list.insert(1, 'b'); expect(list).toEqual(['a', 'b', 'c']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'b[null -> 1]', 'c[1 -> 2]'], additions: ['b[null -> 1]'], moves: ['c[1 -> 2]'], @@ -294,7 +324,8 @@ void main() { detector.collectChanges(); list.removeAt(0); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + var iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['a', 'a', 'b[3 -> 2]', 'b[4 -> 3]'], additions: [], moves: ['b[3 -> 2]', 'b[4 -> 3]'], @@ -305,10 +336,12 @@ void main() { it('should support insertions/moves', () { var list = ['a', 'a', 'b', 'b']; var record = detector.watch(list, null, 'handler'); + var iterator; detector.collectChanges(); list.insert(0, 'b'); expect(list).toEqual(['b', 'a', 'a', 'b', 'b']); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['b[2 -> 0]', 'a[0 -> 1]', 'a[1 -> 2]', 'b', 'b[null -> 4]'], additions: ['b[null -> 4]'], moves: ['b[2 -> 0]', 'a[0 -> 1]', 'a[1 -> 2]'], @@ -319,25 +352,29 @@ void main() { var hiddenList = [1]; var list = new UnmodifiableListView(hiddenList); var record = detector.watch(list, null, 'handler'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + var iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['1[null -> 0]'], additions: ['1[null -> 0]'], moves: [], removals: [])); // assert no changes detected - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); // change the hiddenList normally this should trigger change detection // but because we are wrapped in UnmodifiableListView we see nothing. hiddenList[0] = 2; - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); it('should bug', () { var list = [1, 2, 3, 4]; var record = detector.watch(list, null, 'handler'); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + var iterator; + + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['1[null -> 0]', '2[null -> 1]', '3[null -> 2]', '4[null -> 3]'], additions: ['1[null -> 0]', '2[null -> 1]', '3[null -> 2]', '4[null -> 3]'], moves: [], @@ -345,14 +382,16 @@ void main() { detector.collectChanges(); list.removeRange(0, 1); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['2[1 -> 0]', '3[2 -> 1]', '4[3 -> 2]'], additions: [], moves: ['2[1 -> 0]', '3[2 -> 1]', '4[3 -> 2]'], removals: ['1[0 -> null]'])); list.insert(0, 1); - expect(detector.collectChanges().currentValue, toEqualCollectionRecord( + iterator = detector.collectChanges()..moveNext(); + expect(iterator.current.currentValue, toEqualCollectionRecord( collection: ['1[null -> 0]', '2[0 -> 1]', '3[1 -> 2]', '4[2 -> 3]'], additions: ['1[null -> 0]'], moves: ['2[0 -> 1]', '3[1 -> 2]', '4[2 -> 3]'], @@ -364,17 +403,22 @@ void main() { it('should do basic map watching', () { var map = {}; var record = detector.watch(map, null, 'handler'); - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); + var changeIterator; map['a'] = 'A'; - expect(detector.collectChanges().currentValue, toEqualMapRecord( + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a[null -> A]'], additions: ['a[null -> A]'], changes: [], removals: [])); map['b'] = 'B'; - expect(detector.collectChanges().currentValue, toEqualMapRecord( + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'b[null -> B]'], additions: ['b[null -> B]'], changes: [], @@ -382,7 +426,9 @@ void main() { map['b'] = 'BB'; map['d'] = 'D'; - expect(detector.collectChanges().currentValue, toEqualMapRecord( + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'b[B -> BB]', 'd[null -> D]'], additions: ['d[null -> D]'], changes: ['b[B -> BB]'], @@ -390,14 +436,18 @@ void main() { map.remove('b'); expect(map).toEqual({'a': 'A', 'd':'D'}); - expect(detector.collectChanges().currentValue, toEqualMapRecord( + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue, toEqualMapRecord( map: ['a', 'd'], additions: [], changes: [], removals: ['b[BB -> null]'])); map.clear(); - expect(detector.collectChanges().currentValue, toEqualMapRecord( + changeIterator = detector.collectChanges(); + expect(changeIterator.moveNext()).toEqual(true); + expect(changeIterator.current.currentValue, toEqualMapRecord( map: [], additions: [], changes: [], @@ -410,7 +460,7 @@ void main() { map['f' + 'oo'] = 0; - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); it('should test string values by value rather than by reference', () { @@ -419,14 +469,14 @@ void main() { map['foo'] = 'b' + 'ar'; - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); it('should not see a NaN value as a change', () { var map = {'foo': double.NAN}; var record = detector..watch(map, null, null)..collectChanges(); - expect(detector.collectChanges()).toEqual(null); + expect(detector.collectChanges().moveNext()).toEqual(false); }); }); @@ -486,21 +536,20 @@ class ChangeMatcher extends Matcher { Description describe(Description description) => description..add(expected.toString()); - Description describeMismatch(changes, Description mismatchDescription, + Description describeMismatch(Iterator changes, + Description mismatchDescription, Map matchState, bool verbose) { List list = []; - while(changes != null) { - list.add(changes.handler); - changes = changes.nextChange; + while(changes.moveNext()) { + list.add(changes.current.handler); } return mismatchDescription..add(list.toString()); } - bool matches(changes, Map matchState) { + bool matches(Iterator changes, Map matchState) { int count = 0; - while(changes != null) { - if (changes.handler != expected[count++]) return false; - changes = changes.nextChange; + while(changes.moveNext()) { + if (changes.current.handler != expected[count++]) return false; } return count == expected.length; } @@ -518,6 +567,7 @@ class CollectionRecordMatcher extends Matcher { Description describeMismatch(changes, Description mismatchDescription, Map matchState, bool verbose) { List diffs = matchState['diffs']; + if (diffs == null) return mismatchDescription; return mismatchDescription..add(diffs.join('\n')); } @@ -651,6 +701,7 @@ class MapRecordMatcher extends Matcher { Description describeMismatch(changes, Description mismatchDescription, Map matchState, bool verbose) { List diffs = matchState['diffs']; + if (diffs == null) return mismatchDescription; return mismatchDescription..add(diffs.join('\n')); }