Created attachment 52190 [details] [review]
patch

Check out all the "-" lines :)

This would also let us avoid having to find a solution to 313158, because we
don't have to worry about synchronisation.

This looks good, but there are a couple of issues, and I have a few ideas:

* For some reason Rhythmbox has a pause button when the playing source is
selected, and a stop button when another source is selected. This makes the
pause button on the toolbar disappear when a different source is selected. I
think the solution is to just have pause displayed irrespective of which source
is selected. Alternatively we could just add <toolitem name="Stop"
action="ControlStop"/> to the toolbar.

* The shuffle and repeat buttons aren't set correctly on startup, which is the
same issue as in bug 313158.

* The wider seek bar is okay, but it should probably shrink when the window is
made thinner. The minimum with on the window is now ~400px, at which point you
can't see the song info any more.

* Having a View->Show/Hide Toolbar menu item would be nice.

Created attachment 52234 [details] [review]
updated patch

* Added View/Toolbar menu item.
* Show only icons in toolbar when in small display mode
* Added ControlStop action to toolbar
* Changed seek bar back to original size

With the most recent patch, the pause button disappears when I press it, leaving
an empty space in the toolbar where the play button should be.  I have to change
to another source and then back to get the play button to reappear.

Jon, could you upload a screenshot as well?

Created attachment 52282 [details]
screenshot

Created attachment 52283 [details]
screenshot - small display mode

Jonathan, I think the button disappearing is a GTK+ bug.  I'll try to write a
test-case.  The button actually reappears when you press down but don't release
any of the other toolbar buttons.  Odd.

The pause button doesn't disappear anymore.  But now we don't have any shuffle
and repeat icons.  I think these might have been deleted from cvs during all the
icon changes lately.

Created attachment 53383 [details] [review]
updated patch

Synced with CVS and added a Burn CD action to the toolbar.

The icon name is "stock_media-shuffle"

Created attachment 54028 [details] [review]
new patch

Updated patch, which has correctly working icons with current cvs.

This also implements the button-seperation from bug 159824. Aside from being a
HIG recommentation and fixing some a11y issues mentioned on that bug, it also
fixes the odd graphical glitches, and allows you to use either pause or stop
irrespective of what the selected source is.

See Bug #320937 for a solution suggestion for the Search Entry and Hide Browser
problem, if a toolbar is introduced.

This have been discussed in
http://mail.gnome.org/archives/rhythmbox-devel/2005-November/msg00015.html

If a toggle button was used for play, there would be no need for changing icons
or seperated play/pause buttons. 

The play button would then be be "play/not play". Sort of a music On/Off switch.

Created attachment 54554 [details] [review]
patch with ideas from ML

This is an updated patch taking into account some of the suggestions from the
mailing list. The header (song info and seek bar) is put onto line, so that it
doesn't take up as much vertical space, the volume control has been moved up,
and the play button is not a toggle action.

There are some issues with the volume button, because it's not actually in the
toolbar, but I'm planning to work on that if people like this layout.

This looks very nice!  Maybe we should make the volume control a proper toolbar
button.  I think you are right that it belongs there.

With the song and artist info on one line we need to make sure that we ellipsize
each field when necessary or wrap the text.  Preferrably both.  It would be nice
to wrap first on the break between the song and artist.

Currently how the song header works is that is displays "Title from Album by
Artist" is that will fit, but will leave out the album to display "Title by
Artist" if it won't (with the artist being ellipsised). I guess we should fix it
to remove the artist if it's too small for that, and just have the ellipsised title.

Wrapping would also be good, but I'm not sure how to do that with several
different widgets.


I though I'd check with people to see if they liked the volume-on-toolbar before
actually putting it on the toolbar, because of the amount of work involved.
Making it a real toolbar item is moderately difficult, because BaconVolumeButton
will need to be modified to handle size changes and the like.

For me with this patch, when I play a song with a long title the RB window
expands to fit (even off the screen).

