From 7f941eb936f5038cde4324b7ea98ccb709ef0dd4 Mon Sep 17 00:00:00 2001 From: vsavkin Date: Thu, 4 Dec 2014 13:14:44 -0800 Subject: [PATCH] fix(change_detector): adding new ranges when disabling the current enabled record --- .../change_detection/src/change_detector.js | 2 +- modules/change_detection/src/record.js | 47 +++++++++++++ modules/change_detection/src/record_range.js | 52 ++------------ .../test/change_detector_spec.js | 68 +++++++++++++++---- 4 files changed, 109 insertions(+), 60 deletions(-) diff --git a/modules/change_detection/src/change_detector.js b/modules/change_detection/src/change_detector.js index 7f2fa273e1..058defcdca 100644 --- a/modules/change_detection/src/change_detector.js +++ b/modules/change_detection/src/change_detector.js @@ -40,7 +40,7 @@ export class ChangeDetector { } } - record = record.nextEnabled; + record = record.findNextEnabled(); } return count; diff --git a/modules/change_detection/src/record.js b/modules/change_detection/src/record.js index 41234be6b1..89723affb0 100644 --- a/modules/change_detection/src/record.js +++ b/modules/change_detection/src/record.js @@ -173,6 +173,10 @@ export class Record { return (this._mode & RECORD_FLAG_DISABLED) === RECORD_FLAG_DISABLED; } + isEnabled():boolean { + return ! this.disabled; + } + set disabled(value:boolean) { if (value) { this._mode |= RECORD_FLAG_DISABLED; @@ -316,6 +320,49 @@ export class Record { groupMemento() { return isPresent(this.protoRecord) ? this.protoRecord.groupMemento : null; } + + + /** + * 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. + */ + findNextEnabled() { + if (this.isEnabled()) return this.nextEnabled; + + var record = this.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. + */ + findPrevEnabled() { + if (this.isEnabled()) return this.prevEnabled; + + var record = this.prev; + while (isPresent(record) && record.disabled) { + if (record.isMarkerRecord && record.recordRange.disabled) { + record = record.recordRange.headRecord.prev; + } else { + record = record.prev; + } + } + return record; + } } function isSame(a, b) { diff --git a/modules/change_detection/src/record_range.js b/modules/change_detection/src/record_range.js index 3e306ddc1a..6617f0ddb4 100644 --- a/modules/change_detection/src/record_range.js +++ b/modules/change_detection/src/record_range.js @@ -129,8 +129,8 @@ export class RecordRange { addRange(child:RecordRange) { var lastRecord = this.tailRecord.prev; - var prevEnabledRecord = RecordRange._prevEnabled(this.tailRecord); - var nextEnabledRerord = RecordRange._nextEnabled(this.tailRecord); + var prevEnabledRecord = this.tailRecord.findPrevEnabled(); + var nextEnabledRerord = this.tailRecord.findNextEnabled(); var firstEnabledChildRecord = child.findFirstEnabledRecord(); var lastEnabledChildRecord = child.findLastEnabledRecord(); @@ -174,10 +174,10 @@ export class RecordRange { } enableRecord(record:Record) { - if (!record.disabled) return; + if (record.isEnabled()) return; - var prevEnabled = RecordRange._prevEnabled(record); - var nextEnabled = RecordRange._nextEnabled(record); + var prevEnabled = record.findPrevEnabled(); + var nextEnabled = record.findNextEnabled(); record.prevEnabled = prevEnabled; record.nextEnabled = nextEnabled; @@ -203,8 +203,8 @@ export class RecordRange { } enable() { - var prevEnabledRecord = RecordRange._prevEnabled(this.headRecord); - var nextEnabledRecord = RecordRange._nextEnabled(this.tailRecord); + var prevEnabledRecord = this.headRecord.findPrevEnabled(); + var nextEnabledRecord = this.tailRecord.findNextEnabled(); var firstEnabledthisRecord = this.findFirstEnabledRecord(); var lastEnabledthisRecord = this.findLastEnabledRecord(); @@ -268,44 +268,6 @@ 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. - */ - static _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. - */ - static _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 diff --git a/modules/change_detection/test/change_detector_spec.js b/modules/change_detection/test/change_detector_spec.js index 8f5a18eb88..8f2e976507 100644 --- a/modules/change_detection/test/change_detector_spec.js +++ b/modules/change_detection/test/change_detector_spec.js @@ -1,4 +1,4 @@ -import {ddescribe, describe, it, iit, xit, expect} from 'test_lib/test_lib'; +import {ddescribe, describe, it, iit, xit, expect, beforeEach} from 'test_lib/test_lib'; import {isPresent, isBlank, isJsObject} from 'facade/lang'; import {List, ListWrapper, MapWrapper} from 'facade/collection'; @@ -28,7 +28,7 @@ export function main() { var prr = new ProtoRecordRange(); prr.addRecordsFromAST(ast(exp), memo, memo, content); - var dispatcher = new LoggingDispatcher(); + var dispatcher = new TestDispatcher(); var rr = prr.instantiate(dispatcher, formatters); rr.setContext(context); @@ -46,6 +46,17 @@ export function main() { describe('change_detection', () => { describe('ChangeDetection', () => { + function createRange(dispatcher, ast, group) { + var prr = new ProtoRecordRange(); + prr.addRecordsFromAST(ast, "memo", group); + return prr.instantiate(dispatcher, null); + } + + function detectChangesInRange(recordRange) { + var cd = new ChangeDetector(recordRange); + cd.detectChanges(); + } + it('should do simple watching', () => { var person = new Person("misko"); @@ -345,18 +356,45 @@ export function main() { } }); - describe("onGroupChange", () => { + describe("adding new ranges", () => { + var dispatcher; + + beforeEach(() => { + dispatcher = new TestDispatcher(); + }); + + /** + * Tests that we can add a new range after the current + * record has been disabled. The new range must be processed + * during the same change detection run. + */ + it("should work when disabling the last enabled record", () => { + var rr = createRange(dispatcher, ast("1"), 1); + + dispatcher.onChange = (group, _) => { + if (group === 1) { // to prevent infinite loop + var rangeToAppend = createRange(dispatcher, ast("2"), 2); + rr.addRange(rangeToAppend); + } + }; + + detectChangesInRange(rr); + + expect(dispatcher.loggedValues).toEqual([[1], [2]]); + }); + }); + + describe("group changes", () => { it("should notify the dispatcher when a group of records changes", () => { var prr = new ProtoRecordRange(); prr.addRecordsFromAST(ast("1 + 2"), "memo", 1); prr.addRecordsFromAST(ast("10 + 20"), "memo", 1); prr.addRecordsFromAST(ast("100 + 200"), "memo2", 2); - var dispatcher = new LoggingDispatcher(); + var dispatcher = new TestDispatcher(); var rr = prr.instantiate(dispatcher, null); - var cd = new ChangeDetector(rr); - cd.detectChanges(); + detectChangesInRange(rr); expect(dispatcher.loggedValues).toEqual([[3, 30], [300]]); }); @@ -365,13 +403,12 @@ export function main() { var prr = new ProtoRecordRange(); prr.addRecordsFromAST(ast("1 + 2"), "memo", "memo"); - var dispatcher = new LoggingDispatcher(); + var dispatcher = new TestDispatcher(); var rr = new RecordRange(null, dispatcher); rr.addRange(prr.instantiate(dispatcher, null)); rr.addRange(prr.instantiate(dispatcher, null)); - var cd = new ChangeDetector(rr); - cd.detectChanges(); + detectChangesInRange(rr); expect(dispatcher.loggedValues).toEqual([[3], [3]]); }); @@ -382,7 +419,7 @@ export function main() { prr.addRecordsFromAST(ast("b()"), "b", 2); prr.addRecordsFromAST(ast("c()"), "b", 2); - var dispatcher = new LoggingDispatcher(); + var dispatcher = new TestDispatcher(); var rr = prr.instantiate(dispatcher, null); var tr = new TestRecord(); @@ -391,8 +428,7 @@ export function main() { tr.c = () => {dispatcher.logValue('InvokeC'); return 'c'}; rr.setContext(tr); - var cd = new ChangeDetector(rr); - cd.detectChanges(); + detectChangesInRange(rr); expect(dispatcher.loggedValues).toEqual(['InvokeA', ['a'], 'InvokeB', 'InvokeC', ['b', 'c']]); }); @@ -444,13 +480,15 @@ class TestData { } } -class LoggingDispatcher extends WatchGroupDispatcher { +class TestDispatcher extends WatchGroupDispatcher { log:List; loggedValues:List; + onChange:Function; constructor() { this.log = null; this.loggedValues = null; + this.onChange = (_, __) => {}; this.clear(); } @@ -458,7 +496,7 @@ class LoggingDispatcher extends WatchGroupDispatcher { this.log = ListWrapper.create(); this.loggedValues = ListWrapper.create(); } - + logValue(value) { ListWrapper.push(this.loggedValues, value); } @@ -470,6 +508,8 @@ class LoggingDispatcher extends WatchGroupDispatcher { var values = ListWrapper.map(records, (r) => r.currentValue); ListWrapper.push(this.loggedValues, values); + + this.onChange(group, records); } _asString(value) {