After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 621014 - Allow notification action buttons to be icons
Allow notification action buttons to be icons
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-06-08 20:02 UTC by Matt N
Modified: 2010-06-29 22:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Enables icon buttons in notifications (2.49 KB, patch)
2010-06-08 20:02 UTC, Matt N
needs-work Details | Review
Changes rhythmbox's notifications so you can see the effect (1.88 KB, patch)
2010-06-08 20:18 UTC, Matt N
reviewed Details | Review
Add icon buttons to notifications (1.46 KB, patch)
2010-06-11 21:21 UTC, Matt N
reviewed Details | Review
Add icon buttons to notifications (1.64 KB, patch)
2010-06-23 22:16 UTC, Matt N
none Details | Review
Add icon buttons to notifications (2.29 KB, patch)
2010-06-23 22:31 UTC, Matt N
none Details | Review
Add icon buttons to notifications (2.30 KB, patch)
2010-06-23 22:48 UTC, Matt N
none Details | Review
Add icon buttons to notifications (2.30 KB, patch)
2010-06-23 22:55 UTC, Matt N
none Details | Review
Add icon buttons to notifications (2.30 KB, patch)
2010-06-23 23:13 UTC, Matt N
reviewed Details | Review
Add icon buttons to notifications (2.78 KB, patch)
2010-06-26 18:09 UTC, Matt N
committed Details | Review

Description Matt N 2010-06-08 20:02:45 UTC
Created attachment 163096 [details] [review]
Enables icon buttons in notifications

Runs the key of the actions passed with a notification by libnotify.  If the key matches a stock icon, use that icon as a button.  Silly things will probably happen if not all of the passed keys match icon names, since the code falls back on text buttons.
Comment 1 Matt N 2010-06-08 20:18:18 UTC
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.
Comment 2 Florian Müllner 2010-06-09 01:06:52 UTC
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(...);
Comment 3 Florian Müllner 2010-06-09 01:08:22 UTC
Review of attachment 163099 [details] [review]:

Not intended for inclusion, so marking reviewed to get it off the list ;)
Comment 4 Matt N 2010-06-11 21:21:52 UTC
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.
Comment 5 Matt N 2010-06-11 21:23:41 UTC
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.
Comment 6 Florian Müllner 2010-06-11 22:45:11 UTC
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 :)
Comment 7 Matt N 2010-06-23 22:16:19 UTC
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.
Comment 8 Matt N 2010-06-23 22:31:07 UTC
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.
Comment 9 Matt N 2010-06-23 22:48:34 UTC
Created attachment 164450 [details] [review]
Add icon buttons to notifications

Rebased to today's git HEAD and with the stylesheet changes
Comment 10 Matt N 2010-06-23 22:55:31 UTC
Created attachment 164452 [details] [review]
Add icon buttons to notifications

Put constant definition somewhere a little more logical, satisfy a nitpicker.
Comment 11 Matt N 2010-06-23 23:13:32 UTC
Created attachment 164453 [details] [review]
Add icon buttons to notifications

Clarify commit message, we accept everyone's icons, not just gtk programs.
Comment 12 Florian Müllner 2010-06-23 23:16:30 UTC
Comment on attachment 164453 [details] [review]
Add icon buttons to notifications

Looks good, thanks!
Comment 13 Dan Winship 2010-06-24 13:41:04 UTC
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)
Comment 14 Matt N 2010-06-26 18:09:32 UTC
Created attachment 164690 [details] [review]
Add icon buttons to notifications

Advertise the x-gnome-icon-buttons capability through NotificationDaemon.GetCapabilities()
Comment 15 Florian Müllner 2010-06-26 18:26:41 UTC
Attachment 164690 [details] pushed as d1e1afd - Add icon buttons to notifications
Comment 16 Marina Zhurakhinskaya 2010-06-29 22:59:29 UTC
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.