Created attachment 55450 [details] [review]
sync with cvs

Sync patch with cvs.  No additional changes.  Still looks great.

The toolbar is really better, and I really like it, but ... :)

One small problem is that the toolbar doesn't respect the menu layout define in
GNOME.
ie: I set "text beside icons" and rb doesn't set this layout as other application

One is the border beside the volume control which is not really beautiful

the third (sorry) is to know if the link to album and artist to last.fm should
dropped.

Thanks for your work

You are right, the toolbar item text position should follow the GNOME setting
when not using Small Display mode.

The volume control should be made into a GtkToolButton (perhaps something like a
GtkMenuToolButton?).  Then it will be added to the toolbar proper and fix the
problem you mention.

As for the web links, hopefully, we can fix the broken GnomeURL widget or get a
replacement in GTK.

humm ok, disregard the last "problem" concerning the artist link.

I have another comments

I saw that RB enables to remove the toolbar, but when it is remove, a big empty
space remains with just the volume control a the right.Perhaps the volume
control has to be moved elsewhere ?

I saw anoter tricky thing:
In the browser select an artist whith one song only
Make sure the random and shuffle are off
press "play"
now press the button "Repeat"

Result: the button 'Press' become sensitive.

thanks

It really shouldn't allow for the toolbar to be removed (and I'm also wondering
who on earth uses the small mode).

The toolbar hiding option is part of the HIG. 
http://developer.gnome.org/projects/gup/hig/2.0/toolbars.html

The space that remains is also because the volume button is not a toolbar-item.

Bastien, I think you're right that small mode doesn't fit with the music service
approach.  In a manner of speaking we use the notification icon as the small
mode now.

I use small mode, but I wouldn't really miss it if it went away.  If I found I
still wanted it, I'd probably write a pygtk/dbus client to display the same
information in a more compact form, which wouldn't be a bad thing.  Small mode
is just good enough to keep me from having to do that.

The issue of the border near the volume control and the fact that it stays when
the toolbar is hidden, is caused because it isn't actually in the toolbar - I've
got a half-done version with that fixed.

If we want to keep the artist/album link, something like SexyUrlLabel[0] would
work well, and let us put all the track-artist-album info in one label.


[0] from libsexy: http://wiki.chipx86.com/wiki/Libsexy

Created attachment 55480 [details] [review]
impoved patch

The volume control is now a proper toolbar item, so it works properly in small
mode and doesn't have the border glitches. I've moved the last.fm link code
inside rb-song-display-box, where it belongs.

The volume button looks really nice.  I'm not sure about moving the time to the
left side though.  One reason is that because we don't use a fixed width for the
text the whole line jumps side to side every second.  Another reason is that I
think the title is more fundamental and is easier to see against the left edge.

I'm starting to wonder how useful the song position slider is - as it is now. 
When I don't need to use it, it is just clutter and steals space from the title
and artist links.  When I do need to use it, it is way too small and the
granularity is way too large.  Maybe we should remove it from the interface by
default and have it optionally appear on a separate hbox underneath the title. 
The time text could still be displayed on the right hand side.  What do you think?

Anyway, thanks for your efforts with this James!  It is looking really nice.

Moving the time to the right was one of the suggestions from the ML, but after
using it for a while I agree that it's probably better on the left.

IIRC Muine has a menu command to bring up the seek bar. Perhaps we could do
something like that, or add an option to the View menu to control visibility -
which would let us make it bigger.

Err, swap left and right in that.

Created attachment 55484 [details]
latest screenshot

I may not be communicating this well.  Here's the way it looks with the latest
patch.

To #31. Is it this email you think of?
http://mail.gnome.org/archives/rhythmbox-devel/2005-November/msg00135.html

In case it is, here is an ASCII muck up =)

[Previous][Play][Next]|[Repeat][Shuffle]|[Create Audio CD]         [Volume]
[   Duration slider  ] [Time] [         Title Artist Album                ]

