Skip to content

Enforce casting requested frame names to strings #1124

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.

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 1 commit into from
Nov 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions src/plot_api/plot_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -2258,7 +2258,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
frameOpts: frameOpts,
transitionOpts: transitionOpts,
};

if(i === frameList.length - 1) {
// The last frame in this .animate call stores the promise resolve
// and reject callbacks. This is how we ensure that the animation
Expand Down Expand Up @@ -2410,14 +2409,15 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
} else if(isFrameArray) {
for(i = 0; i < frameOrGroupNameOrFrameList.length; i++) {
var frameOrName = frameOrGroupNameOrFrameList[i];
if(typeof frameOrName === 'string') {
if(['number', 'string'].indexOf(typeof frameOrName) !== -1) {
frameOrName = String(frameOrName);
// In this case, there's an array and this frame is a string name:
frameList.push({
type: 'byname',
name: frameOrName,
data: setTransitionConfig({name: frameOrName})
});
} else {
} else if(Lib.isPlainObject(frameOrName)) {
frameList.push({
type: 'object',
data: setTransitionConfig(Lib.extendFlat({}, frameOrName))
Expand Down
6 changes: 6 additions & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -1798,6 +1798,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
}

function completeTransition(callback) {
// Fail-safe against purged plot:
if(!gd._transitionData) return;

flushCallbacks(gd._transitionData._interruptCallbacks);

return Promise.resolve().then(function() {
Expand All @@ -1815,6 +1818,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
}

function interruptPreviousTransitions() {
// Fail-safe against purged plot:
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for the comment here.

Copy link
Contributor Author

@rreusser rreusser Nov 9, 2016

Choose a reason for hiding this comment

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

This could go elsewhere so that no failsafe is needed, but it's a trivially inexpensive one-liner, so I think it's a good idea.

if(!gd._transitionData) return;

// If a transition is interrupted, set this to false. At the moment, the only thing that would
// interrupt a transition is another transition, so that it will momentarily be set to true
// again, but this determines whether autorange or dragbox work, so it's for the sake of
Expand Down
53 changes: 53 additions & 0 deletions test/jasmine/tests/animate_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,14 @@ describe('Test animate API', function() {

mockCopy = Lib.extendDeep({}, mock);

// ------------------------------------------------------------
// NB: TRANSITION IS FAKED
//
// This means that you should not expect `.animate` to actually
// modify the plot in any way in the tests below. For tests
// involvingnon-faked transitions, see the bottom of this file.
// ------------------------------------------------------------

spyOn(Plots, 'transition').and.callFake(function() {
// Transition's fake behavior is just to delay by the duration
// and resolve:
Expand Down Expand Up @@ -579,3 +587,48 @@ describe('Test animate API', function() {
});
});
});

describe('Test animate API', function() {
'use strict';

var gd, mockCopy;

beforeEach(function(done) {
gd = createGraphDiv();
mockCopy = Lib.extendDeep({}, mock);
Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why use the two-argument signature here? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

historical reasons!

return Plotly.addFrames(gd, mockCopy.frames);
}).then(done);
});

afterEach(function() {
Plotly.purge(gd);
destroyGraphDiv();
});

it('does not fail if strings are not used', function(done) {
Plotly.addFrames(gd, [{name: 8, data: [{x: [8, 7, 6]}]}]).then(function() {
// Verify it was added as a string name:
expect(gd._transitionData._frameHash['8']).not.toBeUndefined();

// Transition using a number:
return Plotly.animate(gd, [8], {transition: {duration: 0}, frame: {duration: 0}});
}).then(function() {
// Confirm the result:
expect(gd.data[0].x).toEqual([8, 7, 6]);
}).catch(fail).then(done);
});

it('ignores null and undefined frames', function(done) {
var cnt = 0;
gd.on('plotly_animatingframe', function() {cnt++;});

Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}}).then(function() {
// Check only one animating was fired:
expect(cnt).toEqual(1);

// Check unused frames did not affect the current frame:
expect(gd._fullLayout._currentFrame).toEqual('frame0');
}).catch(fail).then(done);
});
});
6 changes: 6 additions & 0 deletions test/jasmine/tests/frame_api_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ describe('Test frame api', function() {
}).catch(fail).then(done);
});

it('casts names to strings', function(done) {
Plotly.addFrames(gd, [{name: 5}]).then(function() {
expect(Object.keys(h)).toEqual(['5']);
}).catch(fail).then(done);
});

it('creates multiple unnamed frames at the same time', function(done) {
Plotly.addFrames(gd, [{}, {}]).then(function() {
expect(f).toEqual([{name: 'frame 0'}, {name: 'frame 1'}]);
Expand Down