GNOME Bugzilla – Bug 782166
Allow including an icon in PopupMenuBase's addAction() and addSettingsAction()
Last modified: 2017-05-09 17:35:07 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).
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.
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().
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.
(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 :)
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
Review of attachment 351070 [details] [review]: Nit: prefix the subject with "popupMenu:", otherwise LGTM
It's a bit too late today already, but I'll address your comments on Monday. Thanks for the review!
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
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).
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.
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.
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.
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).
Review of attachment 351336 [details] [review]: Poor commit message, but let's let it slip :-)
Review of attachment 351338 [details] [review]: Yup
Review of attachment 351343 [details] [review]: OK
(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