Skip to content

Add disablemethod flag to button and slider steps #1695

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 May 17, 2017 · 19 comments
Closed

Add disablemethod flag to button and slider steps #1695

rreusser opened this issue May 17, 2017 · 19 comments
Labels
feature something new

Comments

@rreusser
Copy link
Contributor

rreusser commented May 17, 2017

An emerging pattern for examples of sufficient complexity is that it's becoming necessary to hook into components via events instead of strictly the command api interface. This might be the case if you have to, say, perform a couple precisely sequenced async actions before animating.

However, a problem with sliders, for example, is that bailing on specifying method and args as you normally would breaks the coupling that puts the slider in the correct position when you change the frame via some other pathway, like a play button.

I propose two things:

  • disablemethod for updatemenus and sliders buttons/steps that leaves everything else unaffected and just skips actually executing the method so that you can hook into an event and call it yourself in a more sophisticated way. Otherwise the behavior will be unaffected.
  • allow null for a method. Currently the supplydefaults enum blocks what seems like a legitimate usage when you just want a slider with no associated api call. (i.e. why have events if you can't use them?)

/cc @etpinard @alexcjohnson @bpostlethwaite for what might need to be expedited feedback and fixes

@rreusser
Copy link
Contributor Author

rreusser commented May 17, 2017

Other considerations: Maybe this should be the affirmatively-named enabled with dflt: true instead. There could also be a separate flag for enabling/disabling the automagic two-way coupling, but that seems orthogonal and not currently necessary to address.

@etpinard
Copy link
Contributor

etpinard commented May 17, 2017

allow null for a method. Currently the supplydefaults enum blocks what seems like a legitimate usage when you just want a slider with no associated api call. (i.e. why have events if you can't use them?)

Maybe not null (as null means restyle(gd, astr, null) means set to attribute back to its default), but I like the idea of adding another value for method. Maybe method: 'skip'?

@rreusser
Copy link
Contributor Author

Sorry, just null for the method, not the args. (as in Plotly[null], which is just a no-op as Plotly['skip'] would also be). But I do like the idea of skip.

@etpinard
Copy link
Contributor

as in Plotly[null], which is just a no-op

Right. But there would be no way to restyle it to that value. In other words, there are no null values in any of our attribute declarations for a reason. Actually, that's a lie, there are few null like in contour.start but they mean this default value is auto-computed not this default value is null.

@rreusser
Copy link
Contributor Author

Ohhhh, I see your point. Got it. Yeah, then I like skip.

@cpsievert
Copy link

Thanks for opening @rreusser, I definitely have some good use-cases for this -- where can I find the naming of these events? Is there official documentation anywhere?

@rreusser
Copy link
Contributor Author

rreusser commented May 17, 2017

Conclusion:

  • skip method to just make it a no-op component that does nothing unless you hook into events
  • enable flag (dflt: true) that allows you to still specify, say animate so everything links up correctly except it just doesn't actually invoke the command.

I don't love name enable. That suggests disabling it would gray it out or something, which isn't really the case.

@alexcjohnson
Copy link
Collaborator

👍 for skip
I don't like enable either - it implies that it will do nothing if it's false - including not firing any events or anything.

Ideally it seems like we'd want a way to defer the action, but let you invoke it later - like if the native plotly.js invocation of the animation or whatever were passed into the event that you're going to bind to as a callback. But we can implement that later. For now, lets give it a more precise name at least. I want to say something like executedseparately: true (dflt: false) because if the user doesn't execute the same change as part of the event handler, the plot could end up in an inconsistent state. That's an awfully long name, anybody have a better idea?

@rreusser
Copy link
Contributor Author

Another option is a string instead of a Boolean to make room for also maybe disabling the two way binding, though not sure why that'd be desirable.

It just execute (true/false)

@alexcjohnson
Copy link
Collaborator

Sure, I could go for execute if it's well enough described (with the caveat that if you say you're going to execute and you don't, all bets are off.

@rreusser
Copy link
Contributor Author

Can you clarify the last point? I'm not sure I follow.

@alexcjohnson
Copy link
Collaborator

if the user sets execute: false but then doesn't do the thing specified in the action, then the rest of the UI will think it has happened and update accordingly... but it will be wrong.

Anyway not a big deal, that's just a documentation issue; the functionality sounds good.

@rreusser
Copy link
Contributor Author

Ahhh, I see. Yes, I think the burden is on the user at this advanced level of usage.

@rreusser
Copy link
Contributor Author

rreusser commented May 18, 2017

And a small nitpick that it doesn't feed the new state into controls that way. Controls read from _fullData to see if the piece they connect to has changed. So hopefully nothing gets too weird here.

@alexcjohnson
Copy link
Collaborator

And a small nitpick that it doesn't feed the new state into controls that way. Controls read from _fullData to see if the piece they connect to has changed. So hopefully nothing gets too weird here.

Ah, I see. That's good, and seems like it assuages my fears for correlated components. Let me just follow through the implications for an execute: false slider itself:

  • When you move it somewhere, it stays there, waiting for the change to be realized, even though at that instant plot is in an inconsistent state. This must be how we handle that moment, since the event-handler response could happen an arbitrary time later and we don't want to have it do something like jump back immediately to maintain consistency and then jump to the drop value once the change occurs.
  • If the event handler executes the change it said it would, we're all happy, the slider doesn't move but the plot is once again in a consistent state.
  • But if the event handler executes a change that puts the slider into a different state than where it was dragged, I guess the slider should recognize that and move to the true location? While you still have the mouse down on it? Confusing for the viewer but that would get the plot back to consistency. Is that what happens?

@rreusser
Copy link
Contributor Author

Correct. 90% of the difficulty in implementing the slider was getting it to avoid updating itself while updating itself… It accomplishes this with a somewhat carefully 😬 choreographed sequence of async updates and flags related to whether the source of the update is user input or an internal state change. I've been very happy with how well it's worked.

You make a good point about what happens if you do something bizarre like making it self-inconsistent by overriding the behavior with something that's not what it says it's going to do. To be honest, I don't know exactly what will happen in that case, but I think it's worth a quick experiment to check—though I do think it's not a particularly important corner case since you can always do weird things like gd.on('plotly_restyle', () => Plotly.restyle(...)) that will be obviously problematic. But I'll check to make sure the result isn't something disastrous like an infinite loop.

@etpinard
Copy link
Contributor

@rreusser Is this thing now done via #1700 and #1699 ?

@rreusser
Copy link
Contributor Author

Thanks, yes. Closing.

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

4 participants