From 2b53a2f353d493dd6dc5c698e1ba2c1af5beaad4 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Sat, 22 Nov 2014 16:26:59 -0800 Subject: [PATCH] fix(ChangeDetector): fix issues with handling empty ranges --- modules/change_detection/src/record_range.js | 144 ++++++++++++------ .../test/record_range_spec.js | 106 ++++++++++--- 2 files changed, 188 insertions(+), 62 deletions(-) diff --git a/modules/change_detection/src/record_range.js b/modules/change_detection/src/record_range.js index d1f6f0429a..480947e78a 100644 --- a/modules/change_detection/src/record_range.js +++ b/modules/change_detection/src/record_range.js @@ -100,28 +100,37 @@ export class RecordRange { this.headRecord = Record.createMarker(this); this.tailRecord = Record.createMarker(this); - _glue(this.headRecord, this.tailRecord); + _link(this.headRecord, this.tailRecord); } /// addRecord assumes that the record is newly created, so it is enabled. addRecord(record:Record) { var lastRecord = this.tailRecord.prev; - _glue(lastRecord, record); - _glueEnabled(lastRecord, record); - _glue(record, this.tailRecord); + _link(lastRecord, record); + if (!lastRecord.disabled) { + _linkEnabled(lastRecord, record); + } + _link(record, this.tailRecord); } addRange(child:RecordRange) { var lastRecord = this.tailRecord.prev; - var lastEnabledRecord = this.findLastEnabledRecord(); + var prevEnabledRecord = this._prevEnabled(this.tailRecord); + var nextEnabledRerord = this._nextEnabled(this.tailRecord); + var firstEnabledChildRecord = child.findFirstEnabledRecord(); + var lastEnabledChildRecord = child.findLastEnabledRecord(); - _glue(lastRecord, child.headRecord); - _glue(child.tailRecord, this.tailRecord); + _link(lastRecord, child.headRecord); + _link(child.tailRecord, this.tailRecord); - if (isPresent(lastEnabledRecord) && isPresent(firstEnabledChildRecord)) { - _glueEnabled(lastEnabledRecord, firstEnabledChildRecord); + if (isPresent(prevEnabledRecord) && isPresent(firstEnabledChildRecord)) { + _linkEnabled(prevEnabledRecord, firstEnabledChildRecord); + } + + if (isPresent(nextEnabledRerord) && isPresent(lastEnabledChildRecord)) { + _linkEnabled(lastEnabledChildRecord, nextEnabledRerord); } } @@ -132,21 +141,14 @@ export class RecordRange { var next = child.tailRecord.next; var prev = child.headRecord.prev; - _glue(prev, next); + _link(prev, next); - var nextEnabled = lastEnabledChildRecord.nextEnabled; - var prevEnabled = firstEnabledChildRecord.prevEnabled; - - if (isPresent(nextEnabled)) nextEnabled.prevEnabled = prevEnabled; - if (isPresent(prevEnabled)) prevEnabled.nextEnabled = nextEnabled; - } - - findFirstEnabledRecord() { - return this._nextEnabledInCurrentRange(this.headRecord); - } - - findLastEnabledRecord() { - return this._prevEnabledInCurrentRange(this.tailRecord); + if (isPresent(firstEnabledChildRecord)) { + var nextEnabled = lastEnabledChildRecord.nextEnabled; + var prevEnabled = firstEnabledChildRecord.prevEnabled; + if (isPresent(nextEnabled)) nextEnabled.prevEnabled = prevEnabled; + if (isPresent(prevEnabled)) prevEnabled.nextEnabled = nextEnabled; + } } disableRecord(record:Record) { @@ -162,8 +164,8 @@ export class RecordRange { enableRecord(record:Record) { if (!record.disabled) return; - var prevEnabled = this._prevEnabledInCurrentRange(record); - var nextEnabled = this._nextEnabledInCurrentRange(record); + var prevEnabled = this._prevEnabled(record); + var nextEnabled = this._nextEnabled(record); record.prevEnabled = prevEnabled; record.nextEnabled = nextEnabled; @@ -188,29 +190,38 @@ export class RecordRange { } enableRange(child:RecordRange) { - var prevEnabledRecord = this._prevEnabledInCurrentRange(child.headRecord); - var nextEnabledRecord = this._nextEnabledInCurrentRange(child.tailRecord); + var prevEnabledRecord = this._prevEnabled(child.headRecord); + var nextEnabledRecord = this._nextEnabled(child.tailRecord); var firstEnabledChildRecord = child.findFirstEnabledRecord(); var lastEnabledChildRecord = child.findLastEnabledRecord(); if (isPresent(firstEnabledChildRecord) && isPresent(prevEnabledRecord)){ - _glueEnabled(prevEnabledRecord, firstEnabledChildRecord); + _linkEnabled(prevEnabledRecord, firstEnabledChildRecord); } if (isPresent(lastEnabledChildRecord) && isPresent(nextEnabledRecord)){ - _glueEnabled(lastEnabledChildRecord, nextEnabledRecord); + _linkEnabled(lastEnabledChildRecord, nextEnabledRecord); } child.disabled = false; } - /// Returns the next enabled record in the current range. If no such record, returns null. - _nextEnabledInCurrentRange(record:Record) { - if (record === this.tailRecord) return null; - - record = record.next; - while (isPresent(record) && record !== this.tailRecord && record.disabled) { + /** + * Returns the first enabled record in the current range. + * + * [H ER1 ER2 R3 T] returns ER1 + * [H R1 ER2 R3 T] returns ER2 + * + * If no enabled records, returns null. + * + * [H R1 R2 R3 T] returns null + * + * The function skips disabled sub ranges. + */ + findFirstEnabledRecord() { + var record = this.headRecord.next; + while (record !== this.tailRecord && record.disabled) { if (record.isMarkerRecord && record.recordRange.disabled) { record = record.recordRange.tailRecord.next; } else { @@ -220,12 +231,21 @@ export class RecordRange { return record === this.tailRecord ? null : record; } - /// Returns the prev enabled record in the current range. If no such record, returns null. - _prevEnabledInCurrentRange(record:Record) { - if (record === this.headRecord) return null; - - record = record.prev; - while (isPresent(record) && record !== this.headRecord && record.disabled) { + /** + * Returns the last enabled record in the current range. + * + * [H ER1 ER2 R3 T] returns ER2 + * [H R1 ER2 R3 T] returns ER2 + * + * If no enabled records, returns null. + * + * [H R1 R2 R3 T] returns null + * + * The function skips disabled sub ranges. + */ + findLastEnabledRecord() { + var record = this.tailRecord.prev; + while (record !== this.headRecord && record.disabled) { if (record.isMarkerRecord && record.recordRange.disabled) { record = record.recordRange.headRecord.prev; } else { @@ -235,6 +255,44 @@ export class RecordRange { return record === this.headRecord ? null : record; } + /** + * Returns the next enabled record. This search is not limited to the current range. + * + * [H ER1 T] [H ER2 T] _nextEnable(ER1) will return ER2 + * + * The function skips disabled sub ranges. + */ + _nextEnabled(record:Record) { + record = record.next; + while (isPresent(record) && record.disabled) { + if (record.isMarkerRecord && record.recordRange.disabled) { + record = record.recordRange.tailRecord.next; + } else { + record = record.next; + } + } + return record; + } + + /** + * Returns the prev enabled record. This search is not limited to the current range. + * + * [H ER1 T] [H ER2 T] _nextEnable(ER2) will return ER1 + * + * The function skips disabled sub ranges. + */ + _prevEnabled(record:Record) { + record = record.prev; + while (isPresent(record) && record.disabled) { + if (record.isMarkerRecord && record.recordRange.disabled) { + record = record.recordRange.headRecord.prev; + } else { + record = record.prev; + } + } + return record; + } + /** * Sets the context (the object) on which the change detection expressions will * dereference themselves on. Since the RecordRange can be reused the context @@ -253,12 +311,12 @@ export class RecordRange { } } -function _glue(a:Record, b:Record) { +function _link(a:Record, b:Record) { a.next = b; b.prev = a; } -function _glueEnabled(a:Record, b:Record) { +function _linkEnabled(a:Record, b:Record) { a.nextEnabled = b; b.prevEnabled = a; } diff --git a/modules/change_detection/test/record_range_spec.js b/modules/change_detection/test/record_range_spec.js index 1cbba70aa3..d8173188f3 100644 --- a/modules/change_detection/test/record_range_spec.js +++ b/modules/change_detection/test/record_range_spec.js @@ -16,17 +16,32 @@ import { import {Record} from 'change_detection/record'; export function main() { - function humanize(rr:RecordRange, names:List) { - var lookupName = (item) => - ListWrapper.last( - ListWrapper.find(names, (pair) => pair[0] === item)); + var lookupName = (names, item) => + ListWrapper.last( + ListWrapper.find(names, (pair) => pair[0] === item)); + function enabledRecordsInReverseOrder(rr:RecordRange, names:List) { + var reversed = []; + var record = rr.findLastEnabledRecord(); + while (isPresent(record)) { + ListWrapper.push(reversed, lookupName(names, record)); + record = record.prevEnabled; + } + return reversed; + } + + function enabledRecords(rr:RecordRange, names:List) { var res = []; var record = rr.findFirstEnabledRecord(); while (isPresent(record)) { - ListWrapper.push(res, lookupName(record)); + ListWrapper.push(res, lookupName(names, record)); record = record.nextEnabled; } + + // check that all links are set properly in both directions + var reversed = enabledRecordsInReverseOrder(rr, names); + expect(res).toEqual(ListWrapper.reversed(reversed)); + return res; } @@ -43,7 +58,7 @@ export function main() { rr.addRecord(record1); rr.addRecord(record2); - expect(humanize(rr, [ + expect(enabledRecords(rr, [ [record1, 'record1'], [record2, 'record2'] ])).toEqual(['record1', 'record2']); @@ -80,14 +95,33 @@ export function main() { parent.addRange(child1); parent.addRange(child2); - expect(humanize(parent, recordNames)).toEqual(['record1', 'record2']); + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record2']); + }); + + it('should handle adding an empty range', () => { + var emptyRange = new RecordRange(null, null); + parent.addRange(child1); + parent.addRange(child2); + child1.addRange(emptyRange); + + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record2']); + }); + + it('should handle adding a range into an empty range', () => { + var emptyRange = new RecordRange(null, null); + parent.addRange(emptyRange); + parent.addRange(child2); + + emptyRange.addRange(child1); + + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record2']); }); it('should add nested record ranges', () => { parent.addRange(child1); child1.addRange(child2); - expect(humanize(parent, recordNames)).toEqual(['record1', 'record2']); + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record2']); }); it('should remove record ranges', () => { @@ -96,11 +130,22 @@ export function main() { parent.removeRange(child1); - expect(humanize(parent, recordNames)).toEqual(['record2']); + expect(enabledRecords(parent, recordNames)).toEqual(['record2']); parent.removeRange(child2); - expect(humanize(parent, recordNames)).toEqual([]); + expect(enabledRecords(parent, recordNames)).toEqual([]); + }); + + it('should remove an empty record range', () => { + var emptyRange = new RecordRange(null, null); + parent.addRange(child1); + parent.addRange(emptyRange); + parent.addRange(child2); + + parent.removeRange(emptyRange); + + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record2']); }); it('should remove a record range surrounded by other ranges', () => { @@ -110,7 +155,7 @@ export function main() { parent.removeRange(child2); - expect(humanize(parent, recordNames)).toEqual(['record1', 'record3']); + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record3']); }); }); @@ -139,7 +184,7 @@ export function main() { rr.disableRecord(record1); - expect(humanize(rr, recordNames)).toEqual([]); + expect(enabledRecords(rr, recordNames)).toEqual([]); }); it('should enable a single record', () => { @@ -148,7 +193,7 @@ export function main() { rr.enableRecord(record1); - expect(humanize(rr, recordNames)).toEqual(['record1']); + expect(enabledRecords(rr, recordNames)).toEqual(['record1']); }); it('should disable a record', () => { @@ -163,7 +208,7 @@ export function main() { expect(record2.disabled).toBeTruthy(); expect(record3.disabled).toBeTruthy(); - expect(humanize(rr, recordNames)).toEqual(['record1', 'record4']); + expect(enabledRecords(rr, recordNames)).toEqual(['record1', 'record4']); }); it('should enable a record', () => { @@ -177,7 +222,30 @@ export function main() { rr.enableRecord(record2); rr.enableRecord(record3); - expect(humanize(rr, recordNames)).toEqual(['record1', 'record2', 'record3', 'record4']); + expect(enabledRecords(rr, recordNames)).toEqual(['record1', 'record2', 'record3', 'record4']); + }); + + it('should disable a single record in a range', () => { + var rr1 = new RecordRange(null, null); + rr1.addRecord(record1); + + var rr2 = new RecordRange(null, null); + rr2.addRecord(record2); + + var rr3 = new RecordRange(null, null); + rr3.addRecord(record3); + + rr.addRange(rr1); + rr.addRange(rr2); + rr.addRange(rr3); + + rr2.disableRecord(record2); + + expect(enabledRecords(rr, recordNames)).toEqual(['record1', 'record3']); + + rr2.enableRecord(record2); + + expect(enabledRecords(rr, recordNames)).toEqual(['record1', 'record2', 'record3']); }); }); @@ -217,7 +285,7 @@ export function main() { parent.disableRange(child1); - expect(humanize(parent, recordNames)).toEqual([]); + expect(enabledRecords(parent, recordNames)).toEqual([]); }); it('should enable a single record range', () => { @@ -227,7 +295,7 @@ export function main() { parent.enableRange(child1); - expect(humanize(parent, recordNames)).toEqual(['record1']); + expect(enabledRecords(parent, recordNames)).toEqual(['record1']); }); it('should disable a record range', () => { @@ -240,7 +308,7 @@ export function main() { parent.disableRange(child2); parent.disableRange(child3); - expect(humanize(parent, recordNames)).toEqual(['record1', 'record4']); + expect(enabledRecords(parent, recordNames)).toEqual(['record1', 'record4']); }); it('should enable a record range', () => { @@ -255,7 +323,7 @@ export function main() { parent.enableRange(child2); parent.enableRange(child3); - expect(humanize(parent, recordNames)).toEqual([ + expect(enabledRecords(parent, recordNames)).toEqual([ 'record1', 'record2', 'record3', 'record4' ]); });