Ensure getInstanceModel is only called when a new instance will be created

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 222 matching lines @@
         !identical(terminator, _templateElement)) {
       var subIterator = _mdv(terminator)._templateIterator;
       if (subIterator != null) {
         return subIterator.getTerminatorAt(subIterator.terminators.length - 1);
       }
     }
 
     return terminator;
   }
 
-  void insertInstanceAt(int index, List<Node> instanceNodes) {
+  void insertInstanceAt(int index, DocumentFragment fragment,
+        List<Node> instanceNodes) {
+
     var previousTerminator = getTerminatorAt(index - 1);
-    var terminator = instanceNodes.length > 0 ? instanceNodes.last
-        : previousTerminator;
+    var terminator = null;
+    if (fragment != null) {
+      terminator = fragment.lastChild;
+    } else if (instanceNodes.length > 0) {
+      terminator = instanceNodes.last;
+    }
+    if (terminator == null) terminator = previousTerminator;

[justinfagnani, 2013/07/23 20:42:06]

Initialize terminator to null? I'm not sure if fragment.lastChild or
instanceNodes.last can be null, but talking to Steven, it sounded like there
were potential code size benefits to not allowing variables to be null. He
recommended making all possible instance vars final, not sure if that applies to
locals.

[Jennifer Messerly, 2013/07/23 20:49:39]

:confused: it is initialized to null ... yes, fragment.lastChild can be null,
likewise instanceNodes can be empty.

This particular case should be no problem for any competent compiler :). After
this line, "terminator" will have the same non-null properties as
previousTerminator.

 
     terminators.insert(index, terminator);
 
     var parent = _templateElement.parentNode;
     var insertBeforeNode = previousTerminator.nextNode;
+
+    if (fragment != null) {
+      parent.insertBefore(fragment, insertBeforeNode);
+      return;
+    }
+
     for (var node in instanceNodes) {
       parent.insertBefore(node, insertBeforeNode);
     }
   }
 
   List<Node> extractInstanceAt(int index) {
     var instanceNodes = <Node>[];
     var previousTerminator = getTerminatorAt(index - 1);
     var terminator = getTerminatorAt(index);
     terminators.removeAt(index);
 
     var parent = _templateElement.parentNode;
     while (terminator != previousTerminator) {
       var node = previousTerminator.nextNode;
       if (node == terminator) terminator = previousTerminator;
       node.remove();
       instanceNodes.add(node);
     }
     return instanceNodes;
   }
 
   getInstanceModel(model, BindingDelegate delegate) {
     if (delegate != null) {
       return delegate.getInstanceModel(_templateElement, model);
     }
     return model;
   }
 
-  List<Node> getInstanceNodes(model, BindingDelegate delegate,
-      IdentityMap instanceCache) {
-
-    var instanceNodes = instanceCache.remove(model);
-    if (instanceNodes != null) return instanceNodes;
-
-    var fragment = _templateElement.createInstance(model, delegate);
-    instanceNodes = fragment.nodes.toList();
-    fragment.nodes.clear();
-    return instanceNodes;
+  DocumentFragment getInstanceFragment(model, BindingDelegate delegate) {
+    return _templateElement.createInstance(model, delegate);
   }
 
   void _handleChanges(Iterable<ChangeRecord> splices) {
     if (closed) return;
 
     splices = splices.where((s) => s is ListChangeRecord);
 
     var template = _templateElement;
     var delegate = template.bindingDelegate;
 
@@ skipping 18 matching lines @@
       }
 
       removeDelta -= splice.addedCount;
     }
 
     for (var splice in splices) {
       for (var addIndex = splice.index;
           addIndex < splice.index + splice.addedCount;
           addIndex++) {
 
-        var model = getInstanceModel(iteratedValue[addIndex], delegate);
+        var model = iteratedValue[addIndex];
+        var fragment = null;
+        var instanceNodes = instanceCache.remove(model);
+        if (instanceNodes == null) {

[justinfagnani, 2013/07/23 20:42:06]

check for empty too?

[Jennifer Messerly, 2013/07/23 20:49:39]

if we expanded to an empty set of nodes before (hence cached it) we want to
reuse the empty nodes.

+          var actualModel = getInstanceModel(model, delegate);
+          fragment = getInstanceFragment(actualModel, delegate);
+        }
 
-        var instanceNodes = getInstanceNodes(model, delegate, instanceCache);
-        insertInstanceAt(addIndex, instanceNodes);
+        insertInstanceAt(addIndex, fragment, instanceNodes);
       }
     }
 
     for (var instanceNodes in instanceCache.values) {
       instanceNodes.forEach(_unbindAllRecursively);
     }
   }
 
   void unobserve() {
     if (_sub == null) return;
@@ skipping 21 matching lines @@
         nodeExt._templateIterator = null;
       }
     }
 
     _nodeOrCustom(node).unbindAll();
     for (var c = node.firstChild; c != null; c = c.nextNode) {
       _unbindAllRecursively(c);
     }
   }
 }