Couldn't agree more.

Using 0.9.3 for the first time now, and couldn't find the pause button.
People expect a pause button!

I sooo agree!

Created attachment 71650 [details]
Play/Pause button in Muine

I'd suggest going the Muine way: a bigger Play/Pause button. It is the most important button after all, isn't it? My cd player has a play button that is twice the size of the skip buttons...

The screenshot shows the Muine Play/Pause button in state "playing".

I think the Muine button gives the user a mixed message. The button states that it is for both playing and pausing. If the button is in the pressed state does that mean the pause is the state that is active or that the play is the state that is active. It isn't immediately obvious until the user discovers it for herself by pressing it a few times to investigate.

If RB used the idea in comment 1 then it would be immediate obvious to the user. She hits the play button to play, and then she can hit the pause button to pause. This doesn't have the effect of suggesting two mutually exclusive states are both active.

I think the suggestion in comment 1 makes a lot of sense.  Indeed, it's what I expected when I first fired up rhythmbox.  When the mouse is hovering over the button, the button is highlighted whether it is in the depressed state or not, such that it is very difficult to tell whether the mouse click did anything until the music starts playing (which could be a fair while with some of my Pink Floyd tracks...).

The first time I used rhythmbox I thought it was broken, because pressing the play button appeared to do nothing.

The right-click menu from the notification icon is similarly confusing - my wife couldn't work out how to pause the music from it, because she didn't realise that the play option's tick box had a stateful meaning, and wasn't just a pretty icon with no function (like those for the "next" and "previous" menu options).  I suggest removing the checkbox, and separating the "play" option into "play" and "pause" - one of which will always be greyed out, of course.

Interface components that change are bad UI imho, just like removing menu items (instead of disabling them) is bad UI. The button used in Muine immediately that the application is playing since the button is in the "pressed" state...

