From 0f10624b08ec7924475772348147e34c940b876c Mon Sep 17 00:00:00 2001
From: Kara Erickson <karakara@google.com>
Date: Fri, 5 Feb 2016 16:32:24 -0800
Subject: [PATCH] fix(ngFor): update view locals if identity changes

Closes #6923
---
 .../angular2/src/common/directives/ng_for.ts  |  5 ++
 .../differs/default_iterable_differ.ts        | 57 ++++++++++++--
 .../test/common/directives/ng_for_spec.ts     | 76 ++++++++++++++-----
 .../differs/default_iterable_differ_spec.ts   | 35 ++++++++-
 .../test/core/change_detection/util.ts        |  9 ++-
 modules/angular2/test/public_api_spec.ts      |  1 +
 6 files changed, 146 insertions(+), 37 deletions(-)

diff --git a/modules/angular2/src/common/directives/ng_for.ts b/modules/angular2/src/common/directives/ng_for.ts
index ea63804d34..668d58b167 100644
--- a/modules/angular2/src/common/directives/ng_for.ts
+++ b/modules/angular2/src/common/directives/ng_for.ts
@@ -117,6 +117,11 @@ export class NgFor implements DoCheck {
       var viewRef = <EmbeddedViewRef>this._viewContainer.get(i);
       viewRef.setLocal('last', i === ilen - 1);
     }
