Common interface to interpret and execute API methods #1016

rreusser · 2016-10-06T15:40:38Z

This PR works toward a common interface for executing commands. This includes:

Plots.executeAPICommand(gd, method, args) which executes a command (i.e. from components)
Plots.computeAPICommandBindings(gd, method, args) which interprets the intent of a command and returns a list of properties which affect the value of the component.
cache and check values for bound values

Completion of this feature will complete the implementation of sliders (#986) with two-way binding and will be applicable to other components.

rreusser · 2016-10-18T15:48:42Z

@etpinard @alexcjohnson The code isn't ready for review yet, but the internal usage is:

To hook this up, you provide it the gd, an array of commands (e.g.

[{method: 'restyle', args: [...]}, {method: 'restyle', args: [...]}, ...]

and a callback to make an update. If the commands are straightforward, it will create lookup table by value so that when a restyle or relayout comes through, it can easily check to see if the value has changed (via _fullLayout or _fullData) and execute the callback if there appears to be a matching component state.

From sliders:

if(!sliderOpts._commandObserver) {
  sliderOpts._commandObserver = Plots.createCommandObserver(gd, sliderOpts.steps, function(data) {
    setActive(gd, d3.select(this), sliderOpts, data.index, false, true);
  }.bind(this));
}

rreusser · 2016-10-18T15:52:37Z

(oh, and there's also a sliderOpts._commandObserver.remove() that needs to be called on d3 exit)

etpinard · 2016-10-18T22:44:08Z

src/plot_api/plot_api.js

@@ -2124,6 +2126,7 @@ Plotly.update = function update(gd, traceUpdate, layoutUpdate, traces) {
            data: restyleSpecs.eventData,
            layout: relayoutSpecs.eventData
        });
+        gd.emit('plotly_plotmodified');


why not just subscribe to the existing plotly_afterplot ?

Didn't know about it! Just need to add that event in a couple more places, but that should be perfect.

Ah, other reason, now that I think about it:

Should afterplot be emitted between frames? The exact sequence definitely matters, so the other logic was that a unique event would free us from tight coupling.

Should afterplot be emitted between frames?

Hmm. Very good question. I'd say no. plotly_afterplot should be the fired just before the promise is returned. But, I can be convinced otherwise.

Do you think it should be a separate event for this purpose? Again, my fear was coupling the component update code with events that serve other purposes. But I can try to use plotly_afterplot if you prefer. (though, again, it would need to be called during animation)

I have no strong opinion here. I'll leave it up to you.

etpinard · 2016-10-18T22:44:41Z

src/plot_api/plot_api.js

@@ -2302,6 +2305,8 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
            var newFrame = trans._currentFrame = trans._frameQueue.shift();

            if(newFrame) {
+                gd._fullLayout._currentFrame = newFrame.name;


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

rreusser · 2016-10-20T20:29:55Z

src/plots/command.js

+            'plotly_update',
+            'plotly_animatingframe',
+            'plotly_afterplot'
+        ];


@etpinard I refactored this to just hook into a list of existing events. This seems much simpler and more straightforward.

Very nicely done. Thank you

rreusser · 2016-10-20T20:41:09Z

I think this is ready for review. (oops, just a baseline image missing. adding now.) I'm going to go back over the code, but here's a quick summary of need-to-know:

Plots.hasSimpleAPICommandBindings looks over a method and args to see if it's simple enough that we can infer which state is active (currently limited to one property but maybe multiple traces). When run. That means it looks over the whole list of component commands, so it caches a lookup table of value -> state mappings as it goes and bails out as soon as the answer is that it's not simple.
Plots.createCommandObserver: hooks into events that could conceivably change things. Each time it's called, it compares the current value to the cached value. If things have changed, it executes a callback with the index from the lookup table computed in the first step. Needs to be cleaned up with remove() so that event listeners are properly removed.
Plots.executeAPICommand: a common interface for executing API commands. There's hardly anything to it, but seems to make sense to put this in one place.
Plots.computeAPICommandBindings: Exposed for convenience, but currently only used via Plots.hasSimpleAPICommandBindings. This is the magic that takes a method args and computes the bindings for it.

cc: @etpinard @alexcjohnson

rreusser · 2016-10-20T20:46:14Z

Also, here's a live example illustrating what we're actually talking about here: https://rreusser.github.io/animation-experiments/#binding

etpinard

Looking good! Your test cases are looking pretty solid, I think you pull if off 🎉

I wrote down a few questions for you in this review.

My main concern: what happens to the command-observer when a users tries to relayout an updatemenu button or slider step?

etpinard · 2016-10-20T21:42:09Z

src/components/updatemenus/draw.js

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


much cleaner 🎉

etpinard · 2016-10-20T21:42:30Z

src/components/updatemenus/draw.js

@@ -326,6 +319,22 @@ function drawButtons(gd, gHeader, gButton, menuOpts) {
    Lib.setTranslate(gButton, menuOpts.lx, menuOpts.ly);
 }

+function setActive(gd, menuOpts, buttonOpts, gHeader, gButton, buttonIndex, isSilentUpdate) {


nice abstraction

etpinard · 2016-10-20T21:43:14Z

src/plot_api/plot_api.js

@@ -2324,6 +2317,15 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                    newFrame.frameOpts,
                    newFrame.transitionOpts
                );
+
+                gd.emit('plotly_animatingframe', {


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

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.

Thanks for the info 👍

etpinard · 2016-10-20T21:47:45Z

src/plots/command.js

+
+        // If any command has no method, refuse to bind:
+        if(!method) {
+            return false;


Perhaps logging something like two-way binding unavailable using Lib.log (which are hidden by default) would be nice.

etpinard · 2016-10-20T21:50:40Z

src/plots/command.js

+    var container, value, obj;
+    var changed = false;
+
+    if(binding.type === 'data') {


Looks like this is the exact same block as the check function in the createCommandObserver closure. Maybe DRY this up?

Good observation. Had to change the command a little, but dried it up 👍

etpinard · 2016-10-20T21:59:33Z

src/plots/command.js

+
+    // Determine whether there's anything to do for this binding:
+    var binding;
+    if((binding = exports.hasSimpleAPICommandBindings(gd, commandList, lookupTable))) {


Oh I've seen if(a = b) before, but I'm not sure if I totally understand what that does.

@rreusser would you mind helping me out? Thanks!

This kinda became a degenerate case where I might as well move it into the above variable declaration. It's nothing fancy. Just assigns a variable and checks it in one step.

got it. thanks!

etpinard · 2016-10-20T22:01:32Z

test/jasmine/tests/binding_test.js

+    });
+
+    it('updates bindings on plot', function(done) {
+        Plotly.restyle(gd, 'marker.size', 10).catch(fail).then(done);


Would you mind adding an asserting checking if the updatemenus DOM nodes updated correctly. This might be easier to check using when restyling 'marker.color' instead of 'marker.size'.

etpinard · 2016-10-20T22:04:06Z

test/jasmine/tests/command_test.js

+    describe('restyle', function() {
+        describe('with invalid notation', function() {
+            it('with a scalar value', function() {
+                var result = Plots.computeAPICommandBindings(gd, 'restyle', [['x']]);


Thanks for checking this here - as we don't currently don't dive into args checking that its items are valid 🎉

etpinard · 2016-10-20T22:05:59Z

test/jasmine/tests/command_test.js

+                    y: [[3, 4, 5]],
+                    'marker.size': [10, 20, 25],
+                    'line.color': 'red',
+                    line: {


So, line: { width: [2, 8] } does not override line.color: 'red' here. Go figure. Thanks for checking!

Oh no! Good catch. I changed the order so that this is no longer accurate 😞 I need to fix this. Sigh.

I might need to slightly modify my strategy here. On the bright side, that's a totally bizarre case that's actually undefined in the ecmascript standard itself, so I sorta think you get what you deserve if you rely upon it… To be clear, the multi-property cases (not necessarily single-property multi-trace though) will currently always just bail out early with the result that it's not currently handled by bindings.

I'll see if I can resolve this simply, otherwise I'm fine documenting it and moving on. Strictly speaking, the ecmascript standard really doesn't define how this works, though most browsers tend to iterate keys in the order in which they were added.

otherwise I'm fine documenting it and moving on

I'm fine with that also.

Added a note about this to the specific place in the tests where this should be fixed. Long story short, it will definitely cause bindings to get ignored, so I think it's okay since for current use-cases, the result won't be used.

etpinard · 2016-10-20T22:08:16Z

test/jasmine/tests/command_test.js

+    });
+});
+
+describe('component bindings', function() {


How is this suite different than the new binding_test.js here:

https://github.com/plotly/plotly.js/pull/1016/files#diff-7f09c177b80364e96a43622b17a9d8e7R24

?

Oh I see, one test sliders and the other updatemenus.

@rreusser would you mind merging binding_test.js into this describe block?

Moved. Also made the tests effectively analogous between sliders and updatemenus to ensure the API is the same for both. (which for consistency makes active instead of using a label/value for sliders a nice choice)

rreusser · 2016-10-24T20:13:30Z

test/jasmine/tests/command_test.js

+        destroyGraphDiv(gd);
+    });
+
+    it('attaches and updates bindings for sliders', function(done) {


@etpinard See tests. It checks:

event listeners are attached when sliders are added

event listeners are removed when sliders are update and are no longer bindable

changing slider options triggers recomputing the lookup table so it still works

rreusser · 2016-10-24T20:13:46Z

@etpinard I think this is reviewable. I've addressed the concerns and have added a pretty detailed test for both updatemenus and sliders that tests values changing.

Unfortunately (since this isn't immutablejs), detecting arg changes means we need to scan through the steps more often than I'd like (on each relayout) since we can't otherwise be certain whether the values have changed, but it's a pretty cheap check and tries to bail out early and cache when possible. SVG rendering should almost always be way more of a bottleneck than iterating through a few values, so I'm okay with it.

etpinard · 2016-10-25T14:32:04Z

src/components/sliders/draw.js

-            sliderOpts._commandObserver = Plots.createCommandObserver(gd, sliderOpts.steps, function(data) {
-                if(sliderOpts.active === data.index) return;
-                if(sliderOpts._dragging) return;
+        Plots.createCommandObserver(gd, sliderOpts, sliderOpts.steps, function(data) {


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?

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

etpinard · 2016-10-25T14:34:54Z

test/jasmine/tests/command_test.js

+                    // objects is not strictly defined but is at least consistent across browsers.
+                    // The worst-case scenario right now isn't too bad though since it's an obscure
+                    // case that will definitely cause bailout anyway before any bindings would
+                    // happen.


etpinard · 2016-10-25T14:35:17Z

src/components/updatemenus/defaults.js

@@ -33,6 +33,8 @@ module.exports = function updateMenusDefaults(layoutIn, layoutOut) {
        // used to determine object constancy
        menuOut._index = i;

+        menuOut._input.active = menuOut.active;


Hmm. Why do we need this?

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.

Removed this line.

etpinard · 2016-10-25T14:36:59Z

src/components/updatemenus/defaults.js

@@ -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*:


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

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).

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.

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.

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.

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

etpinard · 2016-10-25T16:20:18Z

test/jasmine/tests/command_test.js

+    it('udpates bound components when the value changes', function(done) {
+        expect(gd.layout.sliders[0].active).toBe(0);
+
+        Plotly.restyle(gd, 'marker.color', 'blue').then(function() {


I'm a big fan of this test and the next. 🍻

Ah, confused it with the deluxe tests down below. Yeah, these are just nice and simple 😄

etpinard · 2016-10-25T16:21:56Z

test/jasmine/tests/updatemenus_test.js

+    });
+
+    it('does not set active on initial plot', function() {
+        expect(gd.layout.updatemenus[0].active).toBeUndefined();


very nice. Thanks for 🔒 this !

etpinard · 2016-10-25T16:22:59Z

Hard fought 💃

Awesome work @rreusser

rreusser added 2 commits October 4, 2016 18:40

Start implementing command execution wrapper

f2d268d

Write failing tests for binding computation

65f618d

rreusser added feature status: in progress labels Oct 6, 2016

rreusser changed the title ~~[WIP] Common interface to interpret and execute API methods~~ Common interface to interpret and execute API methods [WIP] Oct 6, 2016

rreusser added 2 commits October 10, 2016 18:16

Add a lot of tests for binding computation

7ef0404

Implement animate and update binding computation

edb6773

rreusser mentioned this pull request Oct 11, 2016

Bindings #1002

Closed

rreusser added 2 commits October 11, 2016 11:39

Switch return format of bindings to structured output

f50fcaa

Merge remote-tracking branch 'origin/master' into lib-commands

60dd634

etpinard added this to the v1.19.0 milestone Oct 13, 2016

rreusser added 3 commits October 17, 2016 12:58

Command to decide when bindings are simple

4bc8387

First working version of bindings

93b8253

Change the method name

43dc3d4

rreusser added 2 commits October 18, 2016 12:51

Fix updatemenus bindings

65d24f3

Hook up animations to sliders via bindings

d2696e7

etpinard reviewed Oct 18, 2016

View reviewed changes

rreusser added 3 commits October 20, 2016 15:29

Clean up slider positioning code

7db7f03

Add mock for bindings

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

57fcf96

emove custom plotmodified event

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

7db5f1f

rreusser commented Oct 20, 2016

View reviewed changes

rreusser changed the title ~~Common interface to interpret and execute API methods [WIP]~~ Common interface to interpret and execute API methods Oct 20, 2016

rreusser added status: reviewable and removed status: in progress labels Oct 20, 2016

Add binding baseline image

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

26ad1ff

etpinard suggested changes Oct 20, 2016

View reviewed changes

rreusser added 11 commits October 24, 2016 10:33

Fix irritating self-interaction when dragging slider

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

986b4dc

Fix lint issue

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

20ac69f

Change failure modes for command API

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

f993ed7

Ugh linter again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

99d7c8e

Improve robustness of command bindings

f8c0094

Test more behavior of bindings

6abec15

Fix linter issue

5df94a7

Remove binding test file

d35ee35

Add equivalent command API test for udpatemenus

a554cad

DRY up binding change check

e5a80ee

Add note about test failure

45717b1

rreusser commented Oct 24, 2016

View reviewed changes

rreusser mentioned this pull request Oct 25, 2016

Layout animations for container array'd objects #1081

Merged

etpinard reviewed Oct 25, 2016

View reviewed changes

createCommandObserver --> manageCommandObserver

52de9e4

etpinard reviewed Oct 25, 2016

View reviewed changes

rreusser added 2 commits October 25, 2016 10:40

Remove hard-coded updatemenus active default

0c40b02

Revert updatemenus initialization and fix sliders initialization

df2d5bb

etpinard reviewed Oct 25, 2016

View reviewed changes

etpinard approved these changes Oct 25, 2016

View reviewed changes

rreusser merged commit 297f0a7 into master Oct 25, 2016

rreusser deleted the lib-commands branch October 25, 2016 16:54

Common interface to interpret and execute API methods #1016

Common interface to interpret and execute API methods #1016

Conversation

rreusser commented Oct 6, 2016 • edited Loading

rreusser commented Oct 18, 2016 • edited Loading

rreusser commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Oct 20, 2016 • edited Loading

rreusser commented Oct 20, 2016 • edited Loading

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser commented Oct 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rreusser Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 25, 2016

rreusser commented Oct 6, 2016 •

edited

Loading

rreusser commented Oct 18, 2016 •

edited

Loading

rreusser commented Oct 20, 2016 •

edited

Loading

rreusser commented Oct 20, 2016 •

edited

Loading

rreusser Oct 21, 2016 •

edited

Loading

rreusser Oct 21, 2016 •

edited

Loading

rreusser Oct 24, 2016 •

edited

Loading

rreusser Oct 24, 2016 •

edited

Loading

etpinard Oct 25, 2016 •

edited

Loading

rreusser Oct 25, 2016 •

edited

Loading