GNOME Bugzilla – Bug 623200
Add previous and play actions to notifications, and use id's that correspond to named icons
Last modified: 2010-10-01 12:37:10 UTC
This patch lets rhythmbox take advantage of the features we've been adding to our implementation of the notification daemon in gnome shell. If the id's of the actions passed match the names of icons, then g-s will use the icon. Also, allow the full size cover art to be sent through so we can have a nice big image on the other end.
Created attachment 164961 [details] [review] Add previous and play actions to notifications, and use id's that correspond to named icons In conjunction with https://bugzilla.gnome.org/show_bug.cgi?id=621014, this makes RB's notifications in gnome-shell a lot prettier and a bit more functional.
Review of attachment 164961 [details] [review]: ::: plugins/status-icon/rb-status-icon-plugin.c @@ +384,3 @@ GList *caps; + if (notify_init ("Rhythmbox") == FALSE) { What's the reasoning behind this change? @@ +450,3 @@ if (show_action && plugin->priv->notify_supports_actions) { notify_notification_add_action (plugin->priv->notification, + "media-skip-backward", Shouldn't you be using the symbolic icons instead of the normal ones? @@ +885,3 @@ + /* Keep the full size thing for notifications. */ + plugin->priv->notify_pixbuf = pixbuf; This could be exceedingly large, no? Also, I'm pretty sure there's a refcounting bug here, and that we'd rather pass a filename for the file rather than a pixbuf if possible.
Good point. We could switch to using the rb::coverArt-uri extra metadata property (instead of rb::coverArt) to get album art URIs rather than pixbufs. Unfortunately it'd be tricky to fall back to the current method if the URI wasn't available, but that shouldn't happen in most cases anyway. I'm not sure it's a good idea to expose these new actions for the regular notification-daemon. Maybe check for the 'x-gnome-icon-buttons' server capability before adding them?
(In reply to comment #2) > Review of attachment 164961 [details] [review]: > > ::: plugins/status-icon/rb-status-icon-plugin.c > @@ +384,3 @@ > GList *caps; > > + if (notify_init ("Rhythmbox") == FALSE) { > > What's the reasoning behind this change? As per the notification spec, that name should be a proper name. We've run into a number of programs that don't follow that in looking at our notification implementation, but, I figured I'd fix it while I was at it. Sorry I forgot to mention it. > @@ +450,3 @@ > if (show_action && plugin->priv->notify_supports_actions) { > notify_notification_add_action (plugin->priv->notification, > + "media-skip-backward", > > Shouldn't you be using the symbolic icons instead of the normal ones? I don't think so. Nobody has told me that plans for the message tray involve that. > @@ +885,3 @@ > > + /* Keep the full size thing for notifications. */ > + plugin->priv->notify_pixbuf = pixbuf; > > This could be exceedingly large, no? > > Also, I'm pretty sure there's a refcounting bug here, and that we'd rather pass > a filename for the file rather than a pixbuf if possible. I think in practice that won't be more than around 200x200, but I can re-write the patch to use the uri method in a bit. (In reply to comment #3) > Good point. We could switch to using the rb::coverArt-uri extra metadata > property (instead of rb::coverArt) to get album art URIs rather than pixbufs. > Unfortunately it'd be tricky to fall back to the current method if the URI > wasn't available, but that shouldn't happen in most cases anyway. > > I'm not sure it's a good idea to expose these new actions for the regular > notification-daemon. Maybe check for the 'x-gnome-icon-buttons' server > capability before adding them? I'll try and add that shortly, as well. Thanks for the feedback!
Created attachment 165001 [details] [review] Add previous and play actions to notifications, and use id's that correspond to named icons Sends cover art uri through the image_path hint instead of sending the pixbuf. Seems to be working both in g-s and notify-osd. We don't want to use symbolic icons, it's easy enough to fetch the symbolic icon on the notification daemon's end, and to quote owen "We know more about how things will be displayed than rhythmbox."
Review of attachment 165001 [details] [review]: Mostly looks OK now. Looks like the indentation isn't consistent with the rest of the file in a few places though. ::: plugins/status-icon/rb-status-icon-plugin.c @@ +874,3 @@ + + if (plugin->priv->notify_art_uri != NULL) + g_free (plugin->priv->notify_art_uri); no need to check if it's not NULL before freeing it, g_free(NULL) just does nothing. @@ +876,3 @@ + g_free (plugin->priv->notify_art_uri); + + uri = g_uri_unescape_string (g_value_dup_string (metadata), NULL); we leak the string returned by g_value_dup_string here, should use g_value_get_string instead. @@ +881,3 @@ + plugin->priv->notify_art_uri = uri; + else + plugin->priv->notify_art_uri = NULL; plugin->priv->notify_art_uri = uri; .. no need for the if statement at all. @@ +884,3 @@ + } + + notify_playing_entry (plugin, FALSE); should move the logic from db_art_metadata_cb here. since db_art_metadata_cb doesn't provide anything we use for notifications any more, it shouldn't trigger a notification.
Created attachment 165194 [details] [review] Add previous and play actions to notifications, and use id's that correspond to named icons Alright, I think I fixed everything that was wrong (I even went on a hunt for mis-indented code). The one thing I'd like to note is that this does significantly decrease the amount of art that gets displayed in notifications for me. That will be different for everyone of course, but maybe someone who knows better than I could suggest a better solution. The one other question is, this patch displays the RB icon in notifications whenever no cover art is found, which I think is a change from before where no art was displayed. That would be pretty easy to fix, if you deem it necessary.
Review of attachment 165194 [details] [review]: I don't think there's much point committing this until the image_path hint works with the gnome-shell notification daemon. I'm probably going to have to make the album art plugin emit rb::coverArt-uri notifications more consistently too.. ::: plugins/status-icon/rb-status-icon-plugin.c @@ +376,3 @@ const char *primary, const char *secondary, + gchar *image_uri, this should be const, which would make the cast below unnecessary @@ +444,3 @@ + if (image_uri != NULL) { + notify_notification_clear_hints (plugin->priv->notification); + notify_notification_set_hint_string (plugin->priv->notification, we use 8 space tabs, not 2 @@ +739,3 @@ plugin->priv->current_title = NULL; plugin->priv->current_album_and_artist = NULL; + plugin->priv->notify_art_uri = NULL; should add a call to rhythmdb_entry_request_extra_metadata to fetch the album art URI here @@ +877,3 @@ + g_free (plugin->priv->notify_art_uri); + + uri = g_uri_unescape_string (g_value_dup_string (metadata), NULL); this still leaks. the g_uri_unescape_string call shouldn't be there anyway - see bug 623508.
fixed up and pushed as commits 64ba2fc, 2e0204b, and c32cdd6.