Created attachment 67808 [details]
Mock up of the popup new track notification

The top half of the image is the "before" screenshot, from 0.9.3.1

The bottom half of the image is a mock up where the "media icon" was replaced with a 50 x 50 pixel album cover.

I think a larger cover image would be better, maybe 100x100, but I used what seemed to be the size of the current icon.

Replacing the standard tooltip with either libnotify notifications or libsexy's SexyTooltip would be required to do more complex tooltips.


Regarding inserting art, we could probably get the shell to have a signal which gets emitted when one of these is getting created, and have handlers return a GtkWidget* which gets inserted into the notification/tooltip.

Created attachment 70338 [details] [review]
metadatabus.patch

Created attachment 70339 [details] [review]
metadatabus-artdisplay.patch

Created attachment 70340 [details] [review]
trayicon-arty-notify-and-tooltip.patch

ok, let's see if I can explain this...

Patches against current CVS unless otherwise noted.

metadatabus.patch: adds a signal "entry_extra_metadata_notify" to RhythmDB. Anyone who wants can use this to transmit objects (packed as a GValueArray, because PyGTK already knows how to marshal/unmarshal that) to anyone interested. It's a detail signal; the detail is the name of the object.

metadatabus-artdisplay.patch: ArtDisplayPlugin emits "entry_extra_metadata_notify::rb:coverArt" when it gets art. This won't apply to current cvs, because it's against my reworking of artdisplay in bug 346736. Should be easy to apply, though.

trayicon-arty-notify-and-tooltip.patch: fun.
rhythmbox/widgets/eggtrayicon.c:
  * mod egg_tray_icon_notify to not escape text (we do it further up the stack)
  * some necessary minor bug fixes
rhythmbox/shell/rb-tray-icon.{c,h}:
  * use a SexyTooltip. Layout code stolen from gtk_message_dialog.c. Tooltip suppressed when notification show, but no timing code (as yet); tooltip displays immediately on mouse over.
rhythmbox/shell/rb-shell.c:
  * listen to "entry_extra_metadata_notify::rb:coverArt". Display art in notification and tooltip. Also, use "by <b>%s</b> from <b>%s</b>" in notification and tooltip (i.e. make artist and album bold). Tooltip shows artwork, song, artist, album and duration.

Need patch in bug 350107 to get libsexy to display tooltips in the correct position. 

Could someone set the "Depends on" field?

libnotify/notification-daemon bugs this depends on:

http://trac.galago-project.org/ticket/75 dbus 0.61 breaks notification-daemon icons
http://trac.galago-project.org/ticket/77 assertion `style->attach_count > 0'
http://trac.galago-project.org/ticket/78 libnotify reuses stale id if notify_notification_show called on closed notification

They're fixed in current svn, but if you're running <=libnotify-0.4.2 or <=x11-misc/notification-daemon-0.3.5 you'll need to apply the patches.

Created attachment 70341 [details] [review]
metadatabus.patch

oops, forgot the C marshal

Created attachment 70350 [details] [review]
trayicon-arty-notify-and-tooltip.patch

Prevent flickering when the tooltip is changed by hiding and queueing a reshow; also, reposition automatically on size-allocate.

Created attachment 70514 [details] [review]
trayicon-arty-notify-and-tooltip.patch

Remember to escape the song title as well for the notification.

I'd been thinking about something like "entry_extra_metadata_notify" for a while, but I think the patch only adds half of it: it provides for things to send change notification, but it doesn't provide a way of getting the current value.

I think the best way of doing this would be to have another signal "entry_extra_metadata_get", which the db emits when someone calls "GValue* rhythmdb_entry_get_extra_metadata (RhythmDBEntry*, const char *)". The signal would be like _notify except that it works in reverse, it would return a GValue* and have an accumulator set which stops when a handler (which plugins set) returns a non-NULL value. The accumulated value would then be returned to the caller from rhythmdb_entry_get_extra_metadata.


This would also mean that the art-display widget wouldn't use the db directly, it could use rhythmdb_entry_get_extra_metadata instead. Then the art-db could connect to the get signal using AFTER, and so things like the ipod plugin could connect normally, and so override it if there were art present on the ipod.



