Skip to content

Minor changes to animation code ahead of the array animation fix #2313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Feb 5, 2018
Merged

Minor changes to animation code ahead of the array animation fix #2313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09924a6
minor: stop emitting warnings after having reached a count
monfera Jan 29, 2018
6efb0a7
minor: deleting code block that doesn't seem to have a current purpose
monfera Jan 30, 2018
6d4c86b
making the warning invocation counting global, rather than per `addFr…
monfera Feb 4, 2018
1fb2ed1
test case for warning invocations
monfera Feb 4, 2018
a666251
Merge branch 'master' into array-anim
monfera Feb 4, 2018
b1e538d
Check for numeric - text name collisions within frames of a single `a…
monfera Feb 4, 2018
1ccf35c
Moved test to frame_api_test.js
monfera Feb 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -2592,16 +2594,14 @@ 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.
*/
Plotly.addFrames = function(gd, frameList, indices) {
gd = Lib.getGraphDiv(gd);

var numericNameWarningCount = 0;

if(frameList === null || frameList === undefined) {
return Promise.resolve();
}
Expand All @@ -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)) {
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why === instead of > ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation of 1st commit: it looked to me as though Lib.warn continued to be invoked past the 5 warnings, because I didn't see an actual condition that'd bypass this code path, so I added an extra condition in the if line. With the commit, if numericNameWarningCount >= numericNameWarningCountLimit then it doesn't descend in the if, so it won't warn there anymore. The condition for raising the "Too many warnings" is an equality check, because numericNameWarningCount is initially 0 but gets incremented past the condition check ie. before this check. If numericNameWarningCount is 5 here, then we've been through this conditional branch 5 times (with numericNameWarningCount values 1, 2, 3, 4, 5); we emit the "too many warning" things and won't come into this conditional branch again. Hope I got it right but maybe you're seeing an off-by-one error?

Copy link
Contributor

@etpinard etpinard Feb 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. That makes sense. Can we add a test for this? Using jasmine spyOn(Lib, 'warn') should make this task relatively easy.

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
Expand Down Expand Up @@ -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;
Expand Down
24 changes: 8 additions & 16 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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;
}
Expand Down Expand Up @@ -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]];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange. I wonder what Ricky had in mind here. Good catch 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could've been just some initial logic that remained there.

}
}
}

var seq = [plots.previousPromises, interruptPreviousTransitions, prepareTransitions, plots.rehover, executeTransitions];
Expand Down
52 changes: 38 additions & 14 deletions test/jasmine/tests/frame_api_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent test. Thanks!

});

it('treats an undefined list as a noop', function(done) {
Plotly.addFrames(gd, undefined).then(function() {
expect(Object.keys(h)).toEqual([]);
Expand Down Expand Up @@ -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'}]);
Expand Down