Created attachment 163099 [details] [review]
Changes rhythmbox's notifications so you can see the effect

This should apply to rhythmbox HEAD and make it so it passes forward as well as backward actions and that they are named after stock icons.

Review of attachment 163096 [details] [review]:

I think this will be a nice addition, and despite of the large comments below it shouldn't require too much work. I'm pretty sure Marina already pointed out this[1] link, now would be a good time to read it again *hint,hint*

[1] http://lists.cairographics.org/archives/cairo/2008-September/015092.html

::: js/ui/messageTray.js
@@ +22,2 @@
 const ICON_SIZE = 24;
+const NOTIFICATION_BUTTON_SIZE = 36;

Something like BUTTON_ICON_SIZE makes more sense to me.

@@ +270,2 @@
     },
+    addIconButton: function(id, label) {

It is a bad idea to duplicate addButton() for a mere difference in alignment. Obviously because code duplication is bad, but more importantly it gives the layout a touch of randomness. If the first action is added with addIconButton(), all actions will be centered, if it is added with addButton(), everything will be right aligned (don't forget that notifications are not only used from notificationDaemon).

If you absolutely insist on separate functions, you should factor out the common code to a private function, but in my opinion the nicer approach is to just modify addButton().

The only problem there is the different alignment - as it is possible to modify the alignment later, the real problem is to figure out the right rules. This is a design problem, so it's probably best to ping mccann on IRC - some possibilities I see would be to

 - start right-aligned, change to centered if more
   than one action is added
 - center if at least one action is displayed as icon
 - center if all actions are displayed as icons
 - align right if at least one action has a label

@@ +283,3 @@
+        if (!this._buttonBox) {
+        // Note: id seems to be the icon's name in rhythmbox's case
+    addIconButton: function(id, label) {

Any particular reason for using St.Clickable instead of St.Button? If not, the latter is preferable.

@@ +284,3 @@
+        if (!this._buttonBox) {
+        // Note: id seems to be the icon's name in rhythmbox's case
+    addIconButton: function(id, label) {

TextureCache will always return a texture - if the icon is not found it will just be empty.

It looks cleaner to me if you first create the button unconditionally and then set the appropriate content, e.g.

let button = new St.Button({ style_class: ... });
if (Gtk.IconTheme.get_default().has_icon(id)) {
    button.child = St.TextureCache...
} else {
    button.label = label;
}
button.connect(...);

Review of attachment 163099 [details] [review]:

Not intended for inclusion, so marking reviewed to get it off the list ;)

Created attachment 163435 [details] [review]
Add icon buttons to notifications

Now if an id is passed to the AddButton that matches the name of a Gtk stock icon, it will be displayed as a stock icon.  This is another in the 'implement that music player mockup' series.

Hopefully I addressed everything in that patch, which is now tiny.  I decided that it didn't matter so much where the buttons were aligned.  If that turns out to be important, it will mean a bit more rewriting.

Review of attachment 163435 [details] [review]:

(In reply to comment #5)
> I decided that it didn't matter so much where the buttons were aligned.  If that turns
> out to be important, it will mean a bit more rewriting.

Jein :)
I think alignment does matter, and you should get designers' input on it. But yeah, it doesn't block the functionality itself, and there's no problem adding it (if desired) at a later stage.

Code looks good - if I remember those mockups correctly though, the buttons don't have any background, so you might want to use a different style for icon buttons (you can add additional style classes with button.add_style_class_name())

Some nitpicks about the commit message:
 - instead of AddButton you should use Notification.addButton() (per
   convention, AddButton refers to a class name - also addButton is
   a very generic function name, so prefixing with the class is a good
   idea)
 - it's themed icon (GTK stock icons are implemented as themed icons,
   but you are not limited to that subset)
 - 'that music player mockup' is a little obscure for people not following
   development closely :)

Created attachment 164444 [details] [review]
Add icon buttons to notifications

Now if an id is passed to Notification.addButton that matches the name of a Gtk icon, it will be displayed as an icon.  This is another in the implement these mockups (http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/MusicPlayer) series.

Created attachment 164445 [details] [review]
Add icon buttons to notifications

Now if an id is passed to Notification.addButton that matches the name of a Gtk icon, it will be displayed as an icon.  This is another in the implement these mockups (http://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/MusicPlayer) series.

Created attachment 164450 [details] [review]
Add icon buttons to notifications

Rebased to today's git HEAD and with the stylesheet changes

Created attachment 164452 [details] [review]
Add icon buttons to notifications

Put constant definition somewhere a little more logical, satisfy a nitpicker.

Created attachment 164453 [details] [review]
Add icon buttons to notifications

Clarify commit message, we accept everyone's icons, not just gtk programs.

Comment on attachment 164453 [details] [review]
Add icon buttons to notifications

Looks good, thanks!

Comment on attachment 164453 [details] [review]
Add icon buttons to notifications

actually, one more small thing: please add 'x-gnome-icon-buttons' (or something similar starting with 'x-gnome-') to the return value of NotificationDaemon.GetCapabilities, so that we're indicating to the clients that we have this capability.

(ok, to commit with that change)

Created attachment 164690 [details] [review]
Add icon buttons to notifications

Advertise the x-gnome-icon-buttons capability through NotificationDaemon.GetCapabilities()

Attachment 164690 [details] pushed as d1e1afd - Add icon buttons to notifications

This is a comment about the patch for Rhythmbox. I checked out Rhythmbox source code and updated it with your patch. The buttons look really nice in the notifications!

The order of actions should be "Previous", "Pause", "Next", not "Next", "Previous" as you have it now. Also the ids for actions ("media-skip-backward" and "media-skip-forward") are switched now.

I asked Jon again, and we both think "Pause" is a useful action. For example, when you realize you are sick of listening to the music or if you muted the volume and noticed later that the notifications are still coming up. The notification should just slide away if you hit "Pause" (not switch the action button to "Play"). "Play"/"Pause", "Previous", and "Next" actions will always be available from a summary notification for the Rhythmbox.

Other than that, I think we should be ready to submit it to Rhythmbox soon. Perhaps CC me, Owen, and Jon on the Rhythmbox bug once you create it.

We'll also need to create a patch for Rhythmbox to send a larger size album art image in the notification. As you mentioned, it comes out blurred now.