Created attachment 48201 [details] [review]
patch

Adds player options to interface:
* play/pause button in menu
* play/pause button in main interface
* seek slider for position tracking, only visible when playing

Works like this:
* press play button or double click a song in the list, it'll play
* pause pauses
* seek using slider

FIXME:
* stop when the extract button is clicked, so we release the device (this is a
blocker before the patch can be applied)
* add a play icon (like totem does in its playlist) to indicate the currently
playing song (this is a usability issue, and thus a potential blocker)
* possibly (future) a random playback mode (unimportant for now)

Created attachment 48202 [details]
screenshot when not playing

Created attachment 48203 [details]
screenshot when playing

Does double-clicking a song to play it work?
Also, it might be a better idea to have the playback bits on another different
line, possibly in between the album details at the top and the treeview, this
would also allow to put in a nice little volume control.

Double-clicking works, yes.

Outstanding issues as I see it

Doesn't disable Extract when playing

Cast failure when switching track

GLib-GObject-WARNING **: invalid cast from `GtkListStore' to `GtkTreeView'
Gtk-CRITICAL **: gtk_tree_view_get_model: assertion `GTK_IS_TREE_VIEW
(tree_view)' failed
Gtk-CRITICAL **: gtk_tree_model_iter_n_children: assertion `GTK_IS_TREE_MODEL
(tree_model)' failed

No indication in the treeview what track is being played

However, ROCK ON.

Also needs to change the window title

I've committed this patch to CVS on the sj-player-branch.

This patch will only close when it's merged, so let's get this rocking.

Does this mean that gnome-cd can be dropped? That would be wonderfull, imo
gnome-cd is the only application in gnome that is the complete opposite of our
beloved HIG :)

Michael: notice that Ronald is the current maintainer of gnome-cd. Yes, gnome-cd
will DIE DIE DIE.

Death to gnome-CD! Hail the new king!

Also fixed the cast warning and device setting.

Remaining FIXMEs:
* tell what's playing in liststore
* set title
* add error dialog (g_warning now)
* stop when eject/extract buttons are clicked (call stop())

havoc had me convinced.. for a pure CD player, the checkboxes "extract" are
distracting, and there is no previous/next/eject/volume button.
maybe i'll use goobox on autoplay of CDs and occasionnally SJ to rip them
(almost never rip them because of disk space). But I agree that gnome-cd was ugly.

=====

http://www.gnomefiles.org/app.php?soft_id=531

 Latest Version: 0.9.91
* Added goobox/general/use_sound_juicer gconf preference to use Sound Juicer to
extract tracks instead of the internal ripper.

I still don't understand why Goobox, a player/ripper, has an option to use SJ as
a ripper.

I'm thinking maybe the Play button should respect the check boxes too.

Bastien mentioned adding a volume button.  Next/prev buttons are useless. SJ has
an Eject button.

Emmanuel, that discussion is completely out-of-place here. I'm fine with
discussing that, but gnome-multimedia@gnome.org is a better place than this bug
report. This bug report is about adding a player interface to S-J.

Construction of the GStreamer pipeline could fail leaving the user with the
error-message: "Failed to create audio output".

As a GStreamer newbie I have no idea when the construction fails but I already
picture my dad staring at the sentence and having no clue what to do next.
Shouldn't this lead to some error dialog giving hints for resolving the problem? 

(Slightly offtopic: In my ideal world there should be some
distribution-dependant component catching this error and checking trivial
configuration thingies and operating conditions and presenting some kind of
problem solver wizard helping the end-user. At least a more generic way of
handling these kind of errors would be nice as more and more applications will
use GStreamer.)

Bram: the message could be improved, yes. Let's do that in the future. In
practice, it shouldn't happen anyway, because it means a borked sound
configuration or you having messed with gconf keys manually.

Created attachment 48212 [details] [review]
volume patch

This patch adds a volume button to current CVS, thus letting the user control
volume. The button is a direct copy of the Totem one, and has the same issue
which is one of the reasons why it is not in Totem CVS yet (it sometimes loses
keyboard/mouse grab, thus leaving it popped up and not being able to get rid of
it unless you very precisely hit the button below it).

Anyway, it gives a UI idea, I personally like it.

I've just committed basic track state icons.  It needs a bit of love:

* when they first appear the treeview re-flows as the renderer previously took
no space
* the extractor doesn't set the EXTRACTING state.

Created attachment 48215 [details]
volume button UI screeny

I, too, will be happy to see the ugly gnome-cd go. The current UI, however,
looks a bit like someone took a cd-ripper and glued a cd-player on top of it. :)
Would you please consider making the cd-player part more prominent so it doesn't
look like such an afterthought. Maybe something along the lines of the mockup
I've attached.

You'll notice that it's modelled after the excellent Totem media player. The
buttons are smaller (and without text labels), they are divided to two sides and
I've replaced the Re-read button with an Eject button. (Feel free to keep the
Re-read button, I've just never understood what it's for.)

