From 75a9e57640573aa683678db45e9f59e5f59d2368 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Mon, 27 Mar 2017 11:53:19 -0700 Subject: [PATCH] fix(core): fix the key/value differ fixes #15457 --- .../differs/default_keyvalue_differ.ts | 217 +++++++++--------- .../differs/default_keyvalue_differ_spec.ts | 24 +- 2 files changed, 129 insertions(+), 112 deletions(-) diff --git a/packages/core/src/change_detection/differs/default_keyvalue_differ.ts b/packages/core/src/change_detection/differs/default_keyvalue_differ.ts index 3e4842df7657b..4285b2b477e5a 100644 --- a/packages/core/src/change_detection/differs/default_keyvalue_differ.ts +++ b/packages/core/src/change_detection/differs/default_keyvalue_differ.ts @@ -27,13 +27,20 @@ export class DefaultKeyValueDifferFactory implements KeyValueDifferFactory } export class DefaultKeyValueDiffer implements KeyValueDiffer, KeyValueChanges { - private _records: Map = new Map(); + private _records = new Map>(); + private _mapHead: KeyValueChangeRecord_ = null; + // _appendAfter is used in the check loop + private _appendAfter: KeyValueChangeRecord_ = null; + private _previousMapHead: KeyValueChangeRecord_ = null; + private _changesHead: KeyValueChangeRecord_ = null; private _changesTail: KeyValueChangeRecord_ = null; + private _additionsHead: KeyValueChangeRecord_ = null; private _additionsTail: KeyValueChangeRecord_ = null; + private _removalsHead: KeyValueChangeRecord_ = null; private _removalsTail: KeyValueChangeRecord_ = null; @@ -90,67 +97,130 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer, KeyVal onDestroy() {} + /** + * Check the current state of the map vs the previous. + * The algorithm is optimised for when the keys do no change. + */ check(map: Map|{[k: string]: any}): boolean { this._reset(); - const records = this._records; - let oldSeqRecord: KeyValueChangeRecord_ = this._mapHead; - let lastOldSeqRecord: KeyValueChangeRecord_ = null; - let lastNewSeqRecord: KeyValueChangeRecord_ = null; - let seqChanged: boolean = false; + + let insertBefore = this._mapHead; + this._appendAfter = null; this._forEach(map, (value: any, key: any) => { - let newSeqRecord: any; - if (oldSeqRecord && key === oldSeqRecord.key) { - newSeqRecord = oldSeqRecord; - this._maybeAddToChanges(newSeqRecord, value); + if (insertBefore && insertBefore.key === key) { + this._maybeAddToChanges(insertBefore, value); + this._appendAfter = insertBefore; + insertBefore = insertBefore._next; } else { - seqChanged = true; - if (oldSeqRecord !== null) { - this._removeFromSeq(lastOldSeqRecord, oldSeqRecord); - this._addToRemovals(oldSeqRecord); - } - if (records.has(key)) { - newSeqRecord = records.get(key); - this._maybeAddToChanges(newSeqRecord, value); - } else { - newSeqRecord = new KeyValueChangeRecord_(key); - records.set(key, newSeqRecord); - newSeqRecord.currentValue = value; - this._addToAdditions(newSeqRecord); - } + const record = this._getOrCreateRecordForKey(key, value); + insertBefore = this._insertBeforeOrAppend(insertBefore, record); } + }); - if (seqChanged) { - if (this._isInRemovals(newSeqRecord)) { - this._removeFromRemovals(newSeqRecord); - } - if (lastNewSeqRecord == null) { - this._mapHead = newSeqRecord; - } else { - lastNewSeqRecord._next = newSeqRecord; + // Items remaining at the end of the list have been deleted + if (insertBefore) { + if (insertBefore._prev) { + insertBefore._prev._next = null; + } + + this._removalsHead = insertBefore; + this._removalsTail = insertBefore; + + for (let record = insertBefore; record !== null; record = record._nextRemoved) { + if (record === this._mapHead) { + this._mapHead = null; } + this._records.delete(record.key); + record._nextRemoved = record._next; + record.previousValue = record.currentValue; + record.currentValue = null; + record._prev = null; + record._next = null; } - lastOldSeqRecord = oldSeqRecord; - lastNewSeqRecord = newSeqRecord; - oldSeqRecord = oldSeqRecord && oldSeqRecord._next; - }); - this._truncate(lastOldSeqRecord, oldSeqRecord); + } + return this.isDirty; } + /** + * Inserts a record before `before` or append at the end of the list when `before` is null. + * + * Notes: + * - This method appends at `this._appendAfter`, + * - This method updates `this._appendAfter`, + * - The return value is the new value for the insertion pointer. + */ + private _insertBeforeOrAppend( + before: KeyValueChangeRecord_, + record: KeyValueChangeRecord_): KeyValueChangeRecord_ { + if (before) { + const prev = before._prev; + record._next = before; + record._prev = prev; + before._prev = record; + if (prev) { + prev._next = record; + } + if (before === this._mapHead) { + this._mapHead = record; + } + + this._appendAfter = before; + return before; + } + + if (this._appendAfter) { + this._appendAfter._next = record; + record._prev = this._appendAfter; + } else { + this._mapHead = record; + } + + this._appendAfter = record; + return null; + } + + private _getOrCreateRecordForKey(key: K, value: V): KeyValueChangeRecord_ { + if (this._records.has(key)) { + const record = this._records.get(key); + this._maybeAddToChanges(record, value); + const prev = record._prev; + const next = record._next; + if (prev) { + prev._next = next; + } + if (next) { + next._prev = prev; + } + record._next = null; + record._prev = null; + + return record; + } + + const record = new KeyValueChangeRecord_(key); + this._records.set(key, record); + record.currentValue = value; + this._addToAdditions(record); + return record; + } + /** @internal */ _reset() { if (this.isDirty) { let record: KeyValueChangeRecord_; - // Record the state of the mapping - for (record = this._previousMapHead = this._mapHead; record !== null; record = record._next) { + // let `_previousMapHead` contain the state of the map before the changes + this._previousMapHead = this._mapHead; + for (record = this._previousMapHead; record !== null; record = record._next) { record._nextPrevious = record._next; } + // Update `record.previousValue` with the value of the item before the changes + // We need to update all changed items (that's those which have been added and changed) for (record = this._changesHead; record !== null; record = record._nextChanged) { record.previousValue = record.currentValue; } - for (record = this._additionsHead; record != null; record = record._nextAdded) { record.previousValue = record.currentValue; } @@ -161,27 +231,7 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer, KeyVal } } - private _truncate(lastRecord: KeyValueChangeRecord_, record: KeyValueChangeRecord_) { - while (record !== null) { - if (lastRecord === null) { - this._mapHead = null; - } else { - lastRecord._next = null; - } - const nextRecord = record._next; - this._addToRemovals(record); - lastRecord = record; - record = nextRecord; - } - - for (let rec: KeyValueChangeRecord_ = this._removalsHead; rec !== null; - rec = rec._nextRemoved) { - rec.previousValue = rec.currentValue; - rec.currentValue = null; - this._records.delete(rec.key); - } - } - + // Add the record or a given key to the list of changes only when the value has actually changed private _maybeAddToChanges(record: KeyValueChangeRecord_, newValue: any): void { if (!looseIdentical(newValue, record.currentValue)) { record.previousValue = record.currentValue; @@ -190,47 +240,6 @@ export class DefaultKeyValueDiffer implements KeyValueDiffer, KeyVal } } - private _isInRemovals(record: KeyValueChangeRecord_) { - return record === this._removalsHead || record._nextRemoved !== null || - record._prevRemoved !== null; - } - - private _addToRemovals(record: KeyValueChangeRecord_) { - if (this._removalsHead === null) { - this._removalsHead = this._removalsTail = record; - } else { - this._removalsTail._nextRemoved = record; - record._prevRemoved = this._removalsTail; - this._removalsTail = record; - } - } - - private _removeFromSeq(prev: KeyValueChangeRecord_, record: KeyValueChangeRecord_) { - const next = record._next; - if (prev === null) { - this._mapHead = next; - } else { - prev._next = next; - } - record._next = null; - } - - private _removeFromRemovals(record: KeyValueChangeRecord_) { - const prev = record._prevRemoved; - const next = record._nextRemoved; - if (prev === null) { - this._removalsHead = next; - } else { - prev._nextRemoved = next; - } - if (next === null) { - this._removalsTail = prev; - } else { - next._prevRemoved = prev; - } - record._prevRemoved = record._nextRemoved = null; - } - private _addToAdditions(record: KeyValueChangeRecord_) { if (this._additionsHead === null) { this._additionsHead = this._additionsTail = record; @@ -303,12 +312,12 @@ class KeyValueChangeRecord_ implements KeyValueChangeRecord { /** @internal */ _next: KeyValueChangeRecord_ = null; /** @internal */ + _prev: KeyValueChangeRecord_ = null; + /** @internal */ _nextAdded: KeyValueChangeRecord_ = null; /** @internal */ _nextRemoved: KeyValueChangeRecord_ = null; /** @internal */ - _prevRemoved: KeyValueChangeRecord_ = null; - /** @internal */ _nextChanged: KeyValueChangeRecord_ = null; constructor(public key: K) {} diff --git a/packages/core/test/change_detection/differs/default_keyvalue_differ_spec.ts b/packages/core/test/change_detection/differs/default_keyvalue_differ_spec.ts index 7098a8255cfd8..8983870ec162a 100644 --- a/packages/core/test/change_detection/differs/default_keyvalue_differ_spec.ts +++ b/packages/core/test/change_detection/differs/default_keyvalue_differ_spec.ts @@ -7,7 +7,6 @@ */ import {DefaultKeyValueDiffer, DefaultKeyValueDifferFactory} from '@angular/core/src/change_detection/differs/default_keyvalue_differ'; -import {afterEach, beforeEach, describe, expect, it} from '@angular/core/testing/src/testing_internal'; import {kvChangesAsString} from '../../change_detection/util'; // todo(vicb): Update the code & tests for object equality @@ -55,21 +54,17 @@ export function main() { }); it('should expose previous and current value', () => { - let previous: any /** TODO #9100 */, current: any /** TODO #9100 */; - m.set(1, 10); differ.check(m); m.set(1, 20); differ.check(m); - differ.forEachChangedItem((record: any /** TODO #9100 */) => { - previous = record.previousValue; - current = record.currentValue; + differ.forEachChangedItem((record: any) => { + expect(record.previousValue).toEqual(10); + expect(record.currentValue).toEqual(20); }); - expect(previous).toEqual(10); - expect(current).toEqual(20); }); it('should do basic map watching', () => { @@ -198,6 +193,19 @@ export function main() { changes: ['b[0->1]', 'a[0->1]'] })); }); + + it('should when the first item is moved', () => { + differ.check({a: 'a', b: 'b'}); + differ.check({c: 'c', a: 'a'}); + + expect(differ.toString()).toEqual(kvChangesAsString({ + map: ['c[null->c]', 'a'], + previous: ['a', 'b[b->null]'], + additions: ['c[null->c]'], + removals: ['b[b->null]'] + })); + }); + }); describe('diff', () => {