My suggestion is that the duration slider should have same length as the length
of Previous+Play+Next buttons.

Created attachment 55489 [details] [review]
new patch

Updated James' patch to move the time to the right, move the slider to a
separate row, not show slider by default, add menu option to show slider.  What
do you think?

If I am quick, then can I drop a comment before James =)

To me the most important feature of the duration slider is, that I can visually
see the progress of the song. I close to never look at the time.

So I would atleast miss that information.

Perhaps something can be worked out using the statusbar in Bug #320938 ?

With my last patch you can Ctrl-S or use the View menu to show the slider.

Cool patch =)

But I think people would expect a duration slider as default in a music player.

I think removing the duration slider from the default interface have a much too
high prize for the those using it.

Created attachment 55514 [details] [review]
yet another patch

This fixes some refcounting problems, and adds the gconf keys to the schema.

I agree with Martin that the seek bar should be present by default, but having
an option to remove it is fine. Whether it is next to or below the other song
info doesn't worry me personally.

I'm not sure why the toolbar doesn't change when you modify the layout (with
Menus & Toolbars preference), GtkToolButton should handle that automatically -
and looking at the code for it, it looks like it would.

I have now been using the long duration bar for a day or so, and come up with
this suggestion for a toolbar layout, that might satisfy everybody? =)

[Play] [              Title Artist Album             ] [Vol.]
[Prev] [Next] [         Duration slider            ] [ Time ]

My reason for this suggestion is, that the play button is closely related to
SongInfo (play current song on/off), and Previous and Next are closely related
to Time.

Placing the Play button and Volume button at each end of the SongInfo gives a
centered SongInfo.

The length of Time is the same as the length of Previous+Next buttons, so the
Duration Slider is centered.

Would this work?

Created attachment 55539 [details] [review]
patch

Now follows preferred toolbar layout.  Fixed the tooltip for ControlPlay to say
"Stop playback" when playing.  Removed the hseparator from below the
shell-player since the seek bar kinda serves that purpose now.

After doing some user testing, I now agree that the seek bar should be visible
by default.  The test scenario I used was:
"Listen to the second half of the audio program in the most recent episode of
the xxx podcast feed".

Should note that some people aren't figuring out that the play button is a
toggle and can be used to stop/pause.  Hopefully, the updated tooltip will
help.

The patch from #41 gives me Segmentation Fault with latest cvs checkout. =(

(rhythmbox:31672): GdkPixbuf-CRITICAL **: gdk_pixbuf_new_from_file: assertion
`filename != NULL' failed

(rhythmbox:31672): Rhythmbox-WARNING **: Unable to load icon rhythmbox-podcast

(rhythmbox:31672): Gtk-WARNING **: ControlPauseMenu: missing action

(rhythmbox:31672): Gtk-WARNING **: ControlStopMenu: missing action

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(rhythmbox:31672): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion
`G_TYPE_CHECK_INSTANCE (instance)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(rhythmbox:31672): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion
`G_TYPE_CHECK_INSTANCE (instance)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(rhythmbox:31672): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion
`G_TYPE_CHECK_INSTANCE (instance)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(rhythmbox:31672): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion
`G_TYPE_CHECK_INSTANCE (instance)' failed

(rhythmbox:31672): Gtk-WARNING **: PauseTray: missing action

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): Gtk-CRITICAL **: find_menu_position: assertion `GTK_IS_WIDGET
(prev_child)' failed