(In reply to comment #6)
> Interface components that change are bad UI imho, just like removing menu items
> (instead of disabling them) is bad UI. The button used in Muine immediately
> that the application is playing since the button is in the "pressed" state...

It is not like removing menu items. It is reflecting properly the toggle state of a UI element. The idea is to just make the user feedback more obvious. And at that point it is not even about changing the action behing the UI element, the toolbar button.

Created attachment 76738 [details] [review]
Proposed implementation

Attached patch contains a proposed implementation of this change.  This causes the icon to toggle between "Play" and "Pause", instead of between highlighted and non-highlighted.

The main problem I see here is that the current approach is not very HIG compilant¹:

- "Only use toggle buttons in groups, so they are not mistaken for regular buttons" => currently toggle buttons (play/pause, shuffle/repeat, show browser are mixed with normal toolbar buttons (next/pref + extra toolbar buttons)

- "Label a group of toggle buttons with a descriptive heading above or to the left of the group" => no such label is used

I have to agree with Hubert, this *is* a bad regression compared to pre-0.9 releases and should be fixed ASAP to conform with the HIG and other GNOME applications (see Totem for example).

I also agree that the action in the notification area applet should not have the check but instead the corresponding icon ("|>" or "||").

In addition, the shuffle/repeat buttons allow only 2 states (active/inactive) in this design but it would be nice to have at last more repeat states, e.g. not only "no repeat" and "repeat all" but also "repeat only playing song" or "repeat playing album". I will file a separate bug about this.

[1] http://developer.gnome.org/projects/gup/hig/2.0/controls-toggle-buttons.html

Created attachment 77411 [details]
Mockup

As said before, the play/pause button is not the only issue here. Same thing is true for:

- Repeat
- Shuffle
- Show browser

I propose moving those away from the main play toolbar buttons, e.g. reviving the show browser expander from pre-0.9 (which is perfectly HIG) and moving the repeat shuffle somewhere else, allowing more than two states at least for repeat (in the mockup they are moved to the right but perhaps something like what Banshee¹ does would be even better? Look at the bottom left)

I also moved the All/Artists/Album/Title toggle group to the search bar (note the search icon in the left of the entry), just as Evolution uses it. The current approach wastes much space, clutters the interface and is not clear usability wise (e.g. if you do a search with title selected and an album selected in the browser, what happens?)

[1] http://banshee-project.org/images/thumb/9/92/800px-Banshee-0.11.0-release-library.png

Created attachment 77413 [details]
Another mockup + Proposal

Another thing: don't let sources add their own items to the main toolbar. Because THIS is interface morphing that should be avoided at all cost (for play/pause the "morph" makes sense but here it doesn't as the actions are not as common - for example you don't subscribe to a new podcast every 5 minutes and if you do you can go to the menu without a problem, let alone the fact that podcasts would better be subscribed from the web directly)

This second mockup is what I would prefere the rhythmbox interface would look like. In words:

- play/pause button changed back to pre-0.9 look/bahaviour
- non-playback buttons removed from the toolbar.
- playback state (toggle & repeat) are able to hold more than 2 states, moved to the bottom left
- hide browser changed back to pre-0.9 look/bahaviour
- search toggle group integrated with searchbar to make the interface cleaner (doesn't lose any functionality)
- Playing song info (artist/title) and play slider / time moved up. This frees a lot of space and re-uses unused space in the bar above. As heard on IRC people wanted a longer play slider but this should be enough for anyone. For those who need to skip to a specific place in the song it would be nicer to have a "skip to [ n:m ]"-dialog anyway.
- some reordering in the sidebar

I know this has multiple changes out of the scope of this bug but I feel rhythmbox needs quite some interface love and I want to show where I think RB should be heading towards and why the current toggle buttons in the toolbar are bad. Please consider.

I agree with some of the things that Michael Monreal mentioned in his reply.  His mockup, with the slider and current song info moved onto the toolbar, are awesome.  I'm using a widescreen laptop, so vertical pixels are at a premium.  I'll attach a mockup of what I had in mind (which is very similar to Michael's mockup).

John

Created attachment 77670 [details]
Moved the slider and song info onto the toolbar to save some vertical pixels.

I've just abandoned XMMS/Audacious for Rhythmbox, and keep getting confused for a brief instant where to click to pause playback.

If the app is playing, the play/pause button should display the icon for what it will do if clicked: *pause.* This is just basic, fundamental usability. 

All this other UI debate stuff is great -- but at bare minimum fix this horrible brokenness. If the app is playing, show the pause icon. If the app is not playing, show the play icon.

IMHO, not changing the icon is broken behaviour, regardless of a guideline in the HIG.

The icon  doesn't indicate what will happen when you click it while playing. When not playing, you can tell that clicking will start playing. But once it's playing, will clicking again pause the music, or stop it? You can't tell by looking, and either behaviour would be a reasonable expectation. 

I agree 100% with Alex!  The current implementation gives us zero indication of behavior when we click the "Play/Pause" button.
Presumably some audio can not be paused (Podcasts, Internet Radio, etc.) in which case the "Play" button should indicate through tooltip AND icon that clicking the button will "Stop" the current track.
When audio can be paused, then the icon should indicate, again through tooltip AND icon that clicking the button will "Pause" the current track.

Let's gather some consensus on this... I'm tired of hearing about a 27 month old bug.

John Daiker

*** Bug 537914 has been marked as a duplicate of this bug. ***

I don't think the HIG applies to this case.  In this case, there is an overwhelming precedent.  Video and audio players all over the web have play buttons that toggle to a pause icon when clicked -- this is a very common user interface idiom at present.  Since this is what users are used to, we should not expect them to learn a different paradigm because of a rigid interpretation of the HIG.  The response and consensus on this bug report I think bears this out.  This seems like an easy enough bug to fix and, though I'm a newbee when it comes to making contributions to open source code, if no one has fixed it by June I volunteer to give it a try.

I volunteer to fix this. Shall I go ahead and submit a patch?

Jonathan,
Can you please comment on whether we should fix this bug or not? I shall try to submit a patch by end of today so was just confirming.

I have a patch for this fix. Basically what this patch does is this:

If anything is being played:

  If source supports PAUSE (MP3 audio tracks from the library for example), tooltip changes to 'Pause Playback' AND icon changes to stock pause icon.
  
  else
  If source does not support  PAUSE (internet radio, for example) tooltip changes to 'Stop Playback' AND icon changes to stock stop icon.

else if nothing is being played:
     Tooltip changes to 'Start playback' and icon changes to stock play icon.

Attaching the patch in bugzilla for review. This is the first time I am submitting a patch so there might be mistakes ;-) Do let me know so that I can correct them.

Created attachment 135752 [details] [review]
Proposed patch for this bug. Needs review.

Please review this patch and let me know your comments.

Did you notice that there was already a patch here?  You should combine the ideas from both patches.

N_("..") is not appropriate here, you should use _("...") instead.

We need to make sure this doesn't cause any accessibility problems.

Yeah I saw the previous patch. But that was done in 2006 and the base code itself has changed so much.

But I will again go through the patch to make sure I have not missed anything.
Thanks for your help, since this is my first patch (even though trivial) I may miss out many things.

Also, sorry for missing out on the _(".."). Can you please tell me the difference between the two? Sorry if this sounds like a stupid question.

Thanks again for your help.

Couple of things I noticed about the older patch submitted on 2006-11-17. Not sure if that is applicable now:

1. The play button is changed from a toggle button to a normal button (like previous and next buttons) in the previous proposed patch.

Question: Should we do it that way or should we make it as a toggled play/pause button. I personally prefer the button to be a toggle button and appear 'depressed' so that I can visually and quickly know that the button is in active state.


2. An idle callback is added using the function call 
        g_idle_add (_idle_unblock_signal_cb, player); at the end of the function 
rb_shell_player_playing_changed_cb.

Question: I assume this was missed from my patch. Would add it and move the code to update the buttons to a new function which I can then call from my idle callback. But for my knowledge would like to know why this was done at the first place. Why do we register for a idle callback using g_idle_add() and update the player UI controls? What is the rationale?

Would be grateful if anyone can answer my above queries because only then I can submit a revised patch for this bug.

I did not hear anything from anyone on this.

Today I am attaching another patch where I have moved everything to a function which updates the buttons and I call this function from the idle callback also.

However, in this patch, the play button will behave as a toggle button and while anything is being played the button will appear 'depressed'.

Is that fine? If not, I can submit another patch where I will change the play button from a toggle button to a normal one. I the changed the N_("..")s to _("...")s.

Please review this new patch and comment.

Also, am I supposed to edit the changelog file while submitting patches? I am asking since this is the first patch I am submitting to rhythmbox.

Created attachment 135980 [details] [review]
Proposed patch for this bug, after incorporating initial comments. Needs review.

Kindly review this patch.

Please don't nag me via email.  I will review your patch when I get around to it.

Yeah, it's this kind of relentless attention to usability and humble engagement with the community that really demonstrates the promise of open source. Way to go, guys. :)

I can see how it would be frustrating that my priorities are different to yours, but sarcastic commentary isn't going to change that.  There are any number of things you can do that would be more helpful, including not making sarcastic commentary.

Jonathan,

I am sorry you got it all wrong. I just asked in my email to review the patch "When you get time", because I know how busy you are and also appreciate the work you have done for maintaining rhythmbox.

So to clarify, I was not nagging you.

(In reply to comment #26)
> However, in this patch, the play button will behave as a toggle button and
> while anything is being played the button will appear 'depressed'.

This is worse than the current situation.  If you want to change the image and tooltip, you also need to change the action type too.  

> Also, am I supposed to edit the changelog file while submitting patches? I am
> asking since this is the first patch I am submitting to rhythmbox.

No, we do not update the changelog file at all. 


+        /* update the play/pause button */
+        update_play_pause_button(action, player);	

You shouldn't add comments that don't actually explain anything.  It's obvious what the function does.

+/**
+ * update_play_pause_button:
+ * @action: Action entry that this function will modify
+ * @player: The #RBShellPlayer
+ * 
+ * Updates the play pause button/tooltip based on the current state of the
+ * player as well as type of source. E.g while playing internet radio, play 
+ * button becomes STOP but while playing MP3 tracks from the disk, the play
+ * button becomes PAUSE.
+ */
+static void update_play_pause_button(GtkAction *action, RBShellPlayer *player)

This is a static function so it doesn't need API documentation.  The RBShellPlayer should always be the first argument to its own functions.

Please try to follow the formatting of the surrounding code, particularly with respect to tabs/spaces for indenting (we use tabs) and spaces between function names and arguments.

Created attachment 136164 [details] [review]
Proposed patch for this bug, after incorporating more comments. Needs review.

Modified my patch as per Jonathan's comments. 

a) Removed all spaces and replaced them with tabs.
b) Did the indentation properly and formatted all new code to conform to the coding style.
c) Changed the play/pause key into a normal key from a toggle key.
d) Changed update_play_pause_button () to make sure it conforms to existing static functions within rb-shell-player.c file.