+
+    changes.forEachIdentityChange((record) => {
+      var viewRef = <EmbeddedViewRef>this._viewContainer.get(record.currentIndex);
+      viewRef.setLocal('\$implicit', record.item);
+    });
   }
 
   private _perViewChange(view, record) {
diff --git a/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts b/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts
index bf8e715f35..ae0de4e0b8 100644
--- a/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts
+++ b/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts
@@ -40,6 +40,9 @@ export class DefaultIterableDiffer implements IterableDiffer {
   private _movesTail: CollectionChangeRecord = null;
   private _removalsHead: CollectionChangeRecord = null;
   private _removalsTail: CollectionChangeRecord = null;
+  // Keeps track of records where custom track by is the same, but item identity has changed
+  private _identityChangesHead: CollectionChangeRecord = null;
+  private _identityChangesTail: CollectionChangeRecord = null;
 
   constructor(private _trackByFn?: TrackByFn) {
     this._trackByFn = isPresent(this._trackByFn) ? this._trackByFn : trackByIdentity;
@@ -84,6 +87,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
     }
   }
 
+  forEachIdentityChange(fn: Function) {
+    var record: CollectionChangeRecord;
+    for (record = this._identityChangesHead; record !== null; record = record._nextIdentityChange) {
+      fn(record);
+    }
+  }
+
   diff(collection: any): DefaultIterableDiffer {
     if (isBlank(collection)) collection = [];
     if (!isListLikeIterable(collection)) {
@@ -123,7 +133,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
             // TODO(misko): can we limit this to duplicates only?
             record = this._verifyReinsertion(record, item, itemTrackBy, index);
           }
-          record.item = item;
+          if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
         }
 
         record = record._next;
@@ -135,9 +145,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
         if (record === null || !looseIdentical(record.trackById, itemTrackBy)) {
           record = this._mismatch(record, item, itemTrackBy, index);
           mayBeDirty = true;
-        } else if (mayBeDirty) {
-          // TODO(misko): can we limit this to duplicates only?
-          record = this._verifyReinsertion(record, item, itemTrackBy, index);
+        } else {
+          if (mayBeDirty) {
+            // TODO(misko): can we limit this to duplicates only?
+            record = this._verifyReinsertion(record, item, itemTrackBy, index);
+          }
+          if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
         }
         record = record._next;
         index++;
@@ -150,9 +163,12 @@ export class DefaultIterableDiffer implements IterableDiffer {
     return this.isDirty;
   }
 
-  // CollectionChanges is considered dirty if it has any additions, moves or removals.
+  /* CollectionChanges is considered dirty if it has any additions, moves, removals, or identity
+   * changes.
+   */
   get isDirty(): boolean {
-    return this._additionsHead !== null || this._movesHead !== null || this._removalsHead !== null;
+    return this._additionsHead !== null || this._movesHead !== null ||
+           this._removalsHead !== null || this._identityChangesHead !== null;
   }
 
   /**
@@ -183,6 +199,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
       }
       this._movesHead = this._movesTail = null;
       this._removalsHead = this._removalsTail = null;
+      this._identityChangesHead = this._identityChangesTail = null;
 
       // todo(vicb) when assert gets supported
       // assert(!this.isDirty);
@@ -216,12 +233,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
     record = this._linkedRecords === null ? null : this._linkedRecords.get(itemTrackBy, index);
     if (record !== null) {
       // We have seen this before, we need to move it forward in the collection.
+      // But first we need to check if identity changed, so we can update in view if necessary
+      if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
+
       this._moveAfter(record, previousRecord, index);
     } else {
       // Never seen it, check evicted list.
       record = this._unlinkedRecords === null ? null : this._unlinkedRecords.get(itemTrackBy);
       if (record !== null) {
         // It is an item which we have evicted earlier: reinsert it back into the list.
+        // But first we need to check if identity changed, so we can update in view if necessary
+        if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
+
         this._reinsertAfter(record, previousRecord, index);
       } else {
         // It is a new item: add it.
@@ -269,7 +292,6 @@ export class DefaultIterableDiffer implements IterableDiffer {
       record.currentIndex = index;
       this._addToMoves(record, index);
     }
-    record.item = item;
     return record;
   }
 
@@ -469,6 +491,18 @@ export class DefaultIterableDiffer implements IterableDiffer {
     return record;
   }
 
+  /** @internal */
+  _addIdentityChange(record: CollectionChangeRecord, item: any) {
+    record.item = item;
+    if (this._identityChangesTail === null) {
+      this._identityChangesTail = this._identityChangesHead = record;
+    } else {
+      this._identityChangesTail = this._identityChangesTail._nextIdentityChange = record;
+    }
+    return record;
+  }
+
+
   toString(): string {
     var list = [];
     this.forEachItem((record) => list.push(record));
@@ -485,9 +519,13 @@ export class DefaultIterableDiffer implements IterableDiffer {
     var removals = [];
     this.forEachRemovedItem((record) => removals.push(record));
 
+    var identityChanges = [];
+    this.forEachIdentityChange((record) => identityChanges.push(record));
+
     return "collection: " + list.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
            "additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
-           "removals: " + removals.join(', ') + "\n";
+           "removals: " + removals.join(', ') + "\n" + "identityChanges: " +
+           identityChanges.join(', ') + "\n";
   }
 }
 
@@ -513,6 +551,9 @@ export class CollectionChangeRecord {
   _nextAdded: CollectionChangeRecord = null;
   /** @internal */
   _nextMoved: CollectionChangeRecord = null;
+  /** @internal */
+  _nextIdentityChange: CollectionChangeRecord = null;
+
 
   constructor(public item: any, public trackById: any) {}
 
diff --git a/modules/angular2/test/common/directives/ng_for_spec.ts b/modules/angular2/test/common/directives/ng_for_spec.ts
index a56ae0296c..32b7c2071f 100644
--- a/modules/angular2/test/common/directives/ng_for_spec.ts
+++ b/modules/angular2/test/common/directives/ng_for_spec.ts
@@ -361,30 +361,64 @@ export function main() {
              });
        }));
 
