From 09924a6123c0ba6b7ee0c7f96df22f58d75eec40 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Mon, 29 Jan 2018 21:15:46 +0100 Subject: [PATCH 1/6] minor: stop emitting warnings after having reached a count --- src/plot_api/plot_api.js | 19 ++++++++++--------- src/plots/plots.js | 16 ++++++++-------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 82c815a9fee..5a9d5860724 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2592,7 +2592,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. @@ -2601,6 +2601,7 @@ Plotly.addFrames = function(gd, frameList, indices) { gd = Lib.getGraphDiv(gd); var numericNameWarningCount = 0; + var numericNameWarningCountLimit = 5; if(frameList === null || frameList === undefined) { return Promise.resolve(); @@ -2616,7 +2617,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)) { @@ -2634,20 +2635,20 @@ Plotly.addFrames = function(gd, frameList, indices) { for(i = frameList.length - 1; i >= 0; i--) { if(!Lib.isPlainObject(frameList[i])) continue; - var name = (_hash[frameList[i].name] || {}).name; + var name = (_frameHash[frameList[i].name] || {}).name; var newName = frameList[i].name; - if(name && newName && typeof newName === 'number' && _hash[name]) { + if(name && newName && typeof newName === 'number' && _frameHash[name] && numericNameWarningCount < numericNameWarningCountLimit) { numericNameWarningCount++; - Lib.warn('addFrames: overwriting frame "' + _hash[name].name + + Lib.warn('addFrames: overwriting frame "' + _frameHash[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.'); } @@ -2682,10 +2683,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 62dc4776139..3d52fd4b2c5 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; } From 6efb0a709aef6a20f287f2320c2e9b92a24e1fc8 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Tue, 30 Jan 2018 14:07:35 +0100 Subject: [PATCH 2/6] minor: deleting code block that doesn't seem to have a current purpose --- src/plots/plots.js | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 3d52fd4b2c5..90a22d59cf6 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -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]; From 6d4c86b77d41a4cdc00b47074fe32cce5546ab47 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Sun, 4 Feb 2018 07:39:37 +0100 Subject: [PATCH 3/6] making the warning invocation counting global, rather than per `addFrame` --- src/plot_api/plot_api.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 5a9d5860724..5eaae7d75f4 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 @@ -2600,9 +2602,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) { Plotly.addFrames = function(gd, frameList, indices) { gd = Lib.getGraphDiv(gd); - var numericNameWarningCount = 0; - var numericNameWarningCountLimit = 5; - if(frameList === null || frameList === undefined) { return Promise.resolve(); } From 1fb2ed1216b90a0bed7ad403be05a26dc2e9531e Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Sun, 4 Feb 2018 07:39:56 +0100 Subject: [PATCH 4/6] test case for warning invocations --- test/jasmine/tests/animate_test.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index a8dca8d0b53..54f473dce26 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -682,6 +682,25 @@ describe('Animate API details', function() { }).catch(fail).then(done); }); + it('emits warning if strings are not used and this creates ambiguity', function(done) { + spyOn(Lib, 'warn'); + Plotly.addFrames(gd, [{name: '8', data: [{x: [8, 7, 6]}]}]) + .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() {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() {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('ignores null and undefined frames', function(done) { var cnt = 0; gd.on('plotly_animatingframe', function() {cnt++;}); From b1e538db3954cbd1741e174b2f45a4040ecfe17b Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Sun, 4 Feb 2018 08:18:04 +0100 Subject: [PATCH 5/6] Check for numeric - text name collisions within frames of a single `addFrames` call too --- src/plot_api/plot_api.js | 13 ++++++++++--- test/jasmine/tests/animate_test.js | 23 ++++++++++++++++++----- 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 5eaae7d75f4..644251611c4 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -2631,16 +2631,21 @@ 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 = (_frameHash[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' && _frameHash[name] && numericNameWarningCount < numericNameWarningCountLimit) { + if(name && newName && typeof newName === 'number' && collisionPresent && numericNameWarningCount < numericNameWarningCountLimit) { numericNameWarningCount++; - Lib.warn('addFrames: overwriting frame "' + _frameHash[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 ' + @@ -2653,6 +2658,8 @@ Plotly.addFrames = function(gd, frameList, indices) { } } + _frameHashLocal[lookupName] = {name: lookupName}; + insertions.push({ frame: Plots.supplyFrameDefaults(frameList[i]), index: (indices && indices[i] !== undefined && indices[i] !== null) ? indices[i] : bigIndex + i diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index fc63f95753d..c86f5952c50 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -684,13 +684,26 @@ describe('Animate API details', function() { it('emits warning if strings are not used and this creates ambiguity', function(done) { spyOn(Lib, 'warn'); - Plotly.addFrames(gd, [{name: '8', data: [{x: [8, 7, 6]}]}]) - .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]}]}]);}) + + // 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); + 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); + 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]}]}]);}) From 1ccf35cfdec10fafa56ee1c4f5890eb6db78fb85 Mon Sep 17 00:00:00 2001 From: Robert Monfera Date: Sun, 4 Feb 2018 09:00:04 +0100 Subject: [PATCH 6/6] Moved test to frame_api_test.js --- test/jasmine/tests/animate_test.js | 32 ----------------- test/jasmine/tests/frame_api_test.js | 52 ++++++++++++++++++++-------- 2 files changed, 38 insertions(+), 46 deletions(-) diff --git a/test/jasmine/tests/animate_test.js b/test/jasmine/tests/animate_test.js index c86f5952c50..a8dca8d0b53 100644 --- a/test/jasmine/tests/animate_test.js +++ b/test/jasmine/tests/animate_test.js @@ -682,38 +682,6 @@ describe('Animate API details', function() { }).catch(fail).then(done); }); - it('emits warning if strings are not used and this creates ambiguity', function(done) { - spyOn(Lib, 'warn'); - - // 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); - 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); - 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('ignores null and undefined frames', function(done) { var cnt = 0; gd.on('plotly_animatingframe', function() {cnt++;}); 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'}]);