(rhythmbox:31672): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(rhythmbox:31672): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion
`G_TYPE_CHECK_INSTANCE (instance)' failed
Segmentation fault

Martin: That kind of layout can't really be done with a toolbar. I guess it
would be possible with two vertically packed toolbar, but a) that pretty dodgy
and b) toolbars generally only contain "action" items which do something, and
the information doesn't.


Those errors usually occur if you run Rhythmbox uninstalled (shell/rhythmbox)
but didn't compile it with --enable-uninstalled-build.

James: You surdenly know your stuff =) --enable-uninstalled-build did the trick.

I understand your point about action only widgets in one toolbar, but I don't
think it look too bad =)

Created attachment 55578 [details]
Toolbar layout suggestion with two toolbars.

Created attachment 55579 [details]
Toolbar layout suggestion with three toolbars.

It is likely that the third toolbar should be beneath the two others. Perhaps
the third toolbar could have the Hide Browser and Search Entry aswell?

Rather than debating the virtues of different layouts here, I think it would be
better to land the base toolbar stuff in cvs, and then discuss layout changes on
the ML. I think the general consensus is that using toolbar(s) is good, and
there are a couple of related things (source-specific toolbar items, and the
play-order menu) that are blocking on this.

I think the patch from comment #41 is fine, and if people don't like the moved
seek bar, we can always move it back.

OK, I committed it.  I agree about the seek bar.  Thanks James!

2005-12-05  William Jon McCann  <mccann@jhu.edu>

	* widgets/rb-song-display-box.[ch] (rb_song_display_box_init):
	Hide internals and add tooltips.
	(rb_song_display_box_size_allocate): Hide internals.
	(sanitize_string, info_url): Copied from rb-header.
	(rb_song_display_box_sync): New public function.

	* widgets/rb-header.[ch]: Use G_DEFINE_TYPE.
	(rb_header_init): Use a separate row for the seek bar.
	(rb_header_finalize): Don't unref widgets.
	(rb_header_sync): Use rb_song_display_box_sync.
	(rb_header_set_urldata)
	(rb_header_set_show_artist_album): Hide instead of remove widgets.
	(rb_header_set_show_position_slider): Set visibility of seek bar.
	(rb_header_set_show_timeline): Set sensitivity of seek bar.
	(rb_header_get_elapsed_string): Don't shift when you first start playback.

	* shell/rb-statusbar.c (rb_statusbar_init)
	(rb_statusbar_sync_state): Remove shuffle and repeat controls.

	* shell/rb-shell.c (rb_shell_finalize): Destroy tooltips.
	(rb_shell_constructor): Remove hseparator.  Monitor
	toolbar visibility settings.  Add toolbar.  Add
	volume button to toolbar.  Add tooltips for volume button.
	(rb_shell_view_toolbar_changed_cb) 
	(rb_shell_sync_toolbar_visibility)
	(toolbar_visibility_changed_cb): Set visibility of
	toolbar.
	(rb_shell_sync_smalldisplay): Set the toolbar style to
	icons only when in small display mode.
	(rb_shell_volume_widget_changed_cb): Save volume setting.

	* shell/rb-shell-player.c (rb_shell_player_constructor):
	Remove pause and stop actions.  Make play action "important"
	so that text is visible in horizontal toolbar position.
	Remove control buttons.
	(rb_shell_player_init) 
	(rb_shell_player_sync_song_position_slider_visibility):
	Set visbibility of seek bar.
	(rb_shell_player_cmd_play, rb_shell_player_playpause):
	Make play action a toggle.
	(rb_shell_player_sync_volume): Volume control moved
	to player.
	(gconf_song_position_slider_visibility_changed):
	Set visbibility of seek bar.
	(rb_shell_player_sync_buttons):
	Set sensitivity of actions instead of buttons.
	(rb_shell_player_playing_changed_cb): Add callback
	for player notify::playing changes.

	* lib/rb-preferences.h: Add hide toolbar and seek
	bar key names.

	* data/ui/rhythmbox-ui.xml: Add hide toolbar and
	seek bar menu items.  Remove pause and stop actions
	from the control menu.  Add a toolbar.

	* data/rhythmbox.schemas: Add toolbar_hidden
	song_position_slider_hidden keys.

	Patch by: James Livingston  <jrl@ids.org.au>
	          William Jon McCann  <mccann@jhu.edu>
	Fixes #316238.

Okay. I keep them for after 0.9.3. Perhaps I might prefure the just submitted
layout =)