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 789018 - PopupImageMenuItem setIcon() method does not accept icon as string
PopupImageMenuItem setIcon() method does not accept icon as string
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.26.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2017-10-15 16:18 UTC by Jay Strict
Modified: 2017-10-16 18:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
popupMenu: Fix setting ImageMenuItem's icons via strings (1.18 KB, patch)
2017-10-15 22:29 UTC, Florian Müllner
committed Details | Review

Description Jay Strict 2017-10-15 16:18:41 UTC
In js/ui/popupMenu.js function PopupImageMenuItem.setIcon(icon) there is a comment line:

  // The 'icon' parameter can be either a Gio.Icon or a string.

But this seems not to be true. If you give a string as argument to the constructor, like, e.g.,

  new PopupMenu.PopupImageMenuItem("Text", "emblem-system-symbolic");

in a gnome-shell extension, then this fails with

  Error: Expected type GType for Argument 'type' but got type 'string'


Furthermore this new behavior breaks old working extensions. See
https://github.com/jaystrictor/gnome-shell-extension-syncthing/issues/30
Comment 1 Florian Müllner 2017-10-15 22:29:02 UTC
Created attachment 361645 [details] [review]
popupMenu: Fix setting ImageMenuItem's icons via strings

Commit 28ca96064bcb added support for setting PopupImageMenuItem's icons
via GIcons as well as via strings. However as the check whether an object
implements the GIcon interface only works on GObjects, specifying an icon
name was broken. Fix that to actually allow both strings and GIcons.


(In reply to Jay Strict from comment #0)
> Furthermore this new behavior breaks old working extensions.

This change was unintentional, so we should fix it. But: The deal extensions get is that they gain access to all internals, in exchange for no API guarantees. So while extensions usually keep working between one stable version and the next, you cannot assume that an update to a newer version will never require any extension code changes - in particular, something working for an "old working extension" does *not* mean that it must never change in future versions.
Comment 2 Rui Matos 2017-10-16 16:50:44 UTC
Review of attachment 361645 [details] [review]:

looks fine

::: js/ui/popupMenu.js
@@ +409,2 @@
             this._icon.gicon = icon;
         else

this allows any other types, including GObjects, to end up in icon_name but, meh
Comment 3 Florian Müllner 2017-10-16 18:53:26 UTC
Attachment 361645 [details] pushed as c12da66 - popupMenu: Fix setting ImageMenuItem's icons via strings

(In reply to Rui Matos from comment #2)
> this allows any other types, including GObjects, to end up in icon_name but,
> meh

They don't really end up in icon_name, as trying to set the property to anything not a string will throw an exception.