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 782166 - Allow including an icon in PopupMenuBase's addAction() and addSettingsAction()
Allow including an icon in PopupMenuBase's addAction() and addSettingsAction()
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: 2017-05-04 17:23 UTC by Mario Sánchez Prada
Modified: 2017-05-09 17:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Allow specifying an icon on PopupMenuBase.addAction() (2.65 KB, patch)
2017-05-04 17:26 UTC, Mario Sánchez Prada
none Details | Review
Allow specifying an icon on PopupMenuBase.addSettingsAction() (1.32 KB, patch)
2017-05-04 17:27 UTC, Mario Sánchez Prada
none Details | Review
Change the position of the icon in PopupImageMenuItem (1.25 KB, patch)
2017-05-04 17:27 UTC, Mario Sánchez Prada
none Details | Review
popupMenu: Accept either an icon name or a GIcon on PopupImageMenuItem (1.83 KB, patch)
2017-05-08 10:42 UTC, Mario Sánchez Prada
none Details | Review
popupMenu: Allow specifying an icon on PopupMenuBase.addAction() (1.27 KB, patch)
2017-05-08 10:46 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review
popupMenu: Change the position of the icon in PopupImageMenuItem (1.17 KB, patch)
2017-05-08 10:46 UTC, Mario Sánchez Prada
none Details | Review
popupMenu: Change the position of the icon in PopupImageMenuItem (1.22 KB, patch)
2017-05-08 10:47 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review
popupMenu: Accept either an icon name or a GIcon on PopupImageMenuItem (1.83 KB, patch)
2017-05-08 11:27 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review

Description Mario Sánchez Prada 2017-05-04 17:23:54 UTC
Filing a ticket to properly track down my conversation with Florian on IRC.

In a nutshell, the idea is to extend addAction() so that a GIcon can be optionally passed, in which case an instance of PopupImageMenuItem instead of PopupMenuItem will be created.

Ideally (and this is particularly interesting for Endless), we would add the same optional parameter to addSettingsAction(), so that we can create an action item with an icon reusing the code that looks up an application by its desktop file, without having to rewrite it again.

