diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 1a5bd8d1fc0..8d21c672bdd 100644 --- a/src/plot_api/plot_api.js +++ b/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,7 +2420,7 @@ 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++) { @@ -2424,10 +2428,10 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { 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: diff --git a/src/plots/command.js b/src/plots/command.js index b1d86fa23f8..41a60328447 100644 --- a/src/plots/command.js +++ b/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 []; } diff --git a/src/plots/plots.js b/src/plots/plots.js index 3ad048addd5..06f2e91aff5 100644 --- a/src/plots/plots.js +++ b/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; diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index 570a9e741f9..53eb151c01c 100644 --- a/test/jasmine/tests/animate_test.js +++ b/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); }); diff --git a/test/jasmine/tests/command_test.js b/test/jasmine/tests/command_test.js index 01725c22e39..808fe17dfc5 100644 --- a/test/jasmine/tests/command_test.js +++ b/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']]); diff --git a/test/jasmine/tests/frame_api_test.js b/test/jasmine/tests/frame_api_test.js index 9e006fa018d..495acbb6418 100644 --- a/test/jasmine/tests/frame_api_test.js +++ b/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'}]);