-    it('should use custom track by if function is provided',
-       inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
-         var template =
-             `<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
+    describe('track by', function() {
+      it('should not replace tracked items',
+         inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
+           var template =
+               `<template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy" #i="index">
                <p>{{items[i]}}</p>
               </template>`;
-         tcb.overrideTemplate(TestComponent, template)
-             .createAsync(TestComponent)
-             .then((fixture) => {
-               var buildItemList =
-                   () => {
-                     fixture.debugElement.componentInstance.items = [{'id': 'a'}];
-                     fixture.detectChanges();
-                     return fixture.debugElement.queryAll(By.css('p'))[0];
-                   }
-
-               var firstP = buildItemList();
-               var finalP = buildItemList();
-               expect(finalP.nativeElement).toBe(firstP.nativeElement);
-               async.done();
-             });
-       }));
-
+           tcb.overrideTemplate(TestComponent, template)
+               .createAsync(TestComponent)
+               .then((fixture) => {
+                 var buildItemList =
+                     () => {
+                       fixture.debugElement.componentInstance.items = [{'id': 'a'}];
+                       fixture.detectChanges();
+                       return fixture.debugElement.queryAll(By.css('p'))[0];
+                     }
 
+                 var firstP = buildItemList();
+                 var finalP = buildItemList();
+                 expect(finalP.nativeElement).toBe(firstP.nativeElement);
+                 async.done();
+               });
+         }));
+      it('should update implicit local variable on view',
+         inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
+           var template =
+               `<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
+           tcb.overrideTemplate(TestComponent, template)
+               .createAsync(TestComponent)
+               .then((fixture) => {
+                 fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'blue'}];
+                 fixture.detectChanges();
+                 expect(fixture.debugElement.nativeElement).toHaveText('blue');
+                 fixture.debugElement.componentInstance.items = [{'id': 'a', 'color': 'red'}];
+                 fixture.detectChanges();
+                 expect(fixture.debugElement.nativeElement).toHaveText('red');
+                 async.done();
+               });
+         }));
+      it('should move items around and keep them updated ',
+         inject([TestComponentBuilder, AsyncTestCompleter], (tcb: TestComponentBuilder, async) => {
+           var template =
+               `<div><template ngFor #item [ngForOf]="items" [ngForTrackBy]="customTrackBy">{{item['color']}}</template></div>`;
+           tcb.overrideTemplate(TestComponent, template)
+               .createAsync(TestComponent)
+               .then((fixture) => {
+                 fixture.debugElement.componentInstance.items =
+                     [{'id': 'a', 'color': 'blue'}, {'id': 'b', 'color': 'yellow'}];
+                 fixture.detectChanges();
+                 expect(fixture.debugElement.nativeElement).toHaveText('blueyellow');
+                 fixture.debugElement.componentInstance.items =
+                     [{'id': 'b', 'color': 'orange'}, {'id': 'a', 'color': 'red'}];
+                 fixture.detectChanges();
+                 expect(fixture.debugElement.nativeElement).toHaveText('orangered');
+                 async.done();
+               });
+         }));
+    });
   });
 }
 
diff --git a/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts b/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts
index 8962362ab3..a91b18eae4 100644
--- a/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts
+++ b/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts
@@ -25,6 +25,12 @@ class ItemWithId {
   toString() { return `{id: ${this.id}}` }
 }
 
