Skip to content

Common interface to interpret and execute API methods #1016

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 29 commits into from
Oct 25, 2016
Merged

Common interface to interpret and execute API methods #1016

Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
f2d268d
Start implementing command execution wrapper
rreusser Oct 4, 2016
65f618d
Write failing tests for binding computation
rreusser Oct 6, 2016
7ef0404
Add a lot of tests for binding computation
rreusser Oct 10, 2016
edb6773
Implement animate and update binding computation
rreusser Oct 10, 2016
f50fcaa
Switch return format of bindings to structured output
rreusser Oct 11, 2016
60dd634
Merge remote-tracking branch 'origin/master' into lib-commands
rreusser Oct 11, 2016
4bc8387
Command to decide when bindings are simple
rreusser Oct 17, 2016
93b8253
First working version of bindings
rreusser Oct 18, 2016
43dc3d4
Change the method name
rreusser Oct 18, 2016
65d24f3
Fix updatemenus bindings
rreusser Oct 18, 2016
d2696e7
Hook up animations to sliders via bindings
rreusser Oct 18, 2016
7db7f03
Clean up slider positioning code
rreusser Oct 20, 2016
57fcf96
Add mock for bindings
rreusser Oct 20, 2016
7db5f1f
emove custom plotmodified event
rreusser Oct 20, 2016
26ad1ff
Add binding baseline image
rreusser Oct 20, 2016
986b4dc
Fix irritating self-interaction when dragging slider
rreusser Oct 24, 2016
20ac69f
Fix lint issue
rreusser Oct 24, 2016
f993ed7
Change failure modes for command API
rreusser Oct 24, 2016
99d7c8e
Ugh linter again
rreusser Oct 24, 2016
f8c0094
Improve robustness of command bindings
rreusser Oct 24, 2016
6abec15
Test more behavior of bindings
rreusser Oct 24, 2016
5df94a7
Fix linter issue
rreusser Oct 24, 2016
d35ee35
Remove binding test file
rreusser Oct 24, 2016
a554cad
Add equivalent command API test for udpatemenus
rreusser Oct 24, 2016
e5a80ee
DRY up binding change check
rreusser Oct 24, 2016
45717b1
Add note about test failure
rreusser Oct 24, 2016
52de9e4
createCommandObserver --> manageCommandObserver
rreusser Oct 25, 2016
0c40b02
Remove hard-coded updatemenus active default
rreusser Oct 25, 2016
df2d5bb
Revert updatemenus initialization and fix sliders initialization
rreusser Oct 25, 2016
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
34 changes: 20 additions & 14 deletions src/components/sliders/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

var d3 = require('d3');

var Plotly = require('../../plotly');
var Plots = require('../../plots/plots');
var Lib = require('../../lib');
var Color = require('../color');
Expand Down Expand Up @@ -52,6 +51,9 @@ module.exports = function draw(gd) {
sliderGroups.exit().each(function(sliderOpts) {
d3.select(this).remove();

sliderOpts._commandObserver.remove();
delete sliderOpts._commandObserver;

Plots.autoMargin(gd, constants.autoMarginIdRoot + sliderOpts._index);
});

Expand All @@ -65,12 +67,20 @@ module.exports = function draw(gd) {
// If it has fewer than two options, it's not really a slider:
if(sliderOpts.steps.length < 2) return;

var gSlider = d3.select(this);

computeLabelSteps(sliderOpts);

Plots.createCommandObserver(gd, sliderOpts, sliderOpts.steps, function(data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm big a fan of this new pattern here where one routine creates + updates command observer. This should make the command observer logic more easily reused in the future when we integrate it in other components 👍

One small suggestion, now that createCommandObsever can create and update a command observer, maybe we should rename it something like manageCommandObserver similar to manageModebar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I was going to go with createOrUpdateCommandObserver, but that seemed excessive. Changing now.

if(sliderOpts.active === data.index) return;
if(sliderOpts._dragging) return;

setActive(gd, gSlider, sliderOpts, data.index, false, true);
});

drawSlider(gd, d3.select(this), sliderOpts);

// makeInputProxy(gd, d3.select(this), sliderOpts);

});
};