Have you looked into how this affects accessibility?  

Some more minor comments:

+static void update_play_pause_button (RBShellPlayer *player, GtkAction *action)

we format function bodies like this:

[static] <return type>
<function name> (<args>)
{
 ...
}

+		if(rb_source_can_pause (player->priv->source)) {

should be a space between 'if' and the condition here

+			tooltip = g_strdup (_("Pause playback"));

+			tooltip = g_strdup (_("Stop playback"));

+		tooltip = g_strdup (_("Start playback"));

There's no need to duplicate the tooltip string here.

Seemanta, any chance you could continue work on your patch? We would very much like to include it in Ubuntu 10.04 as part of the One Hundred Paper Cuts project.

Launchpad bug:
https://bugs.edge.launchpad.net/ubuntu/+source/rhythmbox/+bug/71228

By all means, David! I had some personal stuff going on so was not actively participating in rhythmbox discussions, though was reading all bugzilla mails.

Surely, I can help fix this bug. What is your need by date ? I shall plan accordingly.

Created attachment 152427 [details] [review]
Make Play button switch to Pause button on toggle

This patch causes the Play button to toggle to a Pause button (both label and icon) on click.

Review of attachment 152427 [details] [review]:

This is worse than the current situation.  You end up with a pause button that looks like it's been pressed, but it's playing, not paused.

So, the ideal solution would be *not* to change the state of the button to *toggled* but just to change the icon and tooltip to reflect the true objective of the button,i.e. PAUSE/PLAY/STOP. Th button itself stays *normal*,without looking depressed,like it has been clicked.

If you confirm, I can submit a new patch for this.

However it's done, the button states need to make sense.  If you want to change the label on the button to show that pressing it will pause playback, then it doesn't make sense for the button to be depressed.

*** Bug 608819 has been marked as a duplicate of this bug. ***

Here is another proposed patch for this bug. I have modified the behavior of the play button in the following ways:

1. It is no longer a toggle button now. Based on the discussions above, I have made it into a normal button.

2. When a song is playing, the icon changes to a pause icon. For internet radio, it would be a stop icon. (Indicating to the user that the music *may* be paused/stopped).

3. When nothing is being played, the icon remains a play icon (indicating to the user, the music *may* be played).

Please review the attached patch and provide your comments. I hope the play button behavior now makes sense.

regards,
Seemanta

Created attachment 152952 [details] [review]
Proposed patch for this bug (Feb 4th)

Please review the proposed patch for this bug. I have already added my comments in the main comments section and also mentioned how this patch changes the behavior of the current play button.

I think the above patch looks good. Rhythmbox devs, what do you think?

(In reply to comment #43)
> Created an attachment (id=152952) [details] [review]
> Proposed patch for this bug (Feb 4th)
> 
> Please review the proposed patch for this bug. I have already added my comments
> in the main comments section and also mentioned how this patch changes the
> behavior of the current play button.

Could the RB devs review the patch in comment 43 , we would like to get this fixed for Ubuntu Maverick.

I'm not interested in making this change in isolation.  If you want to ship this patch, go right ahead.

(In reply to comment #46)
> I'm not interested in making this change in isolation.  If you want to ship
> this patch, go right ahead.

Jonathan , No one said anything about shipping a patch which does not work correct , the comment was for someone to review it! ;)
When you get time and review the patch, if the patch ain't good , we can just ask the submitter for a change/improvement.

Apart from that , if no one has noticed, Cheese has a similar feature. 
When recording a video , the record button changes to a stop button and icons change too. 
I'm not sure what is wrong with the patch here or if it is still wrong, but maybe someone interested in improving the patch could have a look at Cheese code as well.

I'm not interested in making this change in isolation, so I'm not going to review the patch.

(In reply to comment #48)
> I'm not interested in making this change in isolation, 
I'v tried reading this bug report and not really sure what you mean by "in isolation" here. 
Does this change need to be reviewed by the accessibility team? 

Or is it that Rhythmbox is not interested in such a change? Then, maybe we can close the bug as a "wont fix" ?

*** Bug 625391 has been marked as a duplicate of this bug. ***

Created attachment 167256 [details] [review]
Play/Pause/Stop button

This is a modified version of the earlier patch, with the button no longer as a toggle button.  Also dropped the call to gtk_toggle_action_set_active for the player action.

(In reply to comment #51)
> Created an attachment (id=167256) [details] [review]
> Play/Pause/Stop button
> 
> This is a modified version of the earlier patch, with the button no longer as a
> toggle button.  Also dropped the call to gtk_toggle_action_set_active for the
> player action.

This does not works perfectly from i18n point of view, see: https://bugs.launchpad.net/ubuntu/+source/rhythmbox/+bug/695411

I've just checked and this appears to have been fixed at some point in the past. This report should be updated accordingly.

I believe this has been fixed with
https://git.gnome.org/browse/rhythmbox/commit/?id=912811ecb
which is available in rhythmbox 2.99.

@Jeremy Bicha

Hi, thanks for the fix -- On 2.99.1, I'm sure that SPACE is not behaving as the play/pause button. SPACE and ENTER both restart the current track and CTRL+SPACE does nothing.

Googling reveals that the old CTRL+SPACE shortcut has been a problem to many, because they expect that SPACE alone toggles the play/pause state.

I should note that I'm on an external USB keyboard. My laptop has multimedia keys which I can't reach. I'm wondering what the effect of rb_shell_key_press_event_cb is:

https://git.gnome.org/browse/rhythmbox/tree/shell/rb-shell.c#n1987

A workaround is to make a global shortcut for controlling RB playback: http://askubuntu.com/questions/181651/how-to-assign-hot-keys-to-control-rhythmbox

Thanks,
Benjamin