diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 82c815a9fee..644251611c4 100644 --- a/src/plot_api/plot_api.js +++ b/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,7 +2594,7 @@ 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. @@ -2600,8 +2602,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { 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; diff --git a/src/plots/plots.js b/src/plots/plots.js index 0c6dd3bac81..32c16ac1f3e 100644 --- a/src/plots/plots.js +++ b/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]; diff --git a/test/jasmine/tests/frame_api_test.js b/test/jasmine/tests/frame_api_test.js index 9d36144637e..bcd9bee1bd5 100644 --- a/test/jasmine/tests/frame_api_test.js +++ b/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'}]);