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 688820 - GIcon is a bad interface
GIcon is a bad interface
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2012-11-21 16:56 UTC by Allison Karlitskaya (desrt)
Modified: 2013-05-01 20:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GIcon: add g_icon_serialize(), _deserialize() (26.97 KB, patch)
2013-02-26 21:00 UTC, Allison Karlitskaya (desrt)
none Details | Review
Support g_icon_serialize() on GdkPixbuf (10.02 KB, patch)
2013-02-26 21:03 UTC, Allison Karlitskaya (desrt)
none Details | Review
Fix GIcon implementation (13.04 KB, patch)
2013-04-10 06:57 UTC, Allison Karlitskaya (desrt)
none Details | Review
GtkModelMenuItem: add support for icons (7.55 KB, patch)
2013-04-10 06:57 UTC, Allison Karlitskaya (desrt)
none Details | Review
GIcon: pure re-factor of _from_string() (3.27 KB, patch)
2013-04-20 23:01 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Introduce GBytesIcon (12.29 KB, patch)
2013-04-20 23:01 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GIcon: add g_icon_[de]serialize() (19.74 KB, patch)
2013-04-20 23:01 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GMenu: add g_menu_item_set_icon() convenience (2.17 KB, patch)
2013-04-20 23:01 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Fix GIcon implementation (13.09 KB, patch)
2013-04-20 23:26 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GVfsIcon: support icon serialisation (3.22 KB, patch)
2013-04-20 23:36 UTC, Allison Karlitskaya (desrt)
committed Details | Review
GtkModelMenuItem: add support for 'icon' attribute (3.59 KB, patch)
2013-04-22 19:55 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2012-11-21 16:56:47 UTC
http://git.gnome.org/browse/gtk+/tree/gtk/gtkicontheme.c#n3900 tells a pretty sad story.

if (G_IS_LOADABLE_ICON (icon))
 ...
else if (G_IS_THEMED_ICON (icon))
 ...
else if (G_IS_EMBLEMED_ICON (icon))
 ...
else if (GDK_IS_PIXBUF (icon))
 ...
else
 return NULL;

so basically:

 1) I can't implement my own GIcon and expect it to work with Gtk

 2) I can't write code that accepts the same (finite) range of GIcon
    that Gtk accepts without linking against gdkpixbuf and directly using
    its API.



What is needed to correct this situation is an interface on GIcon that exchanges icon data in some form.  I consider that there are two cases that we would likely want to support, and all GIcon implementations would be required to implement at least one of them:

 - give me a GBytes with data in it in either .png or .svg form

 - give me a GBytes with data in it in raw format with metadata about the
   size, colourmap, etc.

The theme and file icon types could then just open the icon file and return it as a GBytes.  GdkPixbuf could implement it fairly directly as well.

On the GTK side we could get a GdkPixbuf out of either of these interfaces quite easily (and we could still keep our special case for the case that it is a GdkPixbuf for efficiency reasons).
Comment 1 Jasper St. Pierre (not reading bugmail) 2012-12-30 17:25:44 UTC
To implement this, we either move GFileIcon/GThemedIcon and friends to GTK+, or we add image loading to Gio.

Neither are very appealing.
Comment 2 Allison Karlitskaya (desrt) 2012-12-30 18:12:08 UTC
I think the suggestion I gave about the interface passing data in .png/.svg form would mean that the "image loading" in GIO would really just be file loading, no?

We may need to teach GIO more about the icon theme spec, but that should be all...
Comment 3 Jasper St. Pierre (not reading bugmail) 2012-12-30 18:17:13 UTC
Right now, GFileIcon can be any format that GdkPixbuf can load, and that's used throughout the GNOME stack.

We can restrict this, but we'd break things in the process.
Comment 4 Allison Karlitskaya (desrt) 2012-12-30 18:20:49 UTC
Or we could ignore the problem.

If the interface specifies that .png or .svg data must be passed and we pipe that into gdk-pixbuf and someone happens to give non-.png/.svg data, it will silently work...

