From f0ff634cbbcc4c38b646e60c5119b2d44c602d4d Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 7 Dec 2016 14:47:47 -0500 Subject: [PATCH 1/4] Basic fixes for making number-named frames work better --- src/plot_api/plot_api.js | 21 ++++++++++++++--- src/plots/command.js | 4 ++-- src/plots/plots.js | 14 +++++++++-- test/jasmine/tests/frame_api_test.js | 35 +++++++++++++++++++++++++--- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 1a5bd8d1fc0..e285a0bd6f6 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2416,7 +2416,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 +2424,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 +2528,8 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { Plotly.addFrames = function(gd, frameList, indices) { gd = helpers.getGraphDiv(gd); + var hasBeenWarnedAboutNumericNames = false; + if(frameList === null || frameList === undefined) { return Promise.resolve(); } @@ -2558,6 +2560,13 @@ 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]) { + 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.'); + } + insertions.push({ frame: Plots.supplyFrameDefaults(frameList[i]), index: (indices && indices[i] !== undefined && indices[i] !== null) ? indices[i] : bigIndex + i @@ -2578,6 +2587,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..05c697e70bf 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/frame_api_test.js b/test/jasmine/tests/frame_api_test.js index 9e006fa018d..9340a539178 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'}]); From 14f4874a94437414ae0a6905422830bd59e568ce Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 7 Dec 2016 14:52:50 -0500 Subject: [PATCH 2/4] Prevent crazy excessive warnings, just in case --- src/plot_api/plot_api.js | 20 ++++++++++++++++---- src/plots/plots.js | 4 ++-- test/jasmine/tests/frame_api_test.js | 22 +++++++++++----------- 3 files changed, 29 insertions(+), 17 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index e285a0bd6f6..a0a7952d3ed 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2528,7 +2528,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { Plotly.addFrames = function(gd, frameList, indices) { gd = helpers.getGraphDiv(gd); - var hasBeenWarnedAboutNumericNames = false; + var numericNameWarningCount = 0; if(frameList === null || frameList === undefined) { return Promise.resolve(); @@ -2563,8 +2563,20 @@ Plotly.addFrames = function(gd, frameList, indices) { var name = (_hash[frameList[i].name] || {}).name; var newName = frameList[i].name; - if (name && newName && typeof newName === 'number' && _hash[name]) { - 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(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({ @@ -2587,7 +2599,7 @@ Plotly.addFrames = function(gd, frameList, indices) { for(i = insertions.length - 1; i >= 0; i--) { frame = insertions[i].frame; - if (typeof frame.name === 'number') { + if(typeof frame.name === 'number') { Lib.warn('Warning: addFrames accepts frames with numeric names, but the numbers are' + 'implicitly cast to strings'); diff --git a/src/plots/plots.js b/src/plots/plots.js index 05c697e70bf..06f2e91aff5 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -1484,8 +1484,8 @@ plots.computeFrame = function(gd, frameName) { // 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') + if(!frameName) { + throw new Error('computeFrame must be given a string frame name'); } var framePtr = frameLookup[frameName.toString()]; diff --git a/test/jasmine/tests/frame_api_test.js b/test/jasmine/tests/frame_api_test.js index 9340a539178..495acbb6418 100644 --- a/test/jasmine/tests/frame_api_test.js +++ b/test/jasmine/tests/frame_api_test.js @@ -74,36 +74,36 @@ describe('Test frame api', function() { }); it('creates multiple unnamed frames in series', function(done) { - Plotly.addFrames(gd, [{}]).then(function () { - return Plotly.addFrames(gd, [{}]) + 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 () { + 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 () { + 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 () { + }).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) { + it('issues a warning if a number-named frame would overwrite a frame', function(done) { var warnings = []; - spyOn(Lib, 'warn').and.callFake(function (msg){ + spyOn(Lib, 'warn').and.callFake(function(msg) { warnings.push(msg); }); - Plotly.addFrames(gd, [{name: 2}]).then(function () { + Plotly.addFrames(gd, [{name: 2}]).then(function() { return Plotly.addFrames(gd, [{name: 2, layout: {foo: 'bar'}}]); - }).then(function () { + }).then(function() { expect(warnings.length).toEqual(1); expect(warnings[0]).toMatch(/overwriting/); }).catch(fail).then(done); From 7aca3c4ec4741909c28037f19261f0ea5fa09161 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 7 Dec 2016 15:02:50 -0500 Subject: [PATCH 3/4] Add additional casting --- src/plot_api/plot_api.js | 8 ++++++-- test/jasmine/tests/command_test.js | 6 ++++++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index a0a7952d3ed..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, 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']]); From 0b3bdca4e3a2e4b86af39fe6597ce58f830399d2 Mon Sep 17 00:00:00 2001 From: Ricky Reusser Date: Wed, 7 Dec 2016 15:21:11 -0500 Subject: [PATCH 4/4] Fix null/undefined frame assumption --- test/jasmine/tests/animate_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); });