Merge pull request #1236 from plotly/fix-number-named-frames

Fix number named frames

Author: rreusser

src/plot_api/plot_api.js

@@ -2327,7 +2327,11 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
             var newFrame = trans._currentFrame = trans._frameQueue.shift();
 
             if(newFrame) {
-                gd._fullLayout._currentFrame = newFrame.name;
+                // Since it's sometimes necessary to do deep digging into frame data,
+                // we'll consider it not 100% impossible for nulls or numbers to sneak through,
+                // so check when casting the name, just to be absolutely certain:
+                var stringName = newFrame.name ? newFrame.name.toString() : null;
+                gd._fullLayout._currentFrame = stringName;
 
                 trans._lastFrameAt = Date.now();
                 trans._timeToNext = newFrame.frameOpts.duration;
@@ -2349,7 +2353,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                 });
 
                 gd.emit('plotly_animatingframe', {
-                    name: newFrame.name,
+                    name: stringName,
                     frame: newFrame.frame,
                     animation: {
                         frame: newFrame.frameOpts,
@@ -2416,18 +2420,18 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                 type: 'object',
                 data: setTransitionConfig(Lib.extendFlat({}, frameOrGroupNameOrFrameList))
             });
-        } else if(allFrames || typeof frameOrGroupNameOrFrameList === 'string') {
+        } else if(allFrames || ['string', 'number'].indexOf(typeof frameOrGroupNameOrFrameList) !== -1) {
             // In this case, null or undefined has been passed so that we want to
             // animate *all* currently defined frames
             for(i = 0; i < trans._frames.length; i++) {
                 frame = trans._frames[i];
 
                 if(!frame) continue;
 
-                if(allFrames || frame.group === frameOrGroupNameOrFrameList) {
+                if(allFrames || String(frame.group) === String(frameOrGroupNameOrFrameList)) {
                     frameList.push({
                         type: 'byname',
-                        name: frame.name,
+                        name: String(frame.name),
                         data: setTransitionConfig({name: frame.name})
                     });
                 }
@@ -2528,6 +2532,8 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
 Plotly.addFrames = function(gd, frameList, indices) {
     gd = helpers.getGraphDiv(gd);
 
+    var numericNameWarningCount = 0;
+
     if(frameList === null || frameList === undefined) {
         return Promise.resolve();
     }
@@ -2558,6 +2564,25 @@ Plotly.addFrames = function(gd, frameList, indices) {
 
     var insertions = [];
     for(i = frameList.length - 1; i >= 0; i--) {
+        var name = (_hash[frameList[i].name] || {}).name;
+        var newName = frameList[i].name;
+
+        if(name && newName && typeof newName === 'number' && _hash[name]) {
+            numericNameWarningCount++;
+
+            Lib.warn('addFrames: overwriting frame "' + _hash[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. ' +
+                    'For the rest of this call, further warnings about numeric frame ' +
+                    'names will be suppressed.');
+            }
+        }
+
         insertions.push({
             frame: Plots.supplyFrameDefaults(frameList[i]),
             index: (indices && indices[i] !== undefined && indices[i] !== null) ? indices[i] : bigIndex + i
@@ -2578,6 +2603,12 @@ Plotly.addFrames = function(gd, frameList, indices) {
     for(i = insertions.length - 1; i >= 0; i--) {
         frame = insertions[i].frame;
 
+        if(typeof frame.name === 'number') {
+            Lib.warn('Warning: addFrames accepts frames with numeric names, but the numbers are' +
+                'implicitly cast to strings');
+
+        }
+
         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:

src/plots/command.js

@@ -301,8 +301,8 @@ exports.computeAPICommandBindings = function(gd, method, args) {
 function computeAnimateBindings(gd, args) {
     // We'll assume that the only relevant modification an animation
     // makes that's meaningfully tracked is the frame:
-    if(Array.isArray(args[0]) && args[0].length === 1 && typeof args[0][0] === 'string') {
-        return [{type: 'layout', prop: '_currentFrame', value: args[0][0]}];
+    if(Array.isArray(args[0]) && args[0].length === 1 && ['string', 'number'].indexOf(typeof args[0][0]) !== -1) {
+        return [{type: 'layout', prop: '_currentFrame', value: args[0][0].toString()}];
     } else {
         return [];
     }

src/plots/plots.js

@@ -1478,7 +1478,17 @@ plots.computeFrame = function(gd, frameName) {
     var frameLookup = gd._transitionData._frameHash;
     var i, traceIndices, traceIndex, destIndex;
 
-    var framePtr = frameLookup[frameName];
+    // Null or undefined will fail on .toString(). We'll allow numbers since we
+    // make it clear frames must be given string names, but we'll allow numbers
+    // here since they're otherwise fine for looking up frames as long as they're
+    // properly cast to strings. We really just want to ensure here that this
+    // 1) doesn't fail, and
+    // 2) doens't give an incorrect answer (which String(frameName) would)
+    if(!frameName) {
+        throw new Error('computeFrame must be given a string frame name');
+    }
+
+    var framePtr = frameLookup[frameName.toString()];
 
     // Return false if the name is invalid:
     if(!framePtr) {
@@ -1489,7 +1499,7 @@ plots.computeFrame = function(gd, frameName) {
     var frameNameStack = [framePtr.name];
 
     // Follow frame pointers:
-    while((framePtr = frameLookup[framePtr.baseframe])) {
+    while(framePtr.baseframe && (framePtr = frameLookup[framePtr.baseframe.toString()])) {
         // Avoid infinite loops:
         if(frameNameStack.indexOf(framePtr.name) !== -1) break;
 

test/jasmine/tests/animate_test.js

@@ -549,7 +549,7 @@ describe('Test animate API', function() {
                 transition: {duration: 1},
                 frame: {duration: 1}
             }).then(function() {
-                expect(frames).toEqual(['frame0', 'frame1', undefined, undefined]);
+                expect(frames).toEqual(['frame0', 'frame1', null, null]);
             }).catch(fail).then(done);
 
         });

test/jasmine/tests/command_test.js

@@ -444,6 +444,12 @@ describe('Plots.computeAPICommandBindings', function() {
             expect(result).toEqual([{type: 'layout', prop: '_currentFrame', value: 'framename'}]);
         });
 
+        it('treats numeric frame names as strings', function() {
+            var result = Plots.computeAPICommandBindings(gd, 'animate', [[8]]);
+
+            expect(result).toEqual([{type: 'layout', prop: '_currentFrame', value: '8'}]);
+        });
+
         it('binds to nothing for a multi-frame animate command', function() {
             var result = Plots.computeAPICommandBindings(gd, 'animate', [['frame1', 'frame2']]);
 

test/jasmine/tests/frame_api_test.js

@@ -1,4 +1,5 @@
 var Plotly = require('@lib/index');
+var Lib = require('@src/lib');
 
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
@@ -73,13 +74,41 @@ describe('Test frame api', function() {
         });
 
         it('creates multiple unnamed frames in series', function(done) {
-            Plotly.addFrames(gd, [{}]).then(
-                Plotly.addFrames(gd, [{}])
-            ).then(function() {
+            Plotly.addFrames(gd, [{}]).then(function() {
+                return Plotly.addFrames(gd, [{}]);
+            }).then(function() {
                 expect(f).toEqual([{name: 'frame 0'}, {name: 'frame 1'}]);
             }).catch(fail).then(done);
         });
 
+        it('casts number names to strings on insertion', function(done) {
+            Plotly.addFrames(gd, [{name: 2}]).then(function() {
+                expect(f).toEqual([{name: '2'}]);
+            }).catch(fail).then(done);
+        });
+
+        it('updates frames referenced by number', function(done) {
+            Plotly.addFrames(gd, [{name: 2}]).then(function() {
+                return Plotly.addFrames(gd, [{name: 2, layout: {foo: 'bar'}}]);
+            }).then(function() {
+                expect(f).toEqual([{name: '2', layout: {foo: 'bar'}}]);
+            }).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'}]);