To implement this, we either move GFileIcon/GThemedIcon and friends to GTK+, or we add image loading to Gio.

Neither are very appealing.

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...

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.

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.

(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.

er, isn't necessarily an *icon*

How do you expect emblems to work?

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.

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).

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.

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.

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)

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.

btw: consider how perfectly g_bytes_icon_new(g_resources_lookup_data()) works under this regime...

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.

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.

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?

(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

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?

Yeah, they should be equal. There's a patch that fixes it on bug 698457.

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().

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.

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).

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.

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?

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.

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.

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.

Review of attachment 242042 [details] [review]:

ack

Review of attachment 242043 [details] [review]:

looks good

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.

Review of attachment 242045 [details] [review]:

ack

Review of attachment 242047 [details] [review]:

looks good

Review of attachment 242048 [details] [review]:

You need to bump the gio version requirement, otherwise looks good

(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.

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 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 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.

Created attachment 242167 [details] [review]
GtkModelMenuItem: add support for 'icon' attribute

Add support for icons on a GMenuModel.

Review of attachment 242167 [details] [review]:

Looks good.

Comment on attachment 242167 [details] [review]
GtkModelMenuItem: add support for 'icon' attribute

Attachment 242167 [details] pushed as ca0a189 - GtkModelMenuItem: add support for 'icon' attribute

Attachment 242045 [details] pushed as c1c1b33 - GMenu: add g_menu_item_set_icon() convenience

GBytesIcon seems to g_object_unref () its 'bytes' field in finalize, leading to a segfault. I guess this should be g_bytes_unref () instead?

unref fixed in: https://git.gnome.org/browse/glib/commit/?id=706e636ab83de76c22ed7f2e56df9a1bcb49b39a