Skip to content

Bindings #1002

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

Closed
rreusser opened this issue Oct 4, 2016 · 13 comments
Closed

Bindings #1002

rreusser opened this issue Oct 4, 2016 · 13 comments
Labels
feature something new
Milestone

Comments

@rreusser
Copy link
Contributor

rreusser commented Oct 4, 2016

As controls get more capable in plotly, two-way binding becomes necessary. That is, a slider can change the frame, but the slider should also reflect the current frame when something else (e.g. a play button) changes it. This could also be the case for a marker size, a size ref, or a transform parameter. My current slider PR does this in sort of an ad hoc manner, but I really think it should be more general.

Proposal: bindings

Option 1:

Bindings are defined in the layout. For example:

"layout": {
  "bindings": [{
    "bind": "layout.updatemenus[0].active",
    "to": "data[0].marker.color",
    "lookup": {
      "red": 0,
      "green": 1,
      "blue": 2
    }
  }]
}

In this example, bind is the data that gets changed, to is the item that's observed for changes, and lookup is an optional translation table that maps one to the other. Whenever a plotly event is emitted or when a relayout/restyle/update occurs, we quickly look through the bindings to see what's changed (which shouldn't be expensive enough that it's a problem). If an item has changed, that item is written to the state of the plot. A plotly_bindingchange event is emitted. All components (that's the best I can do right now) must then look through their data to see if anything has changed and update accordingly.

Pros:

  • Bindings link two parts of the plotly state, which accurately represents what's actually happening

Cons:

  • Updates take just a bit of thought and bookkeeping to do efficiently since this isn't react
  • requires a separate specification of bindings in addition to just the slider
  • refers to other parts of the plotly json by integer index within the plotly json, which seems probably okay but 10% fragile

Option 2:

Bindings are defined in the component that needs them, e.g. a slider. The slider has:

"bindto": "data[0].marker.color"

Most of the logic is the same. (I've omitted the lookup table since, though it could be added, we should probably go to great lengths to generally avoid needing.) When a component binds to a variable, we register it as needing a lookup. We scan through those liberally since it's fast, and when one changes, we emit an event. This case is a little simpler since we can emit plotly_bindingchange with data {binding: 'data[0].marker.color', value: 'red'}. The component sees its binding has changed and updates accordingly.

Pros:

  • Requires less typing
  • Collocates binding with the thing that uses the binding
  • Avoids extra reference to updatemenus[0].active
  • The update pattern is maybe a bit more straightforward since the component directly knows about the binding.

Cons:

  • Less general since it specifies a way of hooking up a component to a json item instead of hooking up a json item to a json item

cc: @etpinard @chriddyp @alexcjohnson

@etpinard
Copy link
Contributor

etpinard commented Oct 4, 2016

cc @chriddyp

@etpinard
Copy link
Contributor

etpinard commented Oct 4, 2016

cc @alexcjohnson

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

After brief discussion with @etpinard, going to opt for Option 2 since I think Option 1 has much more complicated side effects, is more challenging to specify, and is more functionality than is actually required for this need.

@alexcjohnson
Copy link
Collaborator

Don't we already have that information in the definition of what each control does? ie if a control sets marker.color to red then we can also tell we need to update it if marker.color changes. If we want to calculate some binding table behind the scenes to make our lives easier that's fine, but it seems redundant and confusing to have to specify this again.

Also it's possible for a control to be bound to more than one attribute, right? Like a button could change both marker.color and marker.size - in which case you'd have to check the state when either one of those changes, and only mark the control as active if the state matches all parts.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

Good point @alexcjohnson. That might be a good approach. It's possible to inspect the method and args and try to determine what's being set, though it's not an entirely trivial thing to do. For example, then we get into marker.size vs. marker: {size: ...}. The thought was to make it explicit, but I can see the argument for making it implicit and locking down the logic.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

As part of this, I'd like to factor out the method and args logic into a general set of helpers, so a routine that builds this table of bindings (even if it's a bit complicated) might not be a bad idea.

@alexcjohnson
Copy link
Collaborator

As part of this, I'd like to factor out the method and args logic into a general set of helpers, so a routine that builds this table of bindings (even if it's a bit complicated) might not be a bad idea.

Sounds great! Complicated is fine ( ❤️ 🐅 ! ) if it makes the resulting experience more robust and simpler, which this seems like it will do.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

@alexcjohnson Thanks for the input! I still haven't ruled out the possibility of weird corner cases in my head, but this feels reasonable and in particular I like it because it avoids any additional API surface area.

I'll go for it and see what happens.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

Here are the difficulties I can come up with:

Plotly.restyle, for example, implements a fair amount of logic around variadic arguments that either modify one trace or multiple traces or set properties from a string and a value or from an object. This function would need to reproduce and interpret all of that logic successfully in order to work as expected.

It's not insurmountable since it's self-contained and well-defined; it just needs a lot of testing and enumeration of cases in order to work reliably.

@alexcjohnson
Copy link
Collaborator

Yeah, Plotly.restyle and Plotly.relayout are monsters in serious need of refactoring. Maybe to start down that road, can we extract the argument-normalizing functionality into a separate module, that you could reuse outside of those functions to ensure consistency?

@rreusser
Copy link
Contributor Author

rreusser commented Oct 4, 2016

I think there's been some step down that path with Lib.coerceTraceIndices(), but yeah, definitely more needed.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 11, 2016

@alexcjohnson @etpinard:

As part of #1016, I've implemented the logic to work out which properties are affected by a method and args. For example, if there are two traces, then Plots.computeAPICommandBindings returns for {method: 'restyle', args: ['marker.width', 5]}:

bindings = [
  'data[0].marker.width',
  'data[1].marker.width'
];

So in that sense, things are good. The tricky part of automating these bindings is then updating them. Figuring out the properties have changed is easy, but matching that with a component state is not straightforward except for the simple case of a single property.

Do you think it's the right move to automate these bindings but only for the relatively simple case where it's very straightforward to match plot state to component state?

I can add logic to accomplish this; my concern is that magic that works only in certain conditions is maybe worse than specifying it manually since it's not clear to the end-user precisely what's going on.

@etpinard etpinard added this to the v1.19.0 milestone Oct 13, 2016
@etpinard
Copy link
Contributor

first iteration completed in #1016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

No branches or pull requests

3 participants