Merge pull request #1041 from plotly/merge-container-arrays-2

rreusser · web-flow · commit c4efa8001546 · 2016-10-17T12:22:08.000-04:00
Improved animation merging for layout and traces
diff --git a/src/lib/index.js b/src/lib/index.js
@@ -589,18 +589,36 @@ lib.objectFromPath = function(path, value) {
 /**
  * Iterate through an object in-place, converting dotted properties to objects.
  *
- * @example
- * lib.expandObjectPaths({'nested.test.path': 'value'});
- * // returns { nested: { test: {path: 'value'}}}
+ * Examples:
+ *
+ *   lib.expandObjectPaths({'nested.test.path': 'value'});
+ *     => { nested: { test: {path: 'value'}}}
+ *
+ * It also handles array notation, e.g.:
+ *
+ *   lib.expandObjectPaths({'foo[1].bar': 'value'});
+ *     => { foo: [null, {bar: value}] }
+ *
+ * It handles merges the results when two properties are specified in parallel:
+ *
+ *   lib.expandObjectPaths({'foo[1].bar': 10, 'foo[0].bar': 20});
+ *     => { foo: [{bar: 10}, {bar: 20}] }
+ *
+ * It does NOT, however, merge mulitple mutliply-nested arrays::
+ *
+ *   lib.expandObjectPaths({'marker[1].range[1]': 5, 'marker[1].range[0]': 4})
+ *     => { marker: [null, {range: 4}] }
  */
 
-// Store this to avoid recompiling regex on every prop since this may happen many
-// many times for animations.
-// TODO: Premature optimization? Remove?
-var dottedPropertyRegex = /^([^\.]*)\../;
+// Store this to avoid recompiling regex on *every* prop since this may happen many
+// many times for animations. Could maybe be inside the function. Not sure about
+// scoping vs. recompilation tradeoff, but at least it's not just inlining it into
+// the inner loop.
+var dottedPropertyRegex = /^([^\[\.]+)\.(.+)?/;
+var indexedPropertyRegex = /^([^\.]+)\[([0-9]+)\](\.)?(.+)?/;
 
 lib.expandObjectPaths = function(data) {
-    var match, key, prop, datum;
+    var match, key, prop, datum, idx, dest, trailingPath;
     if(typeof data === 'object' && !Array.isArray(data)) {
         for(key in data) {
             if(data.hasOwnProperty(key)) {
@@ -611,12 +629,47 @@ lib.expandObjectPaths = function(data) {
                     delete data[key];
 
                     data[prop] = lib.extendDeepNoArrays(data[prop] || {}, lib.objectFromPath(key, lib.expandObjectPaths(datum))[prop]);
+                } else if((match = key.match(indexedPropertyRegex))) {
+                    datum = data[key];
+
+                    prop = match[1];
+                    idx = parseInt(match[2]);
+
+                    delete data[key];
+
+                    data[prop] = data[prop] || [];
+
+                    if(match[3] === '.') {
+                        // This is the case where theere are subsequent properties into which
+                        // we must recurse, e.g. transforms[0].value
+                        trailingPath = match[4];
+                        dest = data[prop][idx] = data[prop][idx] || {};
+
+                        // NB: Extend deep no arrays prevents this from working on multiple
+                        // nested properties in the same object, e.g.
+                        //
+                        // {
+                        //   foo[0].bar[1].range
+                        //   foo[0].bar[0].range
+                        // }
+                        //
+                        // In this case, the extendDeepNoArrays will overwrite one array with
+                        // the other, so that both properties *will not* be present in the
+                        // result. Fixing this would require a more intelligent tracking
+                        // of changes and merging than extendDeepNoArrays currently accomplishes.
+                        lib.extendDeepNoArrays(dest, lib.objectFromPath(trailingPath, lib.expandObjectPaths(datum)));
+                    } else {
+                        // This is the case where this property is the end of the line,
+                        // e.g. xaxis.range[0]
+                        data[prop][idx] = lib.expandObjectPaths(datum);
+                    }
                 } else {
                     data[key] = lib.expandObjectPaths(data[key]);
                 }
             }
         }
     }
+
     return data;
 };
 
diff --git a/src/plots/plots.js b/src/plots/plots.js
@@ -1299,7 +1299,7 @@ plots.modifyFrames = function(gd, operations) {
  */
 plots.computeFrame = function(gd, frameName) {
     var frameLookup = gd._transitionData._frameHash;
-    var i, traceIndices, traceIndex, expandedObj, destIndex, copy;
+    var i, traceIndices, traceIndex, destIndex;
 
     var framePtr = frameLookup[frameName];
 
@@ -1326,9 +1326,7 @@ plots.computeFrame = function(gd, frameName) {
     // Merge, starting with the last and ending with the desired frame:
     while((framePtr = frameStack.pop())) {
         if(framePtr.layout) {
-            copy = Lib.extendDeepNoArrays({}, framePtr.layout);
-            expandedObj = Lib.expandObjectPaths(copy);
-            result.layout = Lib.extendDeepNoArrays(result.layout || {}, expandedObj);
+            result.layout = plots.extendLayout(result.layout, framePtr.layout);
         }
 
         if(framePtr.data) {
@@ -1363,16 +1361,99 @@ plots.computeFrame = function(gd, frameName) {
                     result.traces[destIndex] = traceIndex;
                 }
 
-                copy = Lib.extendDeepNoArrays({}, framePtr.data[i]);
-                expandedObj = Lib.expandObjectPaths(copy);
-                result.data[destIndex] = Lib.extendDeepNoArrays(result.data[destIndex] || {}, expandedObj);
+                result.data[destIndex] = plots.extendTrace(result.data[destIndex], framePtr.data[i]);
             }
         }
     }
 
     return result;
 };
 
+/**
+ * Extend an object, treating container arrays very differently by extracting
+ * their contents and merging them separately.
+ *
+ * This exists so that we can extendDeepNoArrays and avoid stepping into data
+ * arrays without knowledge of the plot schema, but so that we may also manually
+ * recurse into known container arrays, such as transforms.
+ *
+ * See extendTrace and extendLayout below for usage.
+ */
+plots.extendObjectWithContainers = function(dest, src, containerPaths) {
+    var containerProp, containerVal, i, j, srcProp, destProp, srcContainer, destContainer;
+    var copy = Lib.extendDeepNoArrays({}, src || {});
+    var expandedObj = Lib.expandObjectPaths(copy);
+    var containerObj = {};
+
+    // Step through and extract any container properties. Otherwise extendDeepNoArrays
+    // will clobber any existing properties with an empty array and then supplyDefaults
+    // will reset everything to defaults.
+    if(containerPaths && containerPaths.length) {
+        for(i = 0; i < containerPaths.length; i++) {
+            containerProp = Lib.nestedProperty(expandedObj, containerPaths[i]);
+            containerVal = containerProp.get();
+            containerProp.set(null);
+            Lib.nestedProperty(containerObj, containerPaths[i]).set(containerVal);
+        }
+    }
+
+    dest = Lib.extendDeepNoArrays(dest || {}, expandedObj);
+
+    if(containerPaths && containerPaths.length) {
+        for(i = 0; i < containerPaths.length; i++) {
+            srcProp = Lib.nestedProperty(containerObj, containerPaths[i]);
+            srcContainer = srcProp.get();
+
+            if(!srcContainer) continue;
+
+            destProp = Lib.nestedProperty(dest, containerPaths[i]);
+
+            destContainer = destProp.get();
+            if(!Array.isArray(destContainer)) {
+                destContainer = [];
+                destProp.set(destContainer);
+            }
+
+            for(j = 0; j < srcContainer.length; j++) {
+                destContainer[j] = plots.extendObjectWithContainers(destContainer[j], srcContainer[j]);
+            }
+        }
+    }
+
+    return dest;
+};
+
+/*
+ * Extend a trace definition. This method:
+ *
+ *  1. directly transfers any array references
+ *  2. manually recurses into container arrays like transforms
+ *
+ * The result is the original object reference with the new contents merged in.
+ */
+plots.extendTrace = function(destTrace, srcTrace) {
+    return plots.extendObjectWithContainers(destTrace, srcTrace, ['transforms']);
+};
+
+/*
+ * Extend a layout definition. This method:
+ *
+ *  1. directly transfers any array references (not critically important for
+ *     layout since there aren't really data arrays)
+ *  2. manually recurses into container arrays like annotations
+ *
+ * The result is the original object reference with the new contents merged in.
+ */
+plots.extendLayout = function(destLayout, srcLayout) {
+    return plots.extendObjectWithContainers(destLayout, srcLayout, [
+        'annotations',
+        'shapes',
+        'images',
+        'sliders',
+        'updatemenus'
+    ]);
+};
+
 /**
  * Transition to a set of new data and layout properties
  *
@@ -1411,16 +1492,7 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
 
             transitionedTraces.push(traceIdx);
 
-            // This is a multi-step process. First clone w/o arrays so that
-            // we're not modifying the original:
-            var update = Lib.extendDeepNoArrays({}, data[i]);
-
-            // Then expand object paths since we don't obey object-overwrite
-            // semantics here:
-            update = Lib.expandObjectPaths(update);
-
-            // Finally apply the update (without copying arrays, of course):
-            Lib.extendDeepNoArrays(gd.data[traceIndices[i]], update);
+            gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
         }
 
         // Follow the same procedure. Clone it so we don't mangle the input, then
diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js
@@ -84,7 +84,7 @@ describe('Test animate API', function() {
         });
 
         Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
-            Plotly.addFrames(gd, mockCopy.frames);
+            return Plotly.addFrames(gd, mockCopy.frames);
         }).then(done);
     });
 
diff --git a/test/jasmine/tests/lib_test.js b/test/jasmine/tests/lib_test.js
@@ -537,6 +537,90 @@ describe('Test lib.js:', function() {
             expect(input).toBe(expanded);
             expect(origArray).toBe(newArray);
         });
+
+        it('expands bracketed array notation', function() {
+            var input = {'marker[1]': {color: 'red'}};
+            var expected = {marker: [undefined, {color: 'red'}]};
+            expect(Lib.expandObjectPaths(input)).toEqual(expected);
+        });
+
+        it('expands nested arrays', function() {
+            var input = {'marker[1].range[1]': 5};
+            var expected = {marker: [undefined, {range: [undefined, 5]}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        it('expands bracketed array with more nested attributes', function() {
+            var input = {'marker[1]': {'color.alpha': 2}};
+            var expected = {marker: [undefined, {color: {alpha: 2}}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        it('expands bracketed array notation without further nesting', function() {
+            var input = {'marker[1]': 8};
+            var expected = {marker: [undefined, 8]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        it('expands bracketed array notation with further nesting', function() {
+            var input = {'marker[1].size': 8};
+            var expected = {marker: [undefined, {size: 8}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        it('expands bracketed array notation with further nesting', function() {
+            var input = {'marker[1].size.magnitude': 8};
+            var expected = {marker: [undefined, {size: {magnitude: 8}}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        it('combines changes with single array nesting', function() {
+            var input = {'marker[1].foo': 5, 'marker[0].foo': 4};
+            var expected = {marker: [{foo: 4}, {foo: 5}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+
+        // TODO: This test is unimplemented since it's a currently-unused corner case.
+        // Getting the test to pass requires some extension (pun?) to extendDeepNoArrays
+        // that's intelligent enough to only selectively merge *some* arrays, in particular
+        // not data arrays but yes on arrays that were previously expanded. This is a bit
+        // tricky to get to work just right and currently doesn't have any known use since
+        // container arrays are not multiply nested.
+        //
+        // Additional notes on what works or what doesn't work. This case does *not* work
+        // because the two nested arrays that would result from the expansion need to be
+        // deep merged.
+        //
+        //   Lib.expandObjectPaths({'marker.range[0]': 5, 'marker.range[1]': 2})
+        //
+        //   // => {marker: {range: [null, 2]}}
+        //
+        // This case *does* work becuase the array merging does not require a deep extend:
+        //
+        //   Lib.expandObjectPaths({'range[0]': 5, 'range[1]': 2}
+        //
+        //   // => {range: [5, 2]}
+        //
+        // Finally note that this case works fine becuase there's no merge necessary:
+        //
+        //   Lib.expandObjectPaths({'marker.range[1]': 2})
+        //
+        //   // => {marker: {range: [null, 2]}}
+        //
+        /*
+        it('combines changes', function() {
+            var input = {'marker[1].range[1]': 5, 'marker[1].range[0]': 4};
+            var expected = {marker: [undefined, {range: [4, 5]}]};
+            var computed = Lib.expandObjectPaths(input);
+            expect(computed).toEqual(expected);
+        });
+        */
     });
 
     describe('coerce', function() {
diff --git a/test/jasmine/tests/transition_test.js b/test/jasmine/tests/transition_test.js
@@ -65,6 +65,41 @@ function runTests(transitionDuration) {
                 }).catch(fail).then(done);
         });
 
+        it('transitions a transform', function(done) {
+            Plotly.restyle(gd, {
+                'transforms[0]': {
+                    enabled: true,
+                    type: 'filter',
+                    operation: '<',
+                    filtersrc: 'x',
+                    value: 10
+                }
+            }, [0]).then(function() {
+                expect(gd._fullData[0].transforms).toEqual([{
+                    enabled: true,
+                    type: 'filter',
+                    operation: '<',
+                    filtersrc: 'x',
+                    value: 10
+                }]);
+
+                return Plots.transition(gd, [{
+                    'transforms[0].operation': '>'
+                }], null, [0],
+                    {redraw: true, duration: transitionDuration},
+                    {duration: transitionDuration, easing: 'cubic-in-out'}
+                );
+            }).then(function() {
+                expect(gd._fullData[0].transforms).toEqual([{
+                    enabled: true,
+                    type: 'filter',
+                    operation: '>',
+                    filtersrc: 'x',
+                    value: 10
+                }]);
+            }).catch(fail).then(done);
+        });
+
         // This doesn't really test anything that the above tests don't cover, but it combines
         // the behavior and attempts to ensure chaining and events happen in the correct order.
         it('transitions may be chained', function(done) {