Meanwhile, we could try to clean things up a bit.  There is no reason that we should be having non-png/svg icons.
Comment 5 Jasper St. Pierre (not reading bugmail) 2012-12-30 18:23:34 UTC
(In reply to comment #4)
> Meanwhile, we could try to clean things up a bit.  There is no reason that we
> should be having non-png/svg icons.

GFileIcon is used in cases where we want to pass a URI to an image around that isn't necessarily an image.
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-12-30 18:23:46 UTC
er, isn't necessarily an *icon*
Comment 7 Jasper St. Pierre (not reading bugmail) 2012-12-30 19:11:25 UTC
How do you expect emblems to work?
Comment 8 Jasper St. Pierre (not reading bugmail) 2012-12-30 19:12:02 UTC
Also, for GThemedIcon, we'd have to pass in the size of the icon to get .png / .svg data back, to ensure that we load the right file.
Comment 9 Allison Karlitskaya (desrt) 2013-02-13 19:12:15 UTC
After thinking about this for a long time and chatting it over with some people it seems that the correct solution is something like so:

 - the GIcon implementations in GIO are considered to be authoritative in the
   sense that you must support them all (including emblem icons) as a consumer
   of GIcon

 - any GIcon implementation outside of GIO will be required to gain support for
   to_string() such that calling g_icon_new_for_string() on the result does not
   require registration of any types outside of GIO itself.

 - we introduce a new GLoadableIcon implementation, GBytesIcon that is a GBytes
   containing png data.

 - GdkPixbuf implements its to_string() by returning a string that, when passed
   to new_for_string() will cause a GBytesIcon to be created


This means that you can do this:

  - create GdkPixbuf
  - g_icon_to_string()

  (different process)

  - g_icon_new_for_string()
  - gtk_image_set_from_gicon()


it might also make sense to add a fast-path for GBytesIcon to gtk -- it's kinda silly to jump through the hoops of using a GInputStream and async IO if the thing is just backed by a GBytes.  It should still implement the loadable interface, though, because it can and to do so will increase compatibility with existing GIcon consumers (Gtk being the biggest example).
Comment 10 Allison Karlitskaya (desrt) 2013-02-26 21:00:53 UTC
Created attachment 237470 [details] [review]
GIcon: add g_icon_serialize(), _deserialize()

A first attempt, for review.

These differ from _to_string() in the following ways:

 - they deal in GVariant instead of utf8 strings which allows for much
   more efficient transmission of binary data (both in terms of space
   but also in terms of not having to check the utf8 data for validity
   at 10 different points as it travels through D-Bus)

 - deserialize() will only return "built in" types; its result is never
   sensitive to the registration of types that live outside of GLib.

 - a new GBytesIcon type is introduced so that we have something to
   deserialize GdkPixbuf to

 - it is intended that all 'proper' GIcon implementations must implement
   these

A separate gdk-pixbuf patch is coming soon.
Comment 11 Allison Karlitskaya (desrt) 2013-02-26 21:03:33 UTC
Created attachment 237471 [details] [review]
Support g_icon_serialize() on GdkPixbuf

Serialize GdkPixbuf by emitting a GVariant that will result in a
png-encoded GBytesIcon when deserialized.

Test this by round-tripping an image through this process and ensuring
that it is pixel-perfect.
Comment 12 Alexander Larsson 2013-02-28 15:44:46 UTC
I don't think this is quite right.
I like GBytesIcon, but i think GdkPixbuf should instead implement GLoadableIcon so that all files can be either theme icon name *or* loadable as bits.

Also, i think serialize should be an i/o operation that falls back on GLoadableIcon to GBytesIcon if not a simple file (icon name, or uri or emblem)
Comment 13 Allison Karlitskaya (desrt) 2013-04-08 21:01:37 UTC
So, serializing a GVfsIcon will work (via the to_string() route).  Files work by to_string() doing the filename thing.  I don't know of any loadable icon that doesn't implement to_string() in order to get serialization non-blocking....

I would therefore like to avoid going the loadable route if all other methods fail, specifically to avoid the block...

On the topic of GvfsIcons, I'm happy to see them reassembled on the other side as a GvfsIcon with the same mountspec/id.  They'd then be loadable by their ultimate user.

Implementing loadable on GdkPixbuf is something we could do but I consider it independent of this proposal.


So, basically, we would end up with the following rules about what you should do to be a GIcon:


     - be one of the base types (file, emblem, theme)
   OR
     - be loadable (ie: bytes, gvfsicon)

AND

     - implement serialize() to one of the base types (including gvfs)
   OR
     - implement to_string()



This gives a method for end-users like Gtk (check file, emblem, theme, loadable) and also gives us a guarantee that serialize() will always work.

In short: other than adding a loadable implementation to GdkPixbuf, I think what we have here is correct.
Comment 14 Allison Karlitskaya (desrt) 2013-04-08 21:06:15 UTC
btw: consider how perfectly g_bytes_icon_new(g_resources_lookup_data()) works under this regime...
Comment 15 Allison Karlitskaya (desrt) 2013-04-10 06:57:30 UTC
Created attachment 241116 [details] [review]
Fix GIcon implementation

The "new rules" for GIcon say that we must support serialisation via
g_icon_serialize() and loadability via GLoadableIcon, so implement both
of those.

Serialise GdkPixbuf by emitting a GVariant that will result in a
png-encoded GBytesIcon when deserialized.  The GLoadableIcon interface
is similar: we return a stream that will read out as a png.

Test the serialisation by round-tripping an image through this process
and ensuring that it is pixel-perfect.
Comment 16 Allison Karlitskaya (desrt) 2013-04-10 06:57:43 UTC
Created attachment 241117 [details] [review]
GtkModelMenuItem: add support for icons

Add support for rendering a GIcon (deserialised from the 'icon'
attribute in the model) as part of a GtkModelMenuItem by rendering it as
if it were the toggle.

This is necessary because GtkModelMenuItem is a GtkCheckMenuItem
subclass and we would need to be a GtkImageMenuItem subclass to do it
the usual way.

Using GtkIconHelper we can just directly render the icon to the cairo_t
that is passed to the draw_indicator() function and avoid having to
support arbitrary widgets as images.  The sizing and padding is
difficult to get right here, though, so add a new internal private hook
pseudo-vfunc to call ourselves to draw at the right point.  A real
virtual function was avoided on account of not wanting to add new public
API (at the request of Benjamin).

There is also some tricky sizing involved when handling the size request
but it is nowhere near as complicated, so to avoid adding another hook,
just copy the few lines of logic (basically, a style property lookup and
an addition) into GtkModelMenuItem directly.
Comment 17 Alexander Larsson 2013-04-10 07:45:38 UTC
I think this is essentially right. However, atm it doesn't *quite* work for GVfsIcon, because the deserialization in from_string() relies on the type being registered, and if you send a GVfsIcon to a random app it may not have registered GVfsIcon. So, I think we need to g_type_ensure() that in the gvfs gio module.

Hmm, actually, looking at g_io_module_load() in gdaemonvfs.c it seems that it calls _g_vfs_icon_add_loadable_interface() which *does* in fact reference G_VFS_TYPE_ICON. However, it also adds an interface after the type initialization has finished, which should break in the new gobject. But i haven't seen any errors about this, have you?
Comment 18 Lars Karlitski 2013-04-10 18:18:26 UTC
(from desrt)

I saw the same problem about the weird interface addition but it is not an issue.  The interface is added before class init could possibly have been called.

I am happy to add an explicit call to ensure the type is loaded...

Fwiw, after talking to Lars, I want to change how I manage the GTK patch.  mpt did a spec for how icons in menus should work and I think it would be better to implement that: https://wiki.ubuntu.com/MenuLayout
Comment 19 fakey 2013-04-19 14:34:04 UTC
I'm trying out the GIcon patch, but for some reason g_icon_serialize (g_icon_deserialize (v)) != v, where v is an 'ay'-typed GVariant. Is it wrong to assume that they should be equal?
Comment 20 Lars Karlitski 2013-04-20 21:17:10 UTC
Yeah, they should be equal. There's a patch that fixes it on bug 698457.
Comment 21 Allison Karlitskaya (desrt) 2013-04-20 23:01:21 UTC
Created attachment 242042 [details] [review]
GIcon: pure re-factor of _from_string()

Split out the 'simple string format' cases of URIs, file paths and
themed icons to a separate function.

This function will be shared by g_icon_deserialize().
Comment 22 Allison Karlitskaya (desrt) 2013-04-20 23:01:25 UTC
Created attachment 242043 [details] [review]
Introduce GBytesIcon

GBytesIcon is an icon that has a GBytes inside of it where the GBytes
contains some sort of encoded image in a widely-recognised file format.
Ideally this will be a PNG.

It implements GLoadableIcon, so GTK will already understand how to use
it, but we will add another patch there to make things more efficient.
Comment 23 Allison Karlitskaya (desrt) 2013-04-20 23:01:29 UTC
Created attachment 242044 [details] [review]
GIcon: add g_icon_[de]serialize()

Add support for serialising a GIcon to a GVariant and deserialising the
result back to a GIcon.

This solves a number of problems suffered by the existing to_string()
API, primarily these:

 - not forcing the icon to be a utf8 string means that we can
   efficiently encode a PNG (ie: just give the array of bytes)

 - there is no need to ensure that proper types are loaded before using
   the deserialisation interface.  'Foreign' icon types will probably
   emit a serialised format the deserialises to a GBytesIcon.

We additionally clearly document what is required for being a consumer
or implementation of #GIcon.

Further patches will be required to GdkPixbuf and GVfsIcon to bring
their implementations in line with the new rules (essentially: introduce
implementations of the new serialize() API).
Comment 24 Allison Karlitskaya (desrt) 2013-04-20 23:01:34 UTC
Created attachment 242045 [details] [review]
GMenu: add g_menu_item_set_icon() convenience

This function takes a GIcon, serialises it and sets the resulting
GVariant as the "icon" attribute on the menu item.  We will need to add
a patch to Gtk to actually consume this icon.
Comment 25 Jasper St. Pierre (not reading bugmail) 2013-04-20 23:22:30 UTC
Review of attachment 242044 [details] [review]:

::: gio/gemblem.c
@@ +362,3 @@
+    return NULL;
+
+  origin = g_enum_get_value (g_type_class_peek (G_TYPE_EMBLEM_ORIGIN), emblem->origin);

Are you sure this is OK?

::: gio/gicon.c
@@ +493,3 @@
+          GEnumValue *origin_value;
+
+          origin_class = g_type_class_ref (G_TYPE_EMBLEM_ORIGIN);

You never unref the type class?
Comment 26 Allison Karlitskaya (desrt) 2013-04-20 23:22:46 UTC
Review of attachment 242044 [details] [review]:

::: gio/gicon.c
@@ +599,3 @@
+
+      bytes = g_variant_get_data_as_bytes (val);
+      g_bytes_icon_new (bytes);

uhh.  icons = , obviously.
Comment 27 Allison Karlitskaya (desrt) 2013-04-20 23:26:44 UTC
Created attachment 242047 [details] [review]
Fix GIcon implementation

The "new rules" for GIcon say that we must support serialisation via
g_icon_serialize() and loadability via GLoadableIcon, so implement both
of those.

Serialise GdkPixbuf by emitting a GVariant that will result in a
png-encoded GBytesIcon when deserialized.  The GLoadableIcon interface
is similar: we return a stream that will read out as a png.

Test the serialisation by round-tripping an image through this process
and ensuring that it is pixel-perfect.
Comment 28 Allison Karlitskaya (desrt) 2013-04-20 23:36:41 UTC
Created attachment 242048 [details] [review]
GVfsIcon: support icon serialisation

Add support for the new icon serialisation interface to GVfsIcon as well
as implementing the new interface on GVfsClass for deserialising.
Comment 29 Alexander Larsson 2013-04-21 14:37:20 UTC
Review of attachment 242042 [details] [review]:

ack
Comment 30 Alexander Larsson 2013-04-21 18:59:51 UTC
Review of attachment 242043 [details] [review]:

looks good
Comment 31 Alexander Larsson 2013-04-21 19:58:51 UTC
Review of attachment 242044 [details] [review]:

Looks good to me with my and other comments fixed

::: gio/gemblem.c
@@ +363,3 @@
+
+  origin = g_enum_get_value (g_type_class_peek (G_TYPE_EMBLEM_ORIGIN), emblem->origin);
+  result = g_variant_new_parsed ("('emblem', <(%v, {'origin': <%s>})>)", icon_data, origin ? origin->value_nick : "");

"unknown" instead of "" maybe?

::: gio/gicon.c
@@ +493,3 @@
+          GEnumValue *origin_value;
+
+          origin_class = g_type_class_ref (G_TYPE_EMBLEM_ORIGIN);

g_type_peek_class is ok here, as these are in the same .so so will always exist while this runs

@@ +639,3 @@
+
+  if (!iface->serialize)
+    return NULL;

Maybe we should g_warn there? As we really want all GIcon classes to support serialization.
Comment 32 Alexander Larsson 2013-04-21 20:00:19 UTC
Review of attachment 242045 [details] [review]:

ack
Comment 33 Alexander Larsson 2013-04-21 20:06:41 UTC
Review of attachment 242047 [details] [review]:

looks good
Comment 34 Alexander Larsson 2013-04-21 20:10:04 UTC
Review of attachment 242048 [details] [review]:

You need to bump the gio version requirement, otherwise looks good
Comment 35 Allison Karlitskaya (desrt) 2013-04-21 20:22:39 UTC
(In reply to comment #25)
> +  origin = g_enum_get_value (g_type_class_peek (G_TYPE_EMBLEM_ORIGIN),
> emblem->origin);
> 
> Are you sure this is OK?

The peek is OK in this case because it's an instance method on a class that owns a paramspec with this type.  If a GEmblem exists (which it must, since we are serialising it) we know that the enum type already exists.
 
> ::: gio/gicon.c
> +          origin_class = g_type_class_ref (G_TYPE_EMBLEM_ORIGIN);
> 
> You never unref the type class?

Ya... This one is a bug :)

(In reply to comment #31)
> +  origin = g_enum_get_value (g_type_class_peek (G_TYPE_EMBLEM_ORIGIN),
> emblem->origin);
> +  result = g_variant_new_parsed ("('emblem', <(%v, {'origin': <%s>})>)",
> icon_data, origin ? origin->value_nick : "");
> 
> "unknown" instead of "" maybe?

sure.

> ::: gio/gicon.c
> @@ +493,3 @@
> +          GEnumValue *origin_value;
> +
> +          origin_class = g_type_class_ref (G_TYPE_EMBLEM_ORIGIN);
> 
> g_type_peek_class is ok here, as these are in the same .so so will always exist
> while this runs

This case is not true.  When deserialising there is no guarantee that this type has been initialised yet.
 
> @@ +639,3 @@
> Maybe we should g_warn there? As we really want all GIcon classes to support
> serialization.

Good idea.  We warn in the case that they return invalid data, so we should warn here too.
Comment 36 Allison Karlitskaya (desrt) 2013-04-21 20:32:22 UTC
Attachment 242042 [details] pushed as 519e989 - GIcon: pure re-factor of _from_string()
Attachment 242043 [details] pushed as 9cc222c - Introduce GBytesIcon
Attachment 242044 [details] pushed as c16f914 - GIcon: add g_icon_[de]serialize()

Pushed with changes.
Comment 37 Allison Karlitskaya (desrt) 2013-04-21 20:37:14 UTC
Comment on attachment 242048 [details] [review]
GVfsIcon: support icon serialisation

Attachment 242048 [details] pushed as c9bbd01 - GVfsIcon: support icon serialisation

Pushed with GLib dep increased to 2.37.0
Comment 38 Allison Karlitskaya (desrt) 2013-04-21 20:39:39 UTC
Comment on attachment 242047 [details] [review]
Fix GIcon implementation

Attachment 242047 [details] pushed as aa0f928 - Fix GIcon implementation

Pushed and also added 2.37 GLib depend.
Comment 39 Allison Karlitskaya (desrt) 2013-04-22 19:55:09 UTC
Created attachment 242167 [details] [review]
GtkModelMenuItem: add support for 'icon' attribute

Add support for icons on a GMenuModel.
Comment 40 Lars Karlitski 2013-04-22 20:05:42 UTC
Review of attachment 242167 [details] [review]:

Looks good.
Comment 41 Allison Karlitskaya (desrt) 2013-04-22 20:12:26 UTC
Comment on attachment 242167 [details] [review]
GtkModelMenuItem: add support for 'icon' attribute

Attachment 242167 [details] pushed as ca0a189 - GtkModelMenuItem: add support for 'icon' attribute
Comment 42 Allison Karlitskaya (desrt) 2013-04-22 20:13:16 UTC
Attachment 242045 [details] pushed as c1c1b33 - GMenu: add g_menu_item_set_icon() convenience
Comment 43 fakey 2013-04-26 17:52:21 UTC
GBytesIcon seems to g_object_unref () its 'bytes' field in finalize, leading to a segfault. I guess this should be g_bytes_unref () instead?