Last, as mentioned on IRC, it would be nice if we could add the icon before the label instead of after, as that would be more consistent with what we see already in other popup menus (e.g. system indicator's popup menu).
Comment 1 Mario Sánchez Prada 2017-05-04 17:26:52 UTC
Created attachment 351068 [details] [review]
Allow specifying an icon on PopupMenuBase.addAction()

This allows passing an optional icon parameter to addAction()
so that a PopupImageMenuItem instance is created instead of a
PopupMenuItem if an icon is specified, either by passing a
string to the icon name, or a Gio.Icon.
Comment 2 Mario Sánchez Prada 2017-05-04 17:27:03 UTC
Created attachment 351069 [details] [review]
Allow specifying an icon on PopupMenuBase.addSettingsAction()

This patch adds an extra icon parameter to addSettingsAction()
that will be passed to PopupMenyBase.addAction().
Comment 3 Mario Sánchez Prada 2017-05-04 17:27:13 UTC
Created attachment 351070 [details] [review]
Change the position of the icon in PopupImageMenuItem

We are moving the icon to be added before the text instead of after,
which is consistent with other menu items in other popup menus, such
as the ones in the system indicator's popup menu.
Comment 4 Mario Sánchez Prada 2017-05-05 12:32:10 UTC
(In reply to Mario Sánchez Prada from comment #2)
> Created attachment 351069 [details] [review] [review]
> Allow specifying an icon on PopupMenuBase.addSettingsAction()
> 
> This patch adds an extra icon parameter to addSettingsAction()
> that will be passed to PopupMenyBase.addAction().

After some more consideration (and also after learning more about the shell's code), I don't think this patch makes sense either as what I'm doing downstream is abusing this method just to launch the gnome-control-center, which I can easily achieve with addAction() + a custom callback.

The other 2 patches still apply, though :)
Comment 5 Florian Müllner 2017-05-05 14:20:00 UTC
Review of attachment 351068 [details] [review]:

When I said on IRC that I wanted the patch split, I was primarily thinking of the iconName/icon change and addAction() one, not that much the addAction()/addSettingsAction() one (that's now obsolete anyway, yay).

(Marking needs-work because of that, the comments below are just random thoughts)

::: js/ui/popupMenu.js
@@ +404,3 @@
+        // but we now support instances of Gio.Icon too, so check that.
+        if (GObject.type_is_a(icon, Gio.Icon))
+            this.setGIcon(icon);

Mmh, I was thinking of either adding a separate setGIcon() function or doing the above in setIcon(), but this might be the best option (although I don't like the setGIcon() name - IMHO setIconName()/setIcon() would make more sense, but meh, extension API ...)

@@ +408,3 @@
+            this.setIcon(icon);
+        else
+            throw TypeError("Invalid argument to PopupImageMenuItem constructor");

You can use a simple:
 if (...)
    this.setGIcon();
  else
    this.setIcon();

GObject properties are typed and gjs will throw an exception if you try to set one to an incompatible value already, so no need to add your own exception here (if that wasn't the case, the check would be better off in the method itself, otherwise we wouldn't catch nonsense like "item.setIcon(new Clutter.Actor());")

@@ +476,3 @@
 
+    addAction: function(title, callback, icon) {
+        let menuItem = null;

Nit: No need to initialize to null, as it'll be assigned a menu item unconditionally anyway
Comment 6 Florian Müllner 2017-05-05 14:20:44 UTC
Review of attachment 351070 [details] [review]:

Nit: prefix the subject with "popupMenu:", otherwise LGTM
Comment 7 Mario Sánchez Prada 2017-05-06 03:53:10 UTC
It's a bit too late today already, but I'll address your comments on Monday.

Thanks for the review!
Comment 8 Mario Sánchez Prada 2017-05-08 10:32:30 UTC
Review of attachment 351068 [details] [review]:

::: js/ui/popupMenu.js
@@ +404,3 @@
+        // but we now support instances of Gio.Icon too, so check that.
+        if (GObject.type_is_a(icon, Gio.Icon))
+            this.setGIcon(icon);

I thought the same thing, but I didn't want to break the current setIcon() API, and this is the best naming I could came up with, which I agree it's not great.

Another option is to have a single setIcon() function taking either a string or a GIcon, and move this code inside there, would that look better to you?

@@ +408,3 @@
+            this.setIcon(icon);
+        else
+            throw TypeError("Invalid argument to PopupImageMenuItem constructor");

Thanks for pointing that out, I'll remove the throw statement.

@@ +476,3 @@
 
+    addAction: function(title, callback, icon) {
+        let menuItem = null;

Ok
Comment 9 Mario Sánchez Prada 2017-05-08 10:42:41 UTC
Created attachment 351335 [details] [review]
popupMenu: Accept either an icon name or a GIcon on PopupImageMenuItem

Add an extra check to setIcon() so that either a GIcon or an string
with the icon's name is handlded, so that we can create menu items
in different ways (e.g. by passing a GIcon created from a resource).
Comment 10 Mario Sánchez Prada 2017-05-08 10:46:25 UTC
Created attachment 351336 [details] [review]
popupMenu: Allow specifying an icon on PopupMenuBase.addAction()

This allows passing an optional icon parameter to addAction()
so that a PopupImageMenuItem instance is created instead of a
PopupMenuItem if an icon is specified.
Comment 11 Mario Sánchez Prada 2017-05-08 10:46:38 UTC
Created attachment 351337 [details] [review]
popupMenu: Change the position of the icon in PopupImageMenuItem

We are moving the icon to be added before the text instead of after,
which is consistent with other menu items in other popup menus, such
as the ones in the system indicator's popup menu.
Comment 12 Mario Sánchez Prada 2017-05-08 10:47:35 UTC
Created attachment 351338 [details] [review]
popupMenu: Change the position of the icon in PopupImageMenuItem

We are moving the icon to be added before the text instead of after,
which is consistent with other menu items in other popup menus, such
as the ones in the system indicator's popup menu.
Comment 13 Mario Sánchez Prada 2017-05-08 11:27:59 UTC
Created attachment 351343 [details] [review]
popupMenu: Accept either an icon name or a GIcon on PopupImageMenuItem

Add an extra check to setIcon() so that either a GIcon or an string
with the icon's name is handlded, so that we can create menu items
in different ways (e.g. by passing a GIcon created from a resource).
Comment 14 Florian Müllner 2017-05-09 16:22:23 UTC
Review of attachment 351336 [details] [review]:

Poor commit message, but let's let it slip :-)
Comment 15 Florian Müllner 2017-05-09 16:22:26 UTC
Review of attachment 351338 [details] [review]:

Yup
Comment 16 Florian Müllner 2017-05-09 16:22:30 UTC
Review of attachment 351343 [details] [review]:

OK
Comment 17 Mario Sánchez Prada 2017-05-09 17:35:07 UTC
(In reply to Florian Müllner from comment #14)
> Review of attachment 351336 [details] [review] [review]:
> 
> Poor commit message, but let's let it slip :-)

Sorry, will try to get a better one next time.

Thanks for the reviews, all pushed:
https://git.gnome.org/browse/gnome-shell/commit/?id=28ca96064bcb95fb3b3461f3eb9959c2aef7eee6
https://git.gnome.org/browse/gnome-shell/commit/?id=e38c26894be3719fd502199b770985d6182e28a0
https://git.gnome.org/browse/gnome-shell/commit/?id=73680e2433f4b42bd9d38d27d74f3e4a91a13537