Expand Down Expand Up @@ -371,19 +381,9 @@ function setActive(gd, sliderGroup, sliderOpts, index, doCallback, doTransition)
sliderGroup._nextMethod = {step: step, doCallback: doCallback, doTransition: doTransition};
sliderGroup._nextMethodRaf = window.requestAnimationFrame(function() {
var _step = sliderGroup._nextMethod.step;
var args = _step.args;
if(!_step.method) return;

sliderOpts._invokingCommand = true;
Plotly[_step.method](gd, args[0], args[1], args[2]).then(function() {
sliderOpts._invokingCommand = false;
}, function() {
sliderOpts._invokingCommand = false;

// This is not a disaster. Some methods like `animate` reject if interrupted
// and *should* nicely log a warning.
Lib.warn('Warning: Plotly.' + _step.method + ' was called and rejected.');
});
Plots.executeAPICommand(gd, _step.method, _step.args);

sliderGroup._nextMethod = null;
sliderGroup._nextMethodRaf = null;
Expand All @@ -405,13 +405,15 @@ function attachGripEvents(item, gd, sliderGroup, sliderOpts) {

var normalizedPosition = positionToNormalizedValue(sliderOpts, d3.mouse(node)[0]);
handleInput(gd, sliderGroup, sliderOpts, normalizedPosition, true);
sliderOpts._dragging = true;

$gd.on('mousemove', function() {
var normalizedPosition = positionToNormalizedValue(sliderOpts, d3.mouse(node)[0]);
handleInput(gd, sliderGroup, sliderOpts, normalizedPosition, false);
});

$gd.on('mouseup', function() {
sliderOpts._dragging = false;
grip.call(Color.fill, sliderOpts.bgcolor);
$gd.on('mouseup', null);
$gd.on('mousemove', null);
Expand Down Expand Up @@ -467,8 +469,12 @@ function setGripPosition(sliderGroup, sliderOpts, position, doTransition) {

var x = normalizedValueToPosition(sliderOpts, position);

// If this is true, then *this component* is already invoking its own command
// and has triggered its own animation.
if(sliderOpts._invokingCommand) return;

var el = grip;
if(doTransition && sliderOpts.transition.duration > 0 && !sliderOpts._invokingCommand) {
if(doTransition && sliderOpts.transition.duration > 0) {
el = el.transition()
.duration(sliderOpts.transition.duration)
.ease(sliderOpts.transition.easing);
Expand Down
6 changes: 5 additions & 1 deletion src/components/updatemenus/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ module.exports = function updateMenusDefaults(layoutIn, layoutOut) {
// used to determine object constancy
menuOut._index = i;

menuOut._input.active = menuOut.active;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, there's an intermediate state in which this value is updated when changes occur, but is undefined before that.

I ran into this while testing: I checked this to see if the external value was updated when component interactions occurred. That worked fine, but before changes were made, the value was undefined, which was perhaps desired but did not reflect the actual state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this line.


contOut.push(menuOut);
}
};
Expand All @@ -48,7 +50,9 @@ function menuDefaults(menuIn, menuOut, layoutOut) {
var visible = coerce('visible', buttons.length > 0);
if(!visible) return;

coerce('active');
// Default to zero since active must be *something*:
Copy link
Contributor

Choose a reason for hiding this comment

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

If that's the case, we should add a dflt in the attribute declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. That works for me. I think that's best since undefined isn't a state that really makes sense (because it's always something).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. The default is set. Removing this. I think this may have been a change that I thought was necessary while debugging but ultimately did not need.

Copy link
Contributor

@etpinard etpinard Oct 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm. Hold on. The updatemenu active was first designed to correspond to the last-clicked button (with no notion of data / layout state). So, on first-render active is undefined meaning that no buttons have been clicked on yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay. Let me adjust on my end then. That makes sense. I guess it just bothered me that the first item is selected when the chart loads but the variable is not set.

Copy link
Contributor Author

@rreusser rreusser Oct 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Reverted updatemenus change and fixed sliders initialization + defaults to match the behavior.

coerce('active', 0);

coerce('direction');
coerce('type');
coerce('showactive');
Expand Down
39 changes: 23 additions & 16 deletions src/components/updatemenus/draw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

var d3 = require('d3');

var Plotly = require('../../plotly');
var Plots = require('../../plots/plots');
var Lib = require('../../lib');
var Color = require('../color');
Expand All @@ -21,7 +20,6 @@ var anchorUtils = require('../legend/anchor_utils');

var constants = require('./constants');


module.exports = function draw(gd) {
var fullLayout = gd._fullLayout,
menuData = makeMenuData(fullLayout);
Expand Down Expand Up @@ -115,6 +113,11 @@ module.exports = function draw(gd) {
headerGroups.each(function(menuOpts) {
var gHeader = d3.select(this);

var _gButton = menuOpts.type === 'dropdown' ? gButton : null;
Plots.createCommandObserver(gd, menuOpts, menuOpts.buttons, function(data) {
setActive(gd, menuOpts, menuOpts.buttons[data.index], gHeader, _gButton, data.index, true);
});

if(menuOpts.type === 'dropdown') {
drawHeader(gd, gHeader, gButton, menuOpts);

Expand Down Expand Up @@ -293,21 +296,9 @@ function drawButtons(gd, gHeader, gButton, menuOpts) {
.call(setItemPosition, menuOpts, posOpts);

button.on('click', function() {
// update 'active' attribute in menuOpts
menuOpts._input.active = menuOpts.active = buttonIndex;

// fold up buttons and redraw header
gButton.attr(constants.menuIndexAttrName, '-1');
setActive(gd, menuOpts, buttonOpts, gHeader, gButton, buttonIndex);

if(menuOpts.type === 'dropdown') {
drawHeader(gd, gHeader, gButton, menuOpts);
}

drawButtons(gd, gHeader, gButton, menuOpts);

// call button method
var args = buttonOpts.args;
Plotly[buttonOpts.method](gd, args[0], args[1], args[2]);
Plots.executeAPICommand(gd, buttonOpts.method, buttonOpts.args);
Copy link
Contributor

Choose a reason for hiding this comment

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

much cleaner 🎉

});

button.on('mouseover', function() {
Expand All @@ -326,6 +317,22 @@ function drawButtons(gd, gHeader, gButton, menuOpts) {
Lib.setTranslate(gButton, menuOpts.lx, menuOpts.ly);
}

function setActive(gd, menuOpts, buttonOpts, gHeader, gButton, buttonIndex, isSilentUpdate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice abstraction

// update 'active' attribute in menuOpts
menuOpts._input.active = menuOpts.active = buttonIndex;

if(menuOpts.type === 'dropdown') {
// fold up buttons and redraw header
gButton.attr(constants.menuIndexAttrName, '-1');

drawHeader(gd, gHeader, gButton, menuOpts);
}

if(!isSilentUpdate || menuOpts.type === 'buttons') {
drawButtons(gd, gHeader, gButton, menuOpts);
}
}

function drawItem(item, menuOpts, itemOpts) {
item.call(drawItemRect, menuOpts)
.call(drawItemText, menuOpts, itemOpts);
Expand Down
18 changes: 10 additions & 8 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2302,14 +2302,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
var newFrame = trans._currentFrame = trans._frameQueue.shift();

if(newFrame) {
gd.emit('plotly_animatingframe', {
name: newFrame.name,
frame: newFrame.frame,
animation: {
frame: newFrame.frameOpts,
transition: newFrame.transitionOpts,
}
});
gd._fullLayout._currentFrame = newFrame.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice. I'll use that for layout.breakpoints


trans._lastFrameAt = Date.now();
trans._timeToNext = newFrame.frameOpts.duration;
Expand All @@ -2324,6 +2317,15 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
newFrame.frameOpts,
newFrame.transitionOpts
);

gd.emit('plotly_animatingframe', {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rreusser can you share some info about why the emit('plotly_animatingframe') changed location?

Copy link
Contributor Author

@rreusser rreusser Oct 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

There are a lot of things taking place synchronously and asynchronously so that I wanted to move the event trigger as late as possible to ensure the changes have actually taken place in order to be picked up by bound components. I think maybe a better long-term solution is an event after Plots.transition has supplied defaults (coming from that method itself to ensure correctness) in order to ensure that the changes have absolutely 100% certainly been applied.

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 info 👍

name: newFrame.name,
frame: newFrame.frame,
animation: {
frame: newFrame.frameOpts,
transition: newFrame.transitionOpts,
}
});
} else {
// If there are no more frames, then stop the RAF loop:
stopAnimationLoop();
Expand Down
Loading