Merge lp:~gary/juju-gui/fixDeltaFromChangeEdgeCase into lp:juju-gui/experimental

[Gary Poster]

optimize and fix databinding.deltaFromChange

This is a grabbag of three small changes. The biggest change is that I did some optimizations of databinding.deltaFromChange and fixed an edge case (with test). I also optimized _setupDependencies for that edge case, added some documentation about bindings (so I could refer to it myself), and optimized the bind/_bind pair a bit by removing redundant calls to code that iterates over all bindings (they are now done once in bind, rather than once per viewlet in _bind).

https://codereview.appspot.com/13348056/

[Gary Poster (gary)]

Reviewers: mp+185053_code.launchpad.net,

Message:
Please take a look.

Description:
optimize and fix databinding.deltaFromChange

This is a grabbag of three small changes. The biggest change is that I
did some optimizations of databinding.deltaFromChange and fixed an edge
case (with test). I also optimized _setupDependencies for that edge
case, added some documentation about bindings (so I could refer to it
myself), and optimized the bind/_bind pair a bit by removing redundant
calls to code that iterates over all bindings (they are now done once in
bind, rather than once per viewlet in _bind).

https://code.launchpad.net/~gary/juju-gui/fixDeltaFromChangeEdgeCase/+merge/185053

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/13348056/

Affected files (+71, -29 lines):
   A [revision details]
   M app/views/databinding.js
   M test/test_databinding.js

[Gary Poster (gary)]

Line-by-line comments for reviewers.

https://codereview.appspot.com/13348056/diff/1/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/13348056/diff/1/app/views/databinding.js#newcode186
app/views/databinding.js:186: var index = indexBindings(bindings, null,
true);
The "true" argument and then the "result.bindings.push.apply" below is
the real fix. All the other changes streamline the logic, resulting in
much fewer duplicate results and hopefully clearer reading.

https://codereview.appspot.com/13348056/diff/1/app/views/databinding.js#newcode393
app/views/databinding.js:393: this._modelChangeHandler();
These were moved from _bind, mostly as an optimization for reading but
also to prevent unnecessary duplication of work.

https://codereview.appspot.com/13348056/diff/1/app/views/databinding.js#newcode533
app/views/databinding.js:533: index[dep] = source;
This is just an optimization, so that if we have more than one of these
implicit bindings on the same source, we don't create multiple copies of
the source. It wasn't causing a bug, but this just makes it easier to
debug further downstream.

https://codereview.appspot.com/13348056/

[Gary Poster (gary)]

*** Submitted:

optimize and fix databinding.deltaFromChange

This is a grabbag of three small changes. The biggest change is that I
did some optimizations of databinding.deltaFromChange and fixed an edge
case (with test). I also optimized _setupDependencies for that edge
case, added some documentation about bindings (so I could refer to it
myself), and optimized the bind/_bind pair a bit by removing redundant
calls to code that iterates over all bindings (they are now done once in
bind, rather than once per viewlet in _bind).

R=benji
CC=
https://codereview.appspot.com/13348056

https://codereview.appspot.com/13348056/

=== modified file 'app/views/databinding.js'
--- app/views/databinding.js	2013-09-11 03:13:28 +0000
+++ app/views/databinding.js	2013-09-11 13:37:02 +0000
@@ -162,8 +162,7 @@
       // Ignore 'possible strict violation'
       var bindings = this._bindings;
       var result = {bindings: [], wildcards: {}};
