Skip to content
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

Play widget: expose playing and repeat #2283

Merged
merged 3 commits into from
Jan 8, 2020

Conversation

jtpio
Copy link
Member

@jtpio jtpio commented Dec 5, 2018

Fixes #1897.

Compared to the other widgets, there is no straightforward way to control the Play widget programmatically, except for _playing attribute as discussed in #1897.

Since the animation logic is happening on the frontend, this change is mostly about triggering the click method for the corresponding buttons.

play_stop_pause_methods

@SylvainCorlay
Copy link
Member

SylvainCorlay commented Apr 25, 2019

I am +1 on this feature.

I am not sure actually calling the click method on the button is the way to go.

However, this looks simple enough, so am fine with merging as-is.

@jasongrout do you want to press the green button? (and closing #1897)

@jasongrout
Copy link
Member

How about using state changes instead of messages? If we have a play state attribute that indicates whether the widget is playing or not, pause becomes setting that attribute to False, pressing play is setting that attribute to True, and clicking stop is equivalent to setting the play state to False and the value attribute to the min value.

In general, I think state changes are simpler to reason about than sending messages, and the bonus here is that we get information into what is happening on the python side.

@jasongrout
Copy link
Member

We already have _playing, as noted in #1897. Perhaps just exposing playing and repeat as official state, and making them actually work bidirectionally would be great.

@vidartf
Copy link
Member

vidartf commented Apr 25, 2019

The methods could also just set the state of _playing, could they not?

@jtpio
Copy link
Member Author

jtpio commented Apr 25, 2019

As far as I can recall setting the state of _playing was not sufficient without significant changes to the frontend animation logic.

But yes it would be good to have both playing and repeat so it's similar to what the other widgets expose.

@SylvainCorlay SylvainCorlay added this to the Minor release milestone Jun 4, 2019
@jtpio jtpio changed the title Add play, stop and pause methods to the Play widget [WIP] Play widget: expose playing and repeat Jan 6, 2020
@jtpio jtpio force-pushed the play-widget branch 3 times, most recently from d2d2e03 to 77c6113 Compare January 6, 2020 14:44
@jtpio
Copy link
Member Author

jtpio commented Jan 6, 2020

Reworking this PR to control the Play widget by exposing and setting playing and repeat:

play-playing-repeat

To trigger a stop programmatically:

play.playing = False
play.value = play.min

@jtpio jtpio changed the title [WIP] Play widget: expose playing and repeat Play widget: expose playing and repeat Jan 6, 2020
@jtpio
Copy link
Member Author

jtpio commented Jan 7, 2020

This should also fix the issue where the play button could be hit multiple times in a row, which would "stack up" the animation.

play-multiple-plays

@jtpio
Copy link
Member Author

jtpio commented Jan 7, 2020

Another thing to look at in a separate PR would be to drop the pause button, and instead have a toggle play / pause button.

Edit: opened #2671

@SylvainCorlay
Copy link
Member

This looks good to me! Thanks @jtpio.

This needs a rebase before it is merged.

@jtpio
Copy link
Member Author

jtpio commented Jan 8, 2020

Done.

@SylvainCorlay SylvainCorlay merged commit 0cd00a5 into jupyter-widgets:master Jan 8, 2020
@jtpio jtpio deleted the play-widget branch January 8, 2020 18:36
@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:controls resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Play widget will stop programmatically, but won't start
4 participants