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

add button repeat #400

Closed
wants to merge 1 commit into from
Closed

Conversation

Easyghost195
Copy link
Contributor

Following the request about "Suggestion to add a repeat button to the interface? #360"

I suggest you a clickable button for looping the current file:
repeat_button

Regards.

@gnome-mpv
Copy link
Collaborator

gnome-mpv commented Dec 5, 2018

I think we should make the button indicate whether or not looping is currently enabled. It's going to be difficult to tell what the button is going to do otherwise.

@@ -419,6 +422,7 @@ static void gmpv_control_box_init(GmpvControlBox *box)
box->previous_button = gtk_button_new();
box->fullscreen_button = gtk_button_new();
box->volume_button = gtk_volume_button_new();
box->repeat_button = gtk_button_new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
box->repeat_button = gtk_button_new();
box->repeat_button = gtk_toggle_button_new();

Use toggle button here so that the user can see whether or not looping is currently enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't looping be tri-state, loop-off, loop-current, and loop-playlist?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that it's enough to have loop-off and loop-playlist for this button. This seems to be what most media players do (and presumably the most common use case).

@@ -446,6 +450,9 @@ static void gmpv_control_box_init(GmpvControlBox *box)
init_button( box->previous_button,
"media-skip-backward-symbolic",
_("Previous Chapter") );
init_button( box->repeat_button,
"media-playlist-repeat",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"media-playlist-repeat",
"media-playlist-repeat-symbolic",

Use tabs to indent. Also, explicitly use symbolic variant of the icon.

@@ -511,6 +518,10 @@ static void gmpv_control_box_init(GmpvControlBox *box)
"clicked",
G_CALLBACK(simple_signal_handler),
box );
g_signal_connect( box->repeat_button,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of using click signal here, it would be better to create a new property and bind it with the loop-playlist property in GmpvModel. We can then bind the repeat button's active property to this property.

@gnome-mpv
Copy link
Collaborator

Edited and merged as 024c0fd.

@gnome-mpv gnome-mpv closed this Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants