[mdv] Avoid observing placeholder arrays in TemplateIterator

pkg/mdv/lib/src/template_iterator.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of mdv;
 
 // This code is a port of Model-Driven-Views:
 // https://github.com/polymer-project/mdv
 // The code mostly comes from src/template_element.js
 
@@ skipping 161 matching lines @@
   final bool isBinding;
 
   _BindingToken(this.value, {this.isBinding: false});
 
   bool get isText => !isBinding;
 }
 
 class _TemplateIterator {
   final Element _templateElement;
   final List<Node> terminators = [];
-  final CompoundBinding inputs;
+  CompoundBinding inputs;
   List iteratedValue;
-  Object _lastValue;
+  bool closed = false;
 
   StreamSubscription _sub;
-  StreamSubscription _valueBinding;
 
-  _TemplateIterator(this._templateElement)
-      : inputs = new CompoundBinding(resolveInputs) {
-
-    _valueBinding = new PathObserver(inputs, 'value').bindSync(valueChanged);
+  _TemplateIterator(this._templateElement) {
+    inputs = new CompoundBinding(resolveInputs);

[justinfagnani, 2013/07/24 00:42:55]

why remove this from the initializer list, or make it non-final? I don't see an
assignment in the changes here.

[Jennifer Messerly, 2013/07/24 00:52:58]

it's sneaky: resolveInputs is no longer static, so we can't access it from an
initializer in Dart

   }
 
-  static Object resolveInputs(Map values) {
+  resolveInputs(Map values) {

[justinfagnani, 2013/07/24 00:42:55]

void?

[Jennifer Messerly, 2013/07/24 00:52:58]

Done.

[Jennifer Messerly, 2013/07/24 01:06:55]

update: it actually can't be void... CompoundBinding expects a value. I've
changed it to explicitly return null and added a comment

+    if (closed) return;
+
     if (values.containsKey('if') && !_toBoolean(values['if'])) {
-      return null;
+      valueChanged(null);
+    } else if (values.containsKey('repeat')) {
+      valueChanged(values['repeat']);
+    } else if (values.containsKey('bind')) {
+      valueChanged([values['bind']]);
+    } else {
+      valueChanged(null);
     }
-
-    if (values.containsKey('repeat')) {
-      return values['repeat'];
-    }
-
-    if (values.containsKey('bind')) {
-      return [values['bind']];
-    }
-
-    return null;
   }
 
   void valueChanged(value) {
-    // TODO(jmesserly): should PathObserver do this for us?
-    var oldValue = _lastValue;
-    _lastValue = value;
+    if (value is! List) value = null;

[justinfagnani, 2013/07/24 00:42:55]

when do we want to use Iterable over List?

[Jennifer Messerly, 2013/07/24 00:52:58]

good question. I think we should follow up with MDV about this.

offhand, it definitely needs indexing for some of the operations, but we could
call .toList(). However I'm not sure if the typical Iterable is going to
implement observability (or how you would define it without indexes).

 
-    if (value is! List) {
-      value = [];
-    }
-
+    var oldValue = iteratedValue;
     unobserve();
     iteratedValue = value;
 
-    if (value is Observable) {
-      _sub = value.changes.listen(_handleChanges);
+    if (iteratedValue is Observable) {
+      _sub = iteratedValue.changes.listen(_handleChanges);
     }
 
-    int addedCount = iteratedValue.length;
-    var removedCount = oldValue is List ? (oldValue as List).length : 0;
-    if (addedCount == 0 && removedCount == 0) return; // nothing to do.
+    var splices = calculateSplices(
+        iteratedValue != null ? iteratedValue : [],
+        oldValue != null ? oldValue : []);
 
-    _handleChanges([new ListChangeRecord(0, addedCount: addedCount,
-        removedCount: removedCount)]);
+    if (splices.length > 0) _handleChanges(splices);
+
+    if (inputs.length == 0) {
+      close();
+      _mdv(_templateElement)._templateIterator = null;
+    }
   }
 
   Node getTerminatorAt(int index) {
     if (index == -1) return _templateElement;
     var terminator = terminators[index];
     if (terminator is Element && (terminator as Element).isTemplate &&
         !identical(terminator, _templateElement)) {
       var subIterator = _mdv(terminator)._templateIterator;
       if (subIterator != null) {
         return subIterator.getTerminatorAt(subIterator.terminators.length - 1);
@@ skipping 45 matching lines @@
     var instanceNodes = instanceCache.remove(model);
     if (instanceNodes != null) return instanceNodes;
 
     var fragment = _templateElement.createInstance(model, syntax);
     instanceNodes = fragment.nodes.toList();
     fragment.nodes.clear();
     return instanceNodes;
   }
 
   void _handleChanges(Iterable<ChangeRecord> splices) {
+    if (closed) return;
+
     splices = splices.where((s) => s is ListChangeRecord);
 
     var template = _templateElement;
     var syntax = template.attributes['syntax'];
 
     if (template.parentNode == null || template.document.window == null) {
-      abandon();
+      close();
       // TODO(jmesserly): MDV calls templateIteratorTable.delete(this) here,
       // but I think that's a no-op because only nodes are used as keys.
       // See https://github.com/Polymer/mdv/pull/114.
       return;
     }
 
     // TODO(jmesserly): IdentityMap matches JS semantics, but it's O(N) right
     // now. See http://dartbug.com/4161.
     var instanceCache = new IdentityMap();
     var removeDelta = 0;
@@ skipping 24 matching lines @@
       instanceNodes.forEach(_unbindAllRecursively);
     }
   }
 
   void unobserve() {
     if (_sub == null) return;
     _sub.cancel();
     _sub = null;
   }
 
-  void abandon() {
+  void close() {
+    if (closed) return;
+
     unobserve();
-    _valueBinding.cancel();
     terminators.clear();
     inputs.dispose();
+    closed = true;
   }
 
   static void _unbindAllRecursively(Node node) {
     var nodeExt = _mdv(node);
     nodeExt._templateInstance = null;
     if (node is Element && (node as Element).isTemplate) {
       // Make sure we stop observing when we remove an element.
       var templateIterator = nodeExt._templateIterator;
       if (templateIterator != null) {
-        templateIterator.abandon();
+        templateIterator.close();
         nodeExt._templateIterator = null;
       }
     }
 
     _nodeOrCustom(node).unbindAll();
     for (var c = node.firstChild; c != null; c = c.nextNode) {
       _unbindAllRecursively(c);
     }
   }
 }