fix(ChangeDetector): fix issues with handling empty ranges

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);
+    _link(lastRecord, record);
-    _glueEnabled(lastRecord, record);
+    if (!lastRecord.disabled) {
-    _glue(record, this.tailRecord);
+      _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);
+    _link(lastRecord, child.headRecord);
-    _glue(child.tailRecord, this.tailRecord);
+    _link(child.tailRecord, this.tailRecord);
 
-    if (isPresent(lastEnabledRecord) && isPresent(firstEnabledChildRecord)) {
+    if (isPresent(prevEnabledRecord) && isPresent(firstEnabledChildRecord)) {
-      _glueEnabled(lastEnabledRecord, 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;
+    if (isPresent(firstEnabledChildRecord)) {
-    var prevEnabled = firstEnabledChildRecord.prevEnabled;
+      var nextEnabled = lastEnabledChildRecord.nextEnabled;
-
+      var prevEnabled = firstEnabledChildRecord.prevEnabled;
-    if (isPresent(nextEnabled)) nextEnabled.prevEnabled = prevEnabled;
+      if (isPresent(nextEnabled)) nextEnabled.prevEnabled = prevEnabled;
-    if (isPresent(prevEnabled)) prevEnabled.nextEnabled = nextEnabled;
+      if (isPresent(prevEnabled)) prevEnabled.nextEnabled = nextEnabled;
-  }
+    }
-
-  findFirstEnabledRecord() {
-    return this._nextEnabledInCurrentRange(this.headRecord);
-  }
-
-  findLastEnabledRecord() {
-    return this._prevEnabledInCurrentRange(this.tailRecord);
   }
 
   disableRecord(record:Record) {
@@ -162,8 +164,8 @@ export class RecordRange {
   enableRecord(record:Record) {
     if (!record.disabled) return;
 
-    var prevEnabled = this._prevEnabledInCurrentRange(record);
+    var prevEnabled = this._prevEnabled(record);
-    var nextEnabled = this._nextEnabledInCurrentRange(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 prevEnabledRecord = this._prevEnabled(child.headRecord);
-    var nextEnabledRecord = this._nextEnabledInCurrentRange(child.tailRecord);
+    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) {
+   * Returns the first enabled record in the current range.
-    if (record === this.tailRecord) return null;
+   *
-
+   * [H ER1 ER2 R3 T] returns ER1
-    record = record.next;
+   * [H R1 ER2 R3 T] returns ER2
-    while (isPresent(record) && record !== this.tailRecord && record.disabled) {
+   *
+   * 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) {
+   * Returns the last enabled record in the current range.
-    if (record === this.headRecord) return null;
+   *
-
+   * [H ER1 ER2 R3 T] returns ER2
-    record = record.prev;
+   * [H R1 ER2 R3 T] returns ER2
-    while (isPresent(record) && record !== this.headRecord && record.disabled) {
+   *
+   * 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;
 }

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 = (names, item) =>
-    var lookupName = (item) =>
+    ListWrapper.last(
-      ListWrapper.last(
+      ListWrapper.find(names, (pair) => pair[0] === item));
-        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'
         ]);
       });