Regarding the display-art-in-tooltip bit: looks okay to me, except that it always in the top-left corner of my screen. Also, when there is no known art it dsiplays the "not found" icon, rather than just noth having it present.

Created attachment 70789 [details] [review]
trayicon-arty-notify-and-tooltip.patch

pass the right number of arguments to g_markup_escape_text

(In reply to comment #12)
> I'd been thinking about something like "entry_extra_metadata_notify" for a
> while, but I think the patch only adds half of it: it provides for things to
> send change notification, but it doesn't provide a way of getting the current
> value.
Sounds good, as long as we can get the semantics right.

> I think the best way of doing this would be to have another signal
> "entry_extra_metadata_get", which the db emits when someone calls "GValue*
> rhythmdb_entry_get_extra_metadata (RhythmDBEntry*, const char *)". The signal
> would be like _notify except that it works in reverse, it would return a
> GValue* and have an accumulator set which stops when a handler (which plugins
> set) returns a non-NULL value. The accumulated value would then be returned to
> the caller from rhythmdb_entry_get_extra_metadata.
OK. I guess refcounting will be handled by GValue. We'll need a GValue marshal/unmarshal for python-gobject; we can register one ourselves for now but it should be in gobject.
 
> This would also mean that the art-display widget wouldn't use the db directly,
> it could use rhythmdb_entry_get_extra_metadata instead. Then the art-db could
> connect to the get signal using AFTER, and so things like the ipod plugin could
> connect normally, and so override it if there were art present on the ipod.
How does this work with the async nature of art search? Presumably the art db only emits in response to "entry_extra_metadata_get" if it already has art; I guess it still emits "entry_extra_metadata_notify" when a search succeeds? Should it only begin searching in response to "entry_extra_metadata_get"?
 
 
> Regarding the display-art-in-tooltip bit: looks okay to me, except that it
> always in the top-left corner of my screen. 
Odd. Which libsexy are you using?

> Also, when there is no known art it
> dsiplays the "not found" icon, rather than just noth having it present.
That was for consistency with the notification, and to make it less visually disruptive if artwork arrives while the tooltip is visible. I'll remove it if that's what you'd prefer.

re GValue marshal/unmarshal: bug 351072

Created attachment 71411 [details] [review]
metadatabus-artdisplay.patch

Created attachment 71412 [details] [review]
trayicon-arty-notify-and-tooltip.patch

Created attachment 71414 [details] [review]
metadatabus.patch

Implement "entry_extra_metadata_request" signal.

Note it is not used as of this patch; my copy of the artdisplay plugin is different from CVS (bug 346736) so I wouldn't be able to write or test a useful patch. However the signal is there.

Also switch to use GValue, not GValueArray. Patch contains marshaller with registration; adds a dnl'd out section to pkgconfig for when pygobject gets fixed (bug 351072) and the marshaller becomes redundant.

(In reply to comment #12)
> Regarding the display-art-in-tooltip bit: looks okay to me, except that it
> always in the top-left corner of my screen. 

This is caused by bug 350107.

Created attachment 72772 [details] [review]
updated extra metadata patch

This adds another signal for gathering all extra metadata for an entry, moves the guts of rb_shell_get_song_properties into rhythmdb, and emits dbus signals for changes to the playing song.  I mostly did this for iradio pluginification.

I had to add the field name as a parameter on the extra-metadata-notify signal for  dbus signal emission.

Nice.

1. You've left out the patch to rb-marshal.list.

2. The gather signal isn't detailed, so doesn't need 
G_SIGNAL_DETAILED.

3. The entry_extra_metadata_request member on the RhythmDB struct is inconsistent with the signal declaration and marshaller:
	GValue *(*entry_extra_metadata_request) (RhythmDB *db, RhythmDBEntry *entry, const char *field);
but
	rhythmdb_signals[ENTRY_EXTRA_METADATA_REQUEST] =
		g_signal_new ("entry_extra_metadata_request",
			      G_OBJECT_CLASS_TYPE (object_class),
			      G_SIGNAL_RUN_LAST | G_SIGNAL_DETAILED,
			      G_STRUCT_OFFSET (RhythmDBClass, entry_extra_metadata_request),
			      rhythmdb_entry_extra_metadata_accumulator, NULL,
			      rb_marshal_BOXED__BOXED,
			      G_TYPE_VALUE, 1,
			      RHYTHMDB_TYPE_ENTRY);

4. The field argument to notify and request should be documented, just to say that it is the same as the ::detail argument.

Created attachment 72820 [details] [review]
updated

Thanks for picking up those mistakes.  This patch should do a little better.

Created attachment 72863 [details] [review]
metadatabus-artdisplay.patch

Updated to above changes. Not updated to current cvs, but will still apply cleanly.

Created attachment 72864 [details] [review]
trayicon-arty-notify-and-tooltip.patch

ditto.

I've committed the extra-metadata signal patch in order to get the iradio plugin patch in.

Created attachment 74151 [details] [review]
updated patch

Updated for cvs.

This doesn't work for me because pygtk-codegen (2.10.3 at least) doesn't handle GValue parameters or return types. Do you have a magic version that doesn, or have written overrides?

Also, on my dual-head (Xinerama) system the tooltip gets displayed on the wrong monitor.

Created attachment 79065 [details] [review]
covers-on-notify-and-sexy-tray-tooltip.patch

Updated patch (for 0.9.7).

(In reply to comment #27)
> This doesn't work for me because pygtk-codegen (2.10.3 at least) doesn't handle
> GValue parameters or return types. Do you have a magic version that doesn, or
> have written overrides?
Nah, I was emitting the signal using GObject.emit(), and letting the emit marshaller do the heavy lifting.  Your bindings in rhythmdb.override aren't quite correct; the GValue needs to be given a type from pyg_type_from_object().  This patch contains the fix.

> Also, on my dual-head (Xinerama) system the tooltip gets displayed on the wrong
> monitor.
Hm.  I'm using sexy_tooltip_position_to_widget(), so if there's a bug it's in libsexy.  I don't use Xinerama, so reproing the bug might be difficult.

Walk-through of patch:

plugins/artdisplay/artdisplay/__init__.py: emit metadata with "rb:coverArt" when art arrives; respond to metadata request for "rb:coverArt".

shell/rb-shell.{c,h}: cache song title, secondary (artist/album) and album art; listen for "rb:coverArt" metadata and update notification with album art if still visible; use markup for notification and tooltip secondaries ("by <b>%s</b>", etc.).

shell/rb-tray-icon.{c,h}: use SexyTooltip; support markup in notifications and tooltips

bindings/python/rhythmdb.override: fix _wrap_rhythmdb_emit_entry_extra_metadata

widgets/eggtrayicon.{c,h}: support markup in notifications and tooltips, fix a refcount bug

(In reply to comment #29)
> > Also, on my dual-head (Xinerama) system the tooltip gets displayed on the wrong
> > monitor.
> Hm.  I'm using sexy_tooltip_position_to_widget(), so if there's a bug it's in
> libsexy.  I don't use Xinerama, so reproing the bug might be difficult.

Right, I put together a Xinerama setup using the Xdmx-on-Xnest trick, and couldn't repro the bug. Perhaps it's been fixed in recent libsexy; I'm running 0.1.10 here.

Looks okay to me, the only question is whether we want to depend on libsexy or just copy sexy-tooltip in like we do with sexy-entry.

What versions of libnotify and/or dbus or other packages are necessary for this to work out of the box on the rhythmbox trunk?

(In reply to comment #32)
> What versions of libnotify and/or dbus or other packages are necessary for this
> to work out of the box on the rhythmbox trunk?

libnotify: older versions will compile & work, but may be ugly.  0.3 is preferred.
dbus: no specific dependency.
libsexy: recent is good, as older versions have bugs.  0.1.6 is the minimum API-wise.  0.1.10 seems to work bug-free.

Committed to cvs (with sexy-toolip copied in). Thanks.

This does not work for me (cover art is shown as a missing image icon), and worse, the tooltip aligns far left on the panel, as opposed to being above the tray icon (which is on the far right).

These are the versions of the dependencies I'm using.  None of them have been patched, they are the upstream release, AFAIK.

$ rpm -q libnotify libsexy dbus
libnotify-0.4.2-5.fc6
libsexy-0.1.10-1.fc6
dbus-1.0.1-8.fc6

Is it necessary to patch libnotify (or any of the deps) itself?  Comment #33 implies that upstream releases above a certain version should work OK.

Ah, so it appears that the cover art only appears in the tooltip if it located in a cover.jpg inside the album directory.  It doesn't appear to work for covers located by Amazon which are downloaded into the ~/.gnome2/rhythmbox/covers/

(In reply to comment #36)
> Ah, so it appears that the cover art only appears in the tooltip if it located
> in a cover.jpg inside the album directory.  It doesn't appear to work for
> covers located by Amazon which are downloaded into the
> ~/.gnome2/rhythmbox/covers/

That's odd.  It shouldn't matter; in either case the pixbuf is sent over the metadata bus as "entry_extra_metadata_notify::rb:coverArt", immediately after being set onto the art widget.

(In reply to comment #35)
> This does not work for me (cover art is shown as a missing image icon), and
> worse, the tooltip aligns far left on the panel, as opposed to being above the
> tray icon (which is on the far right).
> 
> These are the versions of the dependencies I'm using.  None of them have been
> patched, they are the upstream release, AFAIK.
> 
> $ rpm -q libnotify libsexy dbus
> libnotify-0.4.2-5.fc6
> libsexy-0.1.10-1.fc6
> dbus-1.0.1-8.fc6
> 
> Is it necessary to patch libnotify (or any of the deps) itself?  Comment #33
> implies that upstream releases above a certain version should work OK.

Oh... oops (well, something stronger).  I knew there was a reason I'd set bug 350107 as a blocker on this bug.

Basically, sexy_position_to_tooltip() uses gdk_window_get_root_origin(), which is wrong for reparented windows (actually wrong for all windows, but /very/ wrong for reparent windows).  (Should be gdk_window_get_origin().)   The simplest fix, still allowing use of current external libsexy, will be to copy sexy_position_to_tooltip() into shell/rb-tray-icon.c and override it there.

Created attachment 79727 [details] [review]
tooltip-position-origin-fix.patch

Patch as suggested in previous comment.

Created attachment 79729 [details] [review]
tooltip-position-each-time.patch

Another patch, sorry.  This one fixes the case when the tray is repositioned; the tooltip needs to be moved along with the icon.

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

Patches fix the tooltip position problem, OK to commit?

The displaying of art in ~/.gnome2/rhythmbox/covers/ is not fixed, however, and I'm not the only one: see the duplicate bug #394351.

Interesting enough the workaround mentioned on that bug:

> 'album art' plugin has to disabled and enabled while playing each song for the 
> artwork to appear in the tooltip in the notification area.

doesn't work for me.

(In reply to comment #36)
> Ah, so it appears that the cover art only appears in the tooltip if it located
> in a cover.jpg inside the album directory.  It doesn't appear to work for
> covers located by Amazon which are downloaded into the
> ~/.gnome2/rhythmbox/covers/

Must be a control flow issue; covers in ~/.gnome2/rhythmbox/covers/ are returned directly (that is, the metadata is emitted above the call stack from the playing-song-changed signal) but covers in the album directory are loaded async and so arrive on the next mainloop.

OK, so if the ArtDisplay plugin gets playing-song-changed before rb-shell.c does then it will emit the cover before rb-shell.c is ready for it; that would be fixed by having ArtDisplay connect to playing-song-changed with G_CONNECT_AFTER.

However that doesn't seem consistent with Alex's comment 43.  Still, would you mind testing that potential fix anyway?

(In reply to comment #44)

> However that doesn't seem consistent with Alex's comment 43.  Still, would you
> mind testing that potential fix anyway?

Sure, but I'm not sure where the change needs to be done and the syntax required, is it in the rb-shell.c or the Python call?

self.pec_id = sp.connect ('playing-song-changed', self.playing_entry_changed) 

(In reply to comment #45)
> Sure, but I'm not sure where the change needs to be done and the syntax
> required, is it in the rb-shell.c or the Python call?
> 
> self.pec_id = sp.connect ('playing-song-changed', self.playing_entry_changed) 

Yeah, try changing that line to
  self.pec_id = sp.connect_after ('playing-song-changed', self.playing_entry_changed)
Thanks.

(In reply to comment #46)

> Yeah, try changing that line to
>   self.pec_id = sp.connect_after ('playing-song-changed', self.playing_entry_changed)

Nope, makes no difference...  :-/

I've committed the tooltip positioning fixes to svn.  Thanks, Ed.

A few things I should have noticed earlier:
- tooltip should say "Music Player" instead of "Rhythmbox" for consistency with the window title
- the markup should match that used in the main UI - mainly, italics instead of bold.

I'd also like it to leave out the icon in the tooltip if the art display plugin is disabled (or really if nothing will provide images).

(also, the wrong patch was marked as being committed)

Just did a svn snapshot and the tooltip position is indeed fixed.
There is still however the part discussed in bug #394351 where album art does not show in the tooltip.

(In reply to comment #47)
> >   self.pec_id = sp.connect_after ('playing-song-changed', self.playing_entry_changed)
> 
> Nope, makes no difference...  :-/

OK, I managed to reproduce it (turns out almost all of my music has album
covers in the folders, which is why it worked for me).

The problem is that ArtDisplay also connects to 'playing-changed', which is
emitted before 'playing-song-changed'. (I think it's still needed, though.)  So the cover art is emitted before the shell is ready for it.

The obvious solution is to emit the cover art in an idle closure. I'll post a
patch.

Created attachment 80068 [details] [review]
cover-art-emit-on-idle.patch

Emit cover art in an idle closure.

Created attachment 80069 [details] [review]
correct-tooltip-title-and-use-italics.patch

For comment 48:
> A few things I should have noticed earlier:
> - tooltip should say "Music Player" instead of "Rhythmbox" for consistency with
> the window title
> - the markup should match that used in the main UI - mainly, italics instead of
> bold.

Fixed.

(In reply to comment #48)
> I've committed the tooltip positioning fixes to svn.  Thanks, Ed.
Thanks.
> 
> I'd also like it to leave out the icon in the tooltip if the art display plugin
> is disabled (or really if nothing will provide images).

Hmm. How would that work? Should there be a way to advertise capability for metadata?

Regarding comment 48, Jonathan Matthew wrote:
> I'd also like it to leave out the icon in the tooltip if the art
> display plugin is disabled (or really if nothing will provide images).

Would implementing Bug 345975 (album covers embedded in files e.g. mp3 ID3 tags) and this fix work nicely together?

I would expect that if we get a cover image from an mp3 file's ID3 tags that it would be used in these tool tip notifications (even if the python album art plugin is disabled).

(In reply to comment #51)
> Created an attachment (id=80068) [edit]
> cover-art-emit-on-idle.patch
> 
> Emit cover art in an idle closure.

Works for me and looks good.  I notice, however that the covers appear in the tooltip, but do not appear in the pop-up notification on track change (not even a placeholder image appears), is it supposed to also include the cover art in the notification too, or is that not implemented/possible yet?

 

(In reply to comment #52)

> > A few things I should have noticed earlier:
> > - tooltip should say "Music Player" instead of "Rhythmbox" for consistency with
> > the window title
> > - the markup should match that used in the main UI - mainly, italics instead of
> > bold.
> 
> Fixed.

A fairly trivial patch, tested and committed.
 

(In reply to comment #55)
> Works for me and looks good.  I notice, however that the covers appear in the
> tooltip, but do not appear in the pop-up notification on track change (not even
> a placeholder image appears), is it supposed to also include the cover art in
> the notification too, or is that not implemented/possible yet?

Works fine here; could be a problem with notification-daemon/libnotify. See comment 8. (Yeah, I forgot about that when I wrote comment 33. Sorry.)

using notifcation-daemon 0.3.6 and libnotify 0.4.3
I still get a 'missing icon' icon the tooltip and nothing in the pop-up notification.
Any idea guys?

(In reply to comment #58)
> using notifcation-daemon 0.3.6 and libnotify 0.4.3
> I still get a 'missing icon' icon the tooltip and nothing in the pop-up
> notification.
> Any idea guys?

To get the cover art in the tooltip you still need to apply patch #80068 against SVN (hasn't been committed yet), and for the notification pop-up, it appears that  libnotify needs patches, see comment #8 and comment #57, so libnotify 0.4.2 doesn't work for me (not sure about libnotify 0.4.3).

Ok thank you Alex. I will try that now. 

Patch http://bugzilla.gnome.org/attachment.cgi?id=80068 does indeed fix it for me.
It now shows album art correctly everytime both in tooltip and notification pop-up.

But what about the time where there is not album art. Could we not show the ugly 'missing icon' icon when there is no album art?

(In reply to comment #61)
> But what about the time where there is not album art. Could we not show the
> ugly 'missing icon' icon when there is no album art?
 
It's doable, but the problem is consistency: do we show an icon when art is being searched for?  Either way, you get an icon suddenly appearing or disappearing when the art search succeeds or fails.  It's more consistent to show an icon the whole time.

It's kind of annoying to have the tooltip pop up immediately when the mouse pointer enters.  Normal gtk+ tooltips wait half a second.

I think it would look nicer to not have an icon unless/until it finds album art.  There's no reason to take up the extra space with a placeholder.  Also, there probably won't be anything worthwhile to show for podcasts, and wasting the space on a placeholder icon would again be a bit annoying...

(In reply to comment #64)
> I think it would look nicer to not have an icon unless/until it finds album
> art.  There's no reason to take up the extra space with a placeholder.  Also,
> there probably won't be anything worthwhile to show for podcasts, 

Actually, that's not true for podcasts anymore, see bug #380746.

Created attachment 80473 [details] [review]
tooltip-half-second-delay.patch

(In reply to comment #63)
> It's kind of annoying to have the tooltip pop up immediately when the mouse
> pointer enters.  Normal gtk+ tooltips wait half a second.

Fixed.

(In reply to comment #65)
> (In reply to comment #64)
> > I think it would look nicer to not have an icon unless/until it finds album
> > art.  There's no reason to take up the extra space with a placeholder.  Also,
> > there probably won't be anything worthwhile to show for podcasts, 
> 
> Actually, that's not true for podcasts anymore, see bug #380746.
> 

There's also the case of internet radio where there is album art.

(In reply to comment #67)

> There's also the case of internet radio where there is album art.

Lookup of album art on Amazon for Internet radio is extremely unreliable because:

1) You almost never get the album name
2) The artist and title are munged together in a single string "Title - Artist" and don't necessarily follow any convention

Unless there is a way of getting a URL to the art from the stream itself, the cover art plugin should never attempt to do an Amazon lookup.

Tooltip delay patch committed to SVN.  Thanks, Ed.

According to:

svn diff -r 4745:4746 plugins/artdisplay/artdisplay/__init__.py

patch #80068 got committed in svn version 4746, so marking patch as being committed.  Not sure if this was intentional or not, but I was going to propose committing it anyway, as it looked fine and was working for me.

Looks like patch overlapped with patch #80695, so all clear.

Comment #61 implies that art appears in notification bubble when libnotify 0.4.3 is installed.  Can somebody confirm this?  (It certainly does *not* work with libnotify 0.4.2).  If so, then the bug can be closed because all is working (with the caveat that libnotify >= 0.4.3 needs to be installed to see cover art in the notification bubbles).

One last request, could you guys reconsider adding a nice placeholder icon instead of the low res 'missing icon' icon that appears in the tooltip when there is no art/album-art available?
Thank you.

(In reply to comment #73)
> One last request, could you guys reconsider adding a nice placeholder icon
> instead of the low res 'missing icon' icon that appears in the tooltip when
> there is no art/album-art available?
> Thank you.

Hussam can you confirm that with libnotify 0.4.3 the notification bubbles show the album art?

Yes, I can confirm that.
Here is a screenshot for proof.
http://img251.imageshack.us/img251/705/screenshot7zc.png

Based on comment #75, closing bug again.  Since this bug is getting rather long, open up another bug for other feature requests like a better placeholder.

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