+class ComplexItem {
+  constructor(private id: string, private color: string) {}
+
+  toString() { return `{id: ${this.id}, color: ${this.color}}` }
+}
+
 // todo(vicb): UnmodifiableListView / frozen object when implemented
 export function main() {
   describe('iterable differ', function() {
@@ -342,8 +348,12 @@ export function main() {
 
       beforeEach(() => { differ = new DefaultIterableDiffer(trackByItemId); });
 
-      it('should not treat maps as new with track by function', () => {
+      it('should treat the collection as dirty if identity changes', () => {
+        differ.diff(buildItemList(['a']));
+        expect(differ.diff(buildItemList(['a']))).toBe(differ);
+      });
 
+      it('should treat seen records as identity changes, not additions', () => {
         let l = buildItemList(['a', 'b', 'c']);
         differ.check(l);
         expect(differ.toString())
@@ -357,11 +367,26 @@ export function main() {
         expect(differ.toString())
             .toEqual(iterableChangesAsString({
               collection: [`{id: a}`, `{id: b}`, `{id: c}`],
+              identityChanges: [`{id: a}`, `{id: b}`, `{id: c}`],
               previous: [`{id: a}`, `{id: b}`, `{id: c}`]
             }));
       });
 
-      it('should track moves normally with track by function', () => {
+      it('should have updated properties in identity change collection', () => {
+        let l = [new ComplexItem('a', 'blue'), new ComplexItem('b', 'yellow')];
+        differ.check(l);
+
+        l = [new ComplexItem('a', 'orange'), new ComplexItem('b', 'red')];
+        differ.check(l);
+        expect(differ.toString())
+            .toEqual(iterableChangesAsString({
+              collection: [`{id: a, color: orange}`, `{id: b, color: red}`],
+              identityChanges: [`{id: a, color: orange}`, `{id: b, color: red}`],
+              previous: [`{id: a, color: orange}`, `{id: b, color: red}`]
+            }));
+      });
+
+      it('should track moves normally', () => {
         let l = buildItemList(['a', 'b', 'c']);
         differ.check(l);
 
@@ -370,13 +395,14 @@ export function main() {
         expect(differ.toString())
             .toEqual(iterableChangesAsString({
               collection: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
+              identityChanges: ['{id: b}[1->0]', '{id: a}[0->1]', '{id: c}'],
               previous: ['{id: a}[0->1]', '{id: b}[1->0]', '{id: c}'],
               moves: ['{id: b}[1->0]', '{id: a}[0->1]']
             }));
 
       });
 
-      it('should track duplicate reinsertion normally with track by function', () => {
+      it('should track duplicate reinsertion normally', () => {
         let l = buildItemList(['a', 'a']);
         differ.check(l);
 
@@ -385,6 +411,7 @@ export function main() {
         expect(differ.toString())
             .toEqual(iterableChangesAsString({
               collection: ['{id: b}[null->0]', '{id: a}[0->1]', '{id: a}[1->2]'],
+              identityChanges: ['{id: a}[0->1]', '{id: a}[1->2]'],
               previous: ['{id: a}[0->1]', '{id: a}[1->2]'],
               moves: ['{id: a}[0->1]', '{id: a}[1->2]'],
               additions: ['{id: b}[null->0]']
@@ -392,7 +419,7 @@ export function main() {
 
       });
 
-      it('should track removals normally with track by function', () => {
+      it('should track removals normally', () => {
         let l = buildItemList(['a', 'b', 'c']);
         differ.check(l);
 
diff --git a/modules/angular2/test/core/change_detection/util.ts b/modules/angular2/test/core/change_detection/util.ts
index 0f4ab88b83..4b86f73968 100644
--- a/modules/angular2/test/core/change_detection/util.ts
+++ b/modules/angular2/test/core/change_detection/util.ts
@@ -1,11 +1,12 @@
 import {isBlank, CONST_EXPR} from 'angular2/src/facade/lang';
 
-export function iterableChangesAsString({collection = CONST_EXPR([]), previous = CONST_EXPR([]),
-                                         additions = CONST_EXPR([]), moves = CONST_EXPR([]),
-                                         removals = CONST_EXPR([])}) {
+export function iterableChangesAsString(
+    {collection = CONST_EXPR([]), previous = CONST_EXPR([]), additions = CONST_EXPR([]),
+     moves = CONST_EXPR([]), removals = CONST_EXPR([]), identityChanges = CONST_EXPR([])}) {
   return "collection: " + collection.join(', ') + "\n" + "previous: " + previous.join(', ') + "\n" +
          "additions: " + additions.join(', ') + "\n" + "moves: " + moves.join(', ') + "\n" +
-         "removals: " + removals.join(', ') + "\n";
+         "removals: " + removals.join(', ') + "\n" + "identityChanges: " +
+         identityChanges.join(', ') + "\n";
 }
 
 export function kvChangesAsString(
diff --git a/modules/angular2/test/public_api_spec.ts b/modules/angular2/test/public_api_spec.ts
index 9f082bb2b7..61051abdeb 100644
--- a/modules/angular2/test/public_api_spec.ts
+++ b/modules/angular2/test/public_api_spec.ts
@@ -419,6 +419,7 @@ var NG_COMMON = [
   'ObservableListDiff.forEachMovedItem():dart',
   'ObservableListDiff.forEachPreviousItem():dart',
   'ObservableListDiff.forEachRemovedItem():dart',
+  'ObservableListDiff.forEachIdentityChange():dart',
   'ObservableListDiff.isDirty:dart',
   'ObservableListDiff.length:dart',
   'ObservableListDiff.onDestroy():dart',