The slider could also be used to show the progess to the ripping instead of a
separate window.

Created attachment 48221 [details]
mockup

That should read: "progress of the ripping".

The eject button does not belong in the main window, neither do the prev/next
buttons. The play button should indeed be more prominent, but I don't really
know how to do that... Putting it to the left is fine with me.

Yeah, the prev/next buttons are redundant, since the list of song is always
visible. How about something like mockup2 then? I placed the Play button to the
far left to make it more prominent.

Does anyone like the idea of using the slider to seek in Play mode and using it
to show the progress of the ripping in Extract mode? Bug 165533 has something
similar.

Created attachment 48225 [details]
mockup2

Not really. A seeking scale is not the same as a progress bar. Having the
progress bar in the statusbar and the seeker next to the play button sounds sane
to me. I'm not an expert in fancy stuff like button order and have no real
opinion on it. I like your layout, but personally see little difference with the
current status. :). Maybe Ross has a better comment. ;).

Basically agreed with Ronald.  

Re-using the slider as a progress bar is wrong.  My plans for replacing the
progress window with a progress bar involve embedding it into the status bar (as
GnomeApp does).

Also, HIG places buttons in the bottom right of dialogs, so unless you have a
good reason I'd prefer to keep them there.

Don't forget this is not a dialog, really... Or we just bug Seth. :).

Like Ronald said, the "buttons to the right" applies mainly to dialogs, not so
much to media players which Sound Juicer now is. The problem with the layout is
that it's different from other media players and users will have to get used to
yet another unintuitive button layout.

IMO, the deciding factors for the button layout are:
	(1) Will there be a Re-read button?
	(2) Will the slider always be visible?

I don't think there should be a Re-read button. That function doesn't seem
important enough to warrant a separate button, a menu item should be enough,
like with Eject, and I think that the slider should always be visible (and maybe
be replaced by a progress bar when ripping), because controls appearing and
disappearing can get confusing. And also if the slider will appear and disappear
according to the Play mode, the volume control should too.

If you are going to have a Re-read button and the slider will always be visible,
the layout that makes most sense is one where the ripping controls (Re-read and
Extract) form one group and the playing controls (Play, Volume, slider) form
another with the slider being between them, like so:

[Play][V][-----=-----][Extract][Re-read]

If you are not going to have a Re-read button and the slider will always be
visible, buttons for the two function of Sound Juicer (playing and ripping)
should be presented next to each other like so:

[Extract][Play][-----=-----][V]

If the slider will keep appearing and disappering, it should be placed to the
right side of the window, because the slider should be to the right of the Play
button (the Play button and the slider are cause and effect and most people
using LTR scripts consider things going from left to right) and because empty
spaces in the middle look bad. So here are the possible layouts for that:

With Re-read button (not playing):
[Extract][Re-read][Play]

With Re-read button (playing):
[Extract][Re-read][Play][-----=-----][V]

Without Re-read button (not playing):
[Extract][Play]

Without Re-read button (playing)
[Extract][Play][-----=-----][V]

Regardless of what you decide, I ask that you to consider the matter further
before committing to a button layout. These things are too often decided without
much thought and as a result we have many less than optimal designs that are
difficult to change afterwards.

Note that the re-read button disappeared mid-afternoon yesterday.

Created attachment 48266 [details]
mockup3

Here one more mockup for the ripping mode. (The Play and volume buttons should
of course be dimmed.)

This is OK with me. As for visibility of slider, it should not be visible when
idle. A progress bar only when extracting, and a scale only when playing. Else,
nothing. The volume button can stay when not playing, but insensitive. When
extracting, we can make the play button insensitive too, that's fine.

As said before, the volume button above needs fixage for the grab-bug before it
can be used. Right now, it's not ready. Also, Totem uses a [ 0, 100 ] scale, SJ
uses a [ 0, 1 ] scale, and for some reason it assumes 100 somewhere in the code
for popup placement. It's a while since I wrote it, so need to dive into it again...

Ronald, have you considered my proposal [1] to make volume button flat, to
distinguish it from a normal click-able button?

[1] http://bugzilla.gnome.org/show_bug.cgi?id=163313#c31

COMMITTED TO HEAD.

WE HAVE A PLAYER.

Don't comment any more here, open new bugs for any missing features.

> As for visibility of slider, it should not be visible when idle. 

Having user interface components appear and disappear is not good, it hurts
discoverability adn the dead space is not being used anyway.  
(dont know where the HIG recommends agianst it but if I looked I'd bet I'd find
it.  will try and check later)

There is a patch I plan on integrating which uses that space to show progress
bars when ripping.

If you want to talk about whether these widgets should be always visible or not,
please open a new bug.  This bug is about adding CD functionality and is fixed.