Improve robustness of command bindings

Author: rreusser

src/components/sliders/draw.js

@@ -71,14 +71,12 @@ module.exports = function draw(gd) {
 
         computeLabelSteps(sliderOpts);
 
-        if(!sliderOpts._commandObserver) {
-            sliderOpts._commandObserver = Plots.createCommandObserver(gd, sliderOpts.steps, function(data) {
-                if(sliderOpts.active === data.index) return;
-                if(sliderOpts._dragging) return;
+        Plots.createCommandObserver(gd, sliderOpts, sliderOpts.steps, function(data) {
+            if(sliderOpts.active === data.index) return;
+            if(sliderOpts._dragging) return;
 
-                setActive(gd, gSlider, sliderOpts, data.index, false, true);
-            });
-        }
+            setActive(gd, gSlider, sliderOpts, data.index, false, true);
+        });
 
         drawSlider(gd, d3.select(this), sliderOpts);
 

src/components/updatemenus/draw.js

@@ -113,12 +113,10 @@ module.exports = function draw(gd) {
     headerGroups.each(function(menuOpts) {
         var gHeader = d3.select(this);
 
-        if(!menuOpts._commandObserver) {
-            var _gButton = menuOpts.type === 'dropdown' ? gButton : null;
-            menuOpts._commandObserver = Plots.createCommandObserver(gd, menuOpts.buttons, function(data) {
-                setActive(gd, menuOpts, menuOpts.buttons[data.index], gHeader, _gButton, data.index, true);
-            });
-        }
+        var _gButton = menuOpts.type === 'dropdown' ? gButton : null;
+        Plots.createCommandObserver(gd, menuOpts, menuOpts.buttons, function(data) {
+            setActive(gd, menuOpts, menuOpts.buttons[data.index], gHeader, _gButton, data.index, true);
+        });
 
         if(menuOpts.type === 'dropdown') {
             drawHeader(gd, gHeader, gButton, menuOpts);

src/plots/command.js

@@ -13,92 +13,63 @@ var Plotly = require('../plotly');
 var Lib = require('../lib');
 
 /*
- * This function checks to see if an array of objects containing
- * method and args properties is compatible with automatic two-way
- * binding. The criteria right now are that
+ * Create or update an observer. This function is designed to be
+ * idempotent so that it can be called over and over as the component
+ * updates, and will attach and detach listeners as needed.
  *
- *   1. multiple traces may be affected
- *   2. only one property may be affected
- *   3. the same property must be affected by all commands
+ * @param {optional object} container
+ *      An object on which the observer is stored. This is the mechanism
+ *      by which it is idempotent. If it already exists, another won't be
+ *      added. Each time it's called, the value lookup table is updated.
+ * @param {array} commandList
+ *      An array of commands, following either `buttons` of `updatemenus`
+ *      or `steps` of `sliders`.
+ * @param {function} onchange
+ *      A listener called when the value is changed. Receives data object
+ *      with information about the new state.
  */
-exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) {
-    var n = commandList.length;
+exports.createCommandObserver = function(gd, container, commandList, onchange) {
+    var ret = {};
+    var enabled = true;
 
-    var refBinding;
+    if(container && container._commandObserver) {
+        ret = container._commandObserver;
+    }
 
-    for(var i = 0; i < n; i++) {
-        var binding;
-        var command = commandList[i];
-        var method = command.method;
-        var args = command.args;
+    if(!ret.cache) {
+        ret.cache = {};
+    }
 
-        // If any command has no method, refuse to bind:
-        if(!method) {
-            return false;
-        }
-        var bindings = exports.computeAPICommandBindings(gd, method, args);
+    // Either create or just recompute this:
+    ret.lookupTable = {};
 
-        // Right now, handle one and *only* one property being set:
-        if(bindings.length !== 1) {
-            return false;
-        }
+    var binding = exports.hasSimpleAPICommandBindings(gd, commandList, ret.lookupTable);
 
-        if(!refBinding) {
-            refBinding = bindings[0];
-            if(Array.isArray(refBinding.traces)) {
-                refBinding.traces.sort();
+    if(container && container._commandObserver) {
+        if(!binding) {
+            // If container exists and there are no longer any bindings,
+            // remove existing:
+            if(container._commandObserver.remove) {
+                container._commandObserver.remove();
+                container._commandObserver = null;
+                return ret;
             }
         } else {
-            binding = bindings[0];
-            if(binding.type !== refBinding.type) {
-                return false;
-            }
-            if(binding.prop !== refBinding.prop) {
-                return false;
-            }
-            if(Array.isArray(refBinding.traces)) {
-                if(Array.isArray(binding.traces)) {
-                    binding.traces.sort();
-                    for(var j = 0; j < refBinding.traces.length; j++) {
-                        if(refBinding.traces[j] !== binding.traces[j]) {
-                            return false;
-                        }
-                    }
-                } else {
-                    return false;
-                }
-            } else {
-                if(binding.prop !== refBinding.prop) {
-                    return false;
-                }
-            }
-        }
+            // If container exists and there *are* bindings, then the lookup
+            // table should have been updated and check is already attached,
+            // so there's nothing to be done:
+            return ret;
+
 
-        binding = bindings[0];
-        var value = binding.value;
-        if(Array.isArray(value)) {
-            value = value[0];
-        }
-        if(bindingsByValue) {
-            bindingsByValue[value] = i;
         }
     }
 
-    return refBinding;
-};
-
-exports.createCommandObserver = function(gd, commandList, onchange) {
-    var cache = {};
-    var lookupTable = {};
-    var check, remove;
-    var enabled = true;
-
     // Determine whether there's anything to do for this binding:
-    var binding;
-    if((binding = exports.hasSimpleAPICommandBindings(gd, commandList, lookupTable))) {
-        bindingValueHasChanged(gd, binding, cache);
 
-        check = function check() {
+    if(binding) {
+        bindingValueHasChanged(gd, binding, ret.cache);
+
+        ret.check = function check() {
             if(!enabled) return;
 
             var container, value, obj;
@@ -117,7 +88,7 @@ exports.createCommandObserver = function(gd, commandList, onchange) {
 
             value = Lib.nestedProperty(container, binding.prop).get();
 
-            obj = cache[binding.type] = cache[binding.type] || {};
+            obj = ret.cache[binding.type] = ret.cache[binding.type] || {};
 
             if(obj.hasOwnProperty(binding.prop)) {
                 if(obj[binding.prop] !== value) {
@@ -130,15 +101,15 @@ exports.createCommandObserver = function(gd, commandList, onchange) {
             if(changed && onchange) {
                 // Disable checks for the duration of this command in order to avoid
                 // infinite loops:
-                if(lookupTable[value] !== undefined) {
-                    disable();
+                if(ret.lookupTable[value] !== undefined) {
+                    ret.disable();
                     Promise.resolve(onchange({
                         value: value,
                         type: binding.type,
                         prop: binding.prop,
                         traces: binding.traces,
-                        index: lookupTable[value]
-                    })).then(enable, enable);
+                        index: ret.lookupTable[value]
+                    })).then(ret.enable, ret.enable);
                 }
             }
 
@@ -155,32 +126,111 @@ exports.createCommandObserver = function(gd, commandList, onchange) {
         ];
 
         for(var i = 0; i < checkEvents.length; i++) {
-            gd._internalOn(checkEvents[i], check);
+            gd._internalOn(checkEvents[i], ret.check);
         }
 
-        remove = function() {
+        ret.remove = function() {
             for(var i = 0; i < checkEvents.length; i++) {
-                gd._removeInternalListener(checkEvents[i], check);
+                gd._removeInternalListener(checkEvents[i], ret.check);
             }
         };
     } else {
-        lookupTable = {};
-        remove = function() {};
+        // TODO: It'd be really neat to actually give a *reason* for this, but at least a warning
+        // is a start
+        Lib.warn('Unable to automatically bind plot updates to API command');
+
+        ret.lookupTable = {};
+        ret.remove = function() {};
     }
 
-    function disable() {
+    ret.disable = function disable() {
         enabled = false;
-    }
+    };
 
-    function enable() {
+    ret.enable = function enable() {
         enabled = true;
+    };
+
+    if(container) {
+        container._commandObserver = ret;
     }
 
-    return {
-        disable: disable,
-        enable: enable,
-        remove: remove
-    };
+    return ret;
+};
+
+/*
+ * This function checks to see if an array of objects containing
+ * method and args properties is compatible with automatic two-way
+ * binding. The criteria right now are that
+ *
+ *   1. multiple traces may be affected
+ *   2. only one property may be affected
+ *   3. the same property must be affected by all commands
+ */
+exports.hasSimpleAPICommandBindings = function(gd, commandList, bindingsByValue) {
+    var n = commandList.length;
+
+    var refBinding;
+
+    for(var i = 0; i < n; i++) {
+        var binding;
+        var command = commandList[i];
+        var method = command.method;
+        var args = command.args;
+
+        // If any command has no method, refuse to bind:
+        if(!method) {
+            return false;
+        }
+        var bindings = exports.computeAPICommandBindings(gd, method, args);
+
+        // Right now, handle one and *only* one property being set:
+        if(bindings.length !== 1) {
+            return false;
+        }
+
+        if(!refBinding) {
+            refBinding = bindings[0];
+            if(Array.isArray(refBinding.traces)) {
+                refBinding.traces.sort();
+            }
+        } else {
+            binding = bindings[0];
+            if(binding.type !== refBinding.type) {
+                return false;
+            }
+            if(binding.prop !== refBinding.prop) {
+                return false;
+            }
+            if(Array.isArray(refBinding.traces)) {
+                if(Array.isArray(binding.traces)) {
+                    binding.traces.sort();
+                    for(var j = 0; j < refBinding.traces.length; j++) {
+                        if(refBinding.traces[j] !== binding.traces[j]) {
+                            return false;
+                        }
+                    }
+                } else {
+                    return false;
+                }
+            } else {
+                if(binding.prop !== refBinding.prop) {
+                    return false;
+                }
+            }
+        }
+
+        binding = bindings[0];
+        var value = binding.value;
+        if(Array.isArray(value)) {
+            value = value[0];
+        }
+        if(bindingsByValue) {
+            bindingsByValue[value] = i;
+        }
+    }
+
+    return refBinding;
 };
 
 function bindingValueHasChanged(gd, binding, cache) {
@@ -213,6 +263,17 @@ function bindingValueHasChanged(gd, binding, cache) {
     return changed;
 }
 
+/*
+ * Execute an API command. There's really not much to this; it just provides
+ * a common hook so that implementations don't need to be synchronized across
+ * multiple components with the ability to invoke API commands.
+ *
+ * @param {string} method
+ *      The name of the plotly command to execute. Must be one of 'animate',
+ *      'restyle', 'relayout', 'update'.
+ * @param {array} args
+ *      A list of arguments passed to the API command
+ */
 exports.executeAPICommand = function(gd, method, args) {
     var apiMethod = Plotly[method];
 
@@ -223,6 +284,7 @@ exports.executeAPICommand = function(gd, method, args) {
 
     return apiMethod.apply(null, allArgs).catch(function(err) {
         Lib.warn('API call to Plotly.' + method + ' rejected.', err);
+        return Promise.reject(err);
     });
 };
 
@@ -243,9 +305,10 @@ exports.computeAPICommandBindings = function(gd, method, args) {
             bindings = computeAnimateBindings(gd, args);
             break;
         default:
-            // This is the case where someone forgot to whitelist and implement
-            // a new API method, so focus on failing visibly.
-            throw new Error('Command bindings for Plotly.' + method + ' not implemented');
+            // This is the case where intelligent logic about what affects
+            // this command is not implemented. It causes no ill effects.
+            // For example, addFrames simply won't bind to a control component.
+            bindings = [];
     }
     return bindings;
 };