-      var index = indexBindings(bindings);
-      // Handle wildcards (before we filter down bindings)
+      // Handle wildcards.
       result.wildcards = indexBindings(bindings, function(binding) {
         if (binding.name !== '*' && binding.name !== '+') {
           return;
@@ -172,30 +171,44 @@
       }, true);
 
       if (modelChangeKeys !== undefined && modelChangeKeys.length !== 0) {
-        bindings = bindings.filter(function(binding) {
+        // In this branch of the the conditional, we only have a specific set
+        // of keys that have changed, so we want to limit the resulting
+        // bindings appropriately.
+        // Find the bindings that match the modelChangeKeys.
+        var filteredBindings = bindings.filter(function(binding) {
           // Change events don't honor nested key paths. This means
           // we may update bindings that impact multiple DOM nodes
           // (our granularity is too low).
           return (modelChangeKeys.indexOf(binding.name.split('.')[0]) > -1);
         });
+        // Add dependents.
+        // We make an index of all bindings to help with this.
+        var index = indexBindings(bindings, null, true);
+        var added = {};
+        filteredBindings.forEach(function(binding) {
+          if (binding.name === '*' ||
+              binding.name === '+') {
+            return;
+          }
+          result.bindings.push(binding);
+          if (binding.dependents) {
+            binding.dependents.forEach(function(dep) {
+              if (!added[dep]) {
+                added[dep] = true;
+                var depends = index[dep];
+                if (depends) {
+                  result.bindings.push.apply(result.bindings, depends);
+                }
+              }
+            });
+          }
+        });
+      } else {
+        // If we don't have modelChangeKeys, we simply want all of the
+        // existing bindings.
+        result.bindings.push.apply(result.bindings, bindings);
       }
 
-      // Handle deps
-      bindings.forEach(function(binding) {
-        if (binding.name === '*' ||
-            binding.name === '+') {
-          return;
-        }
-        result.bindings.push(binding);
-        if (binding.dependents) {
-          binding.dependents.forEach(function(dep) {
-            var depends = index[dep];
-            if (depends) {
-              result.bindings.push(depends);
-            }
-          });
-        }
-      });
 
       return result;
     }
@@ -281,10 +294,24 @@
     };
 
     /**
-     * @method addBinding
-     * @param {Object} config A bindings Object, see description in `bind`.
-     * @param {Object} viewlet A reference to the viewlet being bound.
-     * @return {Object} binding.
+      Add a binding from a configuration and a viewlet.
+
+      A binding has the following attributes and methods.
+
+       * name: String
+       * get(model):
+       * target: (optional) Associated DOM node
+       * field: (optional) Associated NodeHandler
+       * dependents: (optional) Array of binding names that should be updated
+                     when this one is.
+       * format(value): (optional)
+       * update(node, value): (optional)
+
+
+      @method addBinding
+      @param {Object} config A bindings Object, see description in `bind`.
+      @param {Object} viewlet A reference to the viewlet being bound.
+      @return {Object} binding.
      */
     BindingEngine.prototype.addBinding = function(config, viewlet) {
       var defaultBinding = {};
@@ -321,8 +348,8 @@
       From within the DOM, data-bind='model key' attributes can be used to
       associate bindings from the model into the DOM. Nested keys are supported
       by using '.' (dotted) paths. As an important development/debug tip when
-      trying to use YUI to select a dotted path name make sure to quote the
-      entire path. For example Y.one('[data-bind="a.b.c."]').
+      trying to use YUI to select a dotted path name, make sure to quote the
+      entire path. For example Y.one('[data-bind="a.b.c"]').
 
       Viewlets can provide a number of configuration options
       for use here:
@@ -360,6 +387,10 @@
       if (!Y.Lang.isArray(viewlets)) { viewlets = [viewlets]; }
       Y.each(viewlets, function(v) {
         this._bind(model, v);}, this);
+      this._setupHeirarchicalBindings();
+      this._setupDependencies();
+      // Initialize viewlets with starting values.
+      this._modelChangeHandler();
       return this;
     };
 
@@ -440,11 +471,7 @@
 
       }, this);
 
-      this._setupHeirarchicalBindings();
-      this._setupDependencies();
       this._setupWildcarding(viewlet);
-      this._modelChangeHandler();
-
       return this;
     };
 
@@ -503,6 +530,7 @@
               source = self.addBinding({
                 name: dep,
                 dependents: []}, binding.viewlet);
+              index[dep] = source;
             }
             if (source.dependents === undefined) {
               source.dependents = [];

=== modified file 'test/test_databinding.js'
--- test/test_databinding.js	2013-09-11 03:13:28 +0000
+++ test/test_databinding.js	2013-09-11 13:37:02 +0000
@@ -681,6 +681,18 @@
                    .get('value'), 'Sansa Lannister');
     });
 
+    it('should handle multiple dependents', function() {
+      var model = new TestModel({first: 'Ned', last: 'Stark'});
+      container.setHTML(
+          '<input data-bind="full"><span data-bind="full"></span>');
+      engine = new BindingEngine({interval: 0});
+      engine.bind(model, viewlet);
+      assert.equal(container.one('input[data-bind="full"]')
+                   .get('value'), 'Ned Stark');
+      assert.equal(container.one('span[data-bind="full"]')
+                   .get('text'), 'Ned Stark');
+    });
+
   });
 
   describe('modellist tests', function() {