Merge lp:~hatch/juju-gui/unit-breakout-1253114 into lp:juju-gui/experimental

[Jeff Pihach]

Add support to mix in delayed delta changes

Due to a race condition the unit details view would sometimes not update
as per the databinding because there were some deltas which had not yet
been applied. This adds the functionality to mix these deltas in or
filter by them as was the previous functionality.

https://codereview.appspot.com/36850043/

[Jeff Pihach (hatch)]

Reviewers: mp+197619_code.launchpad.net,

Message:
Please take a look.

Description:
Add support to mix in delayed delta changes

Due to a race condition the unit details view would sometimes not update
as per the databinding because there were some deltas which had not yet
been applied. This adds the functionality to mix these deltas in or
filter by them as was the previous functionality.

To QA:
In sandbox deploy service with 10 units, wait a minute or so for a
number
of deltas to come through. Then start clicking on units in the inspector
unit list to display the details, after viewing about 50 units if you
haven't
seen the unit details header unpopulated it's safe to assume that the
issue is resolved.

https://code.launchpad.net/~hatch/juju-gui/unit-breakout-1253114/+merge/197619

(do not edit description out of merge proposal)

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

Affected files (+24, -11 lines):
   A [revision details]
   M app/views/databinding.js
   M app/views/viewlets/unit-details.js

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: app/views/databinding.js
=== modified file 'app/views/databinding.js'
--- app/views/databinding.js 2013-10-11 03:18:33 +0000
+++ app/views/databinding.js 2013-12-03 22:42:52 +0000
@@ -155,12 +155,15 @@

        @method deltaFromChange
        @param {Array} modelChangeKeys array of {String} keys that have
changed.
+ @param {Boolean} mixin if you want to mix the modelChangeKeys into
the
+ changed value list or filter to use only those bindings. Default
false.
        @return {Array} bindings array filtered by keys when present.
      */
- function deltaFromChange(modelChangeKeys) {
+ function deltaFromChange(modelChangeKeys, mixin) {
        /* jshint -W040 */
        // Ignore 'possible strict violation'
        var bindings = this._bindings;
+ mixin = mixin || false;
        var result = {bindings: [], wildcards: {}};
        // Handle wildcards.
        result.wildcards = indexBindings(bindings, function(binding) {
@@ -172,19 +175,26 @@

        if (modelChangeKeys !== undefined && modelChangeKeys.length !== 0) {
          // 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
+ // of keys that have changed, so we may 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);
- });
+ var filteredBindings;
+ if (mixin) {
+ filteredBindings = bindings;
+ ...

[Gary Poster (gary)]

Hey Jeff. Thank you for tackling this tricky problem.

I think your attention on this area has indicated that resetDOMToModel
needs some changes to deltaFromChange, because it should also reset
dependents.

However, I don't think that your change to _modelDOMHandler is right.
It's too big. See my comments below. I could be wrong, but I'd like to
talk this through with you tomorrow.

Thanks,

Gary

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

https://codereview.appspot.com/36850043/diff/1/app/views/databinding.js#newcode159
app/views/databinding.js:159: changed value list or filter to use only
those bindings. Default false.
I want to remove this new parameter. See below.

https://codereview.appspot.com/36850043/diff/1/app/views/databinding.js#newcode176
app/views/databinding.js:176: if (modelChangeKeys !== undefined &&
modelChangeKeys.length !== 0) {
I suggest we change the rest of this code as follows.

var filteredBindings = bindings;
if (modelChangeKeys !== undefined && modelChangeKeys.length !== 0) {
   // 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.
   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.
...

https://codereview.appspot.com/36850043/diff/1/app/views/databinding.js#newcode216
app/views/databinding.js:216: } else {
This branch would disappear. Now, if you don't specify modelChangeKeys,
you get bindings *plus* dependent bindings.

https://codereview.appspot.com/36850043/diff/1/app/views/databinding.js#newcode718
app/views/databinding.js:718: delta = deltaFromChange.call(this);
Shouldn't this include dependents? If so, the deltaFromChange function
could be a lot prettier. See discussion above.

https://codereview.appspot.com/36850043/diff/1/app/views/databinding.js#newcode872
app/views/databinding.js:872: delta = deltaFromChange.call(this,
this._unappliedChanges, true);
I'm proposing that this could be spelled

delta = deltaFromChange.call(this);

However, why do we want to ignore the fact that we know what the
unapplied changes are? It seems wrong. Could you explain? I have a
feeling that this is too blunt of a hammer.

If it *is* the right approach, we need a very good explanation in the
comments...*and* we should remove all references to _unappliedChanges,
because it is never used in your code.

https://codereview.appspot.com/36850043/

[lp:~hatch/juju-gui/unit-breakout-1253114]

1220. By Jeff Pihach on 2013-12-04

cleanup refactor to simplify deltaFromChange and fix hidden bug in resetDOMToModel

[Jeff Pihach (hatch)]

Please take a look.

https://codereview.appspot.com/36850043/

[Gary Poster (gary)]

LGTM, thank you!

https://codereview.appspot.com/36850043/

[Jeff Pihach (hatch)]

*** Submitted:

Add support to mix in delayed delta changes

Due to a race condition the unit details view would sometimes not update
as per the databinding because there were some deltas which had not yet
been applied. This adds the functionality to mix these deltas in or
filter by them as was the previous functionality.

R=gary.poster
CC=
https://codereview.appspot.com/36850043

https://codereview.appspot.com/36850043/

=== modified file 'app/views/databinding.js'
--- app/views/databinding.js	2013-10-11 03:18:33 +0000
+++ app/views/databinding.js	2013-12-04 17:05:50 +0000
@@ -170,45 +170,44 @@
         return binding.name;
       }, true);
 
+      var filteredBindings = bindings;
       if (modelChangeKeys !== undefined && modelChangeKeys.length !== 0) {
         // 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
+        // of keys that have changed, so we may want to limit the resulting
         // bindings appropriately.
+
         // Find the bindings that match the modelChangeKeys.
-        var filteredBindings = bindings.filter(function(binding) {
+        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);
-                }
+      }
+
+      // 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);
-      }
-
+            }
+          });
+        }
+      });
 
       return result;
     }
@@ -839,26 +838,33 @@
       @param {Event} evt Y.Model change event.
      */
     BindingEngine.prototype._modelChangeHandler = function(evt) {
-      var keys, delta;
+      var keys, delta, unappliedChanges = [];
       var initialize = !evt; // If there is no event, this is an initialization.
-      if (Y.Lang.isArray(evt)) {
-        // Object.observe updates
-        keys = evt.map(function(update) { return update.name; });
-      } else {
-        keys = evt && Y.Object.keys(evt.changed);
-      }
-
-      // Mix any unapplied changes into the key set
-      // updating this list. We then use that combined
-      // list to generate the binding set.
-      if (keys) {
-        keys.forEach(function(k) {
-          if (this._unappliedChanges.indexOf(k) === -1) {
-            this._unappliedChanges.push(k);
-          }
-        }, this);
-      }
-      delta = deltaFromChange.call(this, this._unappliedChanges);
+
+      // If this is an initialization then we want to force all
+      // changes not just the unapplied changes.
+      if (!initialize) {
+        if (Y.Lang.isArray(evt)) {
+          // Object.observe updates
+          keys = evt.map(function(update) { return update.name; });
+        } else {
+          keys = evt && Y.Object.keys(evt.changed);
+        }
+
+        // Mix any unapplied changes into the key set
+        // updating this list. We then use that combined
+        // list to generate the binding set.
+        if (keys) {
+          keys.forEach(function(k) {
+            if (this._unappliedChanges.indexOf(k) === -1) {
+              this._unappliedChanges.push(k);
+            }
+          }, this);
+        }
+        unappliedChanges = this._unappliedChanges;
+      }
+
+      delta = deltaFromChange.call(this, unappliedChanges);
 
       if (this._updateTimeout) {
         this._updateTimeout.cancel();
@@ -905,7 +911,6 @@
       // identify conflicts and handle them.
       //
       // This is all done per-binding in the forEach loop below.
-
       var bindingEngine = this;
 
       // updateDOM applies all the changes clearing the buffer.

=== modified file 'app/views/viewlets/unit-details.js'
--- app/views/viewlets/unit-details.js	2013-11-20 15:09:44 +0000
+++ app/views/viewlets/unit-details.js	2013-12-04 17:05:50 +0000
@@ -104,6 +104,7 @@
               data += '<li>' + key + ': ' + value[key] + '</li>';
             });
             node.one('ul').setHTML(data);
+            node.show();
           } else {
             node.hide();
           }