Merge pull request #2313 from plotly/array-anim

Minor changes to animation code ahead of the array animation fix

Author: monfera

src/plot_api/plot_api.js

@@ -42,6 +42,8 @@ var enforceAxisConstraints = axisConstraints.enforce;
 var cleanAxisConstraints = axisConstraints.clean;
 var axisIds = require('../plots/cartesian/axis_ids');
 
+var numericNameWarningCount = 0;
+var numericNameWarningCountLimit = 5;
 
 /**
  * Main plot-creation function
@@ -2592,16 +2594,14 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
  *      - traces {array} trace indices
  *      - baseframe {string} name of frame from which this frame gets defaults
  *
- *  @param {array of integers) indices
+ *  @param {array of integers} indices
  *      an array of integer indices matching the respective frames in `frameList`. If not
  *      provided, an index will be provided in serial order. If already used, the frame
  *      will be overwritten.
  */
 Plotly.addFrames = function(gd, frameList, indices) {
     gd = Lib.getGraphDiv(gd);
 
-    var numericNameWarningCount = 0;
-
     if(frameList === null || frameList === undefined) {
         return Promise.resolve();
     }
@@ -2616,7 +2616,7 @@ Plotly.addFrames = function(gd, frameList, indices) {
 
     var i, frame, j, idx;
     var _frames = gd._transitionData._frames;
-    var _hash = gd._transitionData._frameHash;
+    var _frameHash = gd._transitionData._frameHash;
 
 
     if(!Array.isArray(frameList)) {
@@ -2631,28 +2631,35 @@ Plotly.addFrames = function(gd, frameList, indices) {
     var bigIndex = _frames.length + frameList.length * 2;
 
     var insertions = [];
+    var _frameHashLocal = {};
     for(i = frameList.length - 1; i >= 0; i--) {
         if(!Lib.isPlainObject(frameList[i])) continue;
 
-        var name = (_hash[frameList[i].name] || {}).name;
+        // The entire logic for checking for this type of name collision can be removed once we migrate to ES6 and
+        // use a Map instead of an Object instance, as Map keys aren't converted to strings.
+        var lookupName = frameList[i].name;
+        var name = (_frameHash[lookupName] || _frameHashLocal[lookupName] || {}).name;
         var newName = frameList[i].name;
+        var collisionPresent = _frameHash[name] || _frameHashLocal[name];
 
-        if(name && newName && typeof newName === 'number' && _hash[name]) {
+        if(name && newName && typeof newName === 'number' && collisionPresent && numericNameWarningCount < numericNameWarningCountLimit) {
             numericNameWarningCount++;
 
-            Lib.warn('addFrames: overwriting frame "' + _hash[name].name +
+            Lib.warn('addFrames: overwriting frame "' + (_frameHash[name] || _frameHashLocal[name]).name +
                 '" with a frame whose name of type "number" also equates to "' +
                 name + '". This is valid but may potentially lead to unexpected ' +
                 'behavior since all plotly.js frame names are stored internally ' +
                 'as strings.');
 
-            if(numericNameWarningCount > 5) {
-                Lib.warn('addFrames: This API call has yielded too many warnings. ' +
+            if(numericNameWarningCount === numericNameWarningCountLimit) {
+                Lib.warn('addFrames: This API call has yielded too many of these warnings. ' +
                     'For the rest of this call, further warnings about numeric frame ' +
                     'names will be suppressed.');
             }
         }
 
+        _frameHashLocal[lookupName] = {name: lookupName};
+
         insertions.push({
             frame: Plots.supplyFrameDefaults(frameList[i]),
             index: (indices && indices[i] !== undefined && indices[i] !== null) ? indices[i] : bigIndex + i
@@ -2682,10 +2689,10 @@ Plotly.addFrames = function(gd, frameList, indices) {
         if(!frame.name) {
             // Repeatedly assign a default name, incrementing the counter each time until
             // we get a name that's not in the hashed lookup table:
-            while(_hash[(frame.name = 'frame ' + gd._transitionData._counter++)]);
+            while(_frameHash[(frame.name = 'frame ' + gd._transitionData._counter++)]);
         }
 
-        if(_hash[frame.name]) {
+        if(_frameHash[frame.name]) {
             // If frame is present, overwrite its definition:
             for(j = 0; j < _frames.length; j++) {
                 if((_frames[j] || {}).name === frame.name) break;

src/plots/plots.js

@@ -1779,7 +1779,7 @@ plots.graphJson = function(gd, dataonly, mode, output, useDefaults) {
 plots.modifyFrames = function(gd, operations) {
     var i, op, frame;
     var _frames = gd._transitionData._frames;
-    var _hash = gd._transitionData._frameHash;
+    var _frameHash = gd._transitionData._frameHash;
 
     for(i = 0; i < operations.length; i++) {
         op = operations[i];
@@ -1788,32 +1788,32 @@ plots.modifyFrames = function(gd, operations) {
             // No reason this couldn't exist, but is currently unused/untested:
             /* case 'rename':
                 frame = _frames[op.index];
-                delete _hash[frame.name];
-                _hash[op.name] = frame;
+                delete _frameHash[frame.name];
+                _frameHash[op.name] = frame;
                 frame.name = op.name;
                 break;*/
             case 'replace':
                 frame = op.value;
                 var oldName = (_frames[op.index] || {}).name;
                 var newName = frame.name;
-                _frames[op.index] = _hash[newName] = frame;
+                _frames[op.index] = _frameHash[newName] = frame;
 
                 if(newName !== oldName) {
                     // If name has changed in addition to replacement, then update
                     // the lookup table:
-                    delete _hash[oldName];
-                    _hash[newName] = frame;
+                    delete _frameHash[oldName];
+                    _frameHash[newName] = frame;
                 }
 
                 break;
             case 'insert':
                 frame = op.value;
-                _hash[frame.name] = frame;
+                _frameHash[frame.name] = frame;
                 _frames.splice(op.index, 0, frame);
                 break;
             case 'delete':
                 frame = _frames[op.index];
-                delete _hash[frame.name];
+                delete _frameHash[frame.name];
                 _frames.splice(op.index, 1);
                 break;
         }
@@ -2252,14 +2252,6 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
         var module = contFull._module;
 
         if(!module) continue;
-
-        if(!module.animatable) {
-            var thisUpdate = {};
-
-            for(var ai in data[i]) {
-                thisUpdate[ai] = [data[i][ai]];
-            }
-        }
     }
 
     var seq = [plots.previousPromises, interruptPreviousTransitions, prepareTransitions, plots.rehover, executeTransitions];

test/jasmine/tests/frame_api_test.js

@@ -37,6 +37,44 @@ describe('Test frame api', function() {
     });
 
     describe('#addFrames', function() {
+
+        it('issues a warning if a number-named frame would overwrite a frame', function(done) {
+            var warnings = [];
+            spyOn(Lib, 'warn').and.callFake(function(msg) {
+                warnings.push(msg);
+            });
+
+            // Test with both multiframe additions and repeated `addFrames` calls - both should count toward the warn limit
+            Plotly.addFrames(gd, [
+                {name: 8, data: [{x: [8, 7, 6]}]},
+                {name: 8888, data: [{x: [8, 7, 6]}]},
+                {name: 8, data: [{x: [8, 7, 6]}]},
+                {name: '8', data: [{x: [8, 7, 6]}]}
+            ])
+                .then(function() {
+                    // so far, two warnings
+                    expect(Lib.warn.calls.count()).toEqual(2);
+                    expect(warnings[0]).toMatch(/^addFrames.*overwriting/);
+                    return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);
+                })
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {
+                    // so far, 5 + 1 warnings
+                    expect(Lib.warn.calls.count()).toEqual(5 + 1);
+                    expect(warnings[5]).toMatch(/^addFrames.*suppressed/);
+                    return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);
+                })
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {return Plotly.addFrames(gd, [{name: 8, data: [{x: [3, 2, 1]}]}]);})
+                .then(function() {
+                    // Five (`var numericNameWarningCountLimit = 5`) warnings and one warning saying that there won't be more warnings
+                    expect(Lib.warn.calls.count()).toEqual(5 + 1);
+                }).catch(fail).then(done);
+        });
+
         it('treats an undefined list as a noop', function(done) {
             Plotly.addFrames(gd, undefined).then(function() {
                 expect(Object.keys(h)).toEqual([]);
@@ -102,20 +140,6 @@ describe('Test frame api', function() {
             }).catch(fail).then(done);
         });
 
-        it('issues a warning if a number-named frame would overwrite a frame', function(done) {
-            var warnings = [];
-            spyOn(Lib, 'warn').and.callFake(function(msg) {
-                warnings.push(msg);
-            });
-
-            Plotly.addFrames(gd, [{name: 2}]).then(function() {
-                return Plotly.addFrames(gd, [{name: 2, layout: {foo: 'bar'}}]);
-            }).then(function() {
-                expect(warnings.length).toEqual(1);
-                expect(warnings[0]).toMatch(/overwriting/);
-            }).catch(fail).then(done);
-        });
-
         it('avoids name collisions', function(done) {
             Plotly.addFrames(gd, [{name: 'frame 0'}, {name: 'frame 2'}]).then(function() {
                 expect(f).toEqual([{name: 'frame 0'}, {name: 'frame 2'}]);