Merge lp:~attente/libindicator/gicon-serialization into lp:libindicator/13.10

[William Hua]

Use GIcon's serialization/deserialization interface for indicator icons so that we can load icons as PNG data transmitted over the bus.

Use GIcon's serialization/deserialization interface for indicator icons so that we can load icons as PNG data transmitted over the bus.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:487
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~attente/libindicator/gicon-serialization/+merge/160148/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/libindicator-ci/9/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-amd64-ci/9/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-armhf-ci/9/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/9/rebuild

review: Needs Fixing (continuous-integration)

[Lars Karlitski (larsu)]

Thanks for the patch!

* We need to wait for the new glib before this can land (that's what Jenkins is complaining about).

* Why did you add a changelog entry? Daily releases are created automatically.

* The error checking logic in indicator-image-helper.c is weird. Don't check whether the error is set, but instead check the return value of the function (by convention, @error will be set when functions return NULL or FALSE). Also, use "else" :P

* In indicator_ng_update_entry, you removed the const from 'label' and 'accessible_desc' and freed those strings at the end of the function. That's wrong. They are filled with pointers to the serialized GVariant-data (that's what the '&' in the format string means).

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:489
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~attente/libindicator/gicon-serialization/+merge/160148/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/libindicator-ci/10/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-amd64-ci/10/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-armhf-ci/10/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/10/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:490
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~attente/libindicator/gicon-serialization/+merge/160148/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/libindicator-ci/11/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-amd64-ci/11/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-armhf-ci/11/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/11/rebuild

review: Needs Fixing (continuous-integration)

[lp:~attente/libindicator/gicon-serialization]

487. By Mathieu Trudel-Lapierre on 2013-05-02

Merge changelog changes from /13.04 branch for saucy.

Approved by PS Jenkins bot, Łukasz Zemczak.

488. By PS Jenkins bot on 2013-05-03

Releasing 12.10.2daily13.05.02-0ubuntu1 to ubuntu.

Approved by PS Jenkins bot.

[Ted Gould (ted)]

I'm a little confused on why we want this. It seems like sending PNGs over the bus is a bad idea because if we do that it'll be impossible to match the theme/resolution on the other side of the bus. I think for this to land we also need a way to communicate that information to the indicator service. This patch should really include that information mechanism as well.

review: Needs Information

[Lars Karlitski (larsu)]

This is for images that are not in the theme (for example user avatars or favicons).

This patch basically replaces g_icon_from_string() with g_icon_deserialize(), a new function in Gio which is a bit saner: it doesn't require GdkPixbuf on the other side of the bus. This will be handy for Unity 8, because Qt already has png decoding functionality, it should really not have to link against gdkpixbuf.

[Ted Gould (ted)]

On Sat, 2013-05-18 at 02:58 +0000, Lars Uebernickel wrote:

> This is for images that are not in the theme (for example user avatars
> or favicons).
>
> This patch basically replaces g_icon_from_string() with
> g_icon_deserialize(), a new function in Gio which is a bit saner: it
> doesn't require GdkPixbuf on the other side of the bus. This will be
> handy for Unity 8, because Qt already has png decoding functionality,
> it should really not have to link against gdkpixbuf.

But we don't allow icons that aren't in the theme on the panel. So I
don't see why we need it at all in libindicator. Sure it might be
needed for messaging menu and other things that use avatars, but
libindicator doesn't need that feature.

[William Hua (attente)]

The new keyboard indicator will probably need this support (see: https://wiki.ubuntu.com/LanguageAndText#Icon). But yes, you're correct that the indicator service will need to infer the theming information from the panel service to render the icons properly. I'm not sure if there's a better way than exposing that information through a DBus method though.

[Lars Karlitski (larsu)]

> But we don't allow icons that aren't in the theme on the panel. So I
> don't see why we need it at all in libindicator. Sure it might be
> needed for messaging menu and other things that use avatars, but
> libindicator doesn't need that feature.

Yes it does, IndicatorNg turns the indicator menu models into GtkMenus, and needs to convert icons into GtkImages in the process.

We probaby don't need it for indicator-image-helper (except maybe for the case Will is describing), but I think we should keep it for completeness sake. It would be confusing if we allowed custom icons in the menus but not in the panel itself.

[Ted Gould (ted)]

On Sat, 2013-05-18 at 05:24 +0000, William Hua wrote:

> The new keyboard indicator will probably need this support (see:
> https:��wiki.ubuntu.com�LanguageAndText#Icon). But yes, you're correct
> that the indicator service will need to infer the theming information
> from the panel service to render the icons properly. I'm not sure if
> there's a better way than exposing that information through a DBus
> method though.

Hmm, I don't see where it needs that. It has instructions for the
visual designers on the data in the icons, but I still don't see why
they shouldn't be in the theme. For instance, the background color
would be different depending on the theme. Computer generated icons are
never a good experience.

I think that what would make sense, as a temporary solution, is to have
indicator-keyboard generate fallback icons in the hicolor theme as part
of it's build. That way it can know they exist, but then allow a proper
theme to be generated as well.

[Ted Gould (ted)]

On Sat, 2013-05-18 at 13:00 +0000, Lars Uebernickel wrote:

> > But we don't allow icons that aren't in the theme on the panel. So I
> > don't see why we need it at all in libindicator. Sure it might be
> > needed for messaging menu and other things that use avatars, but
> > libindicator doesn't need that feature.
>
>
> Yes it does, IndicatorNg turns the indicator menu models into
> GtkMenus, and needs to convert icons into GtkImages in the process.

I'm not sure what you're saying here. Sure, they need to be bitmaps to
be on the screen at some point. But we still don't want them to be not
from the theme. It's not that it's not technically possible, it's a
non-feature and something that creates a worse user experience.

> We probaby don't need it for indicator-image-helper (except maybe for
> the case Will is describing), but I think we should keep it for
> completeness sake. It would be confusing if we allowed custom icons in
> the menus but not in the panel itself.

Hmm, not sure why it's confusing. :-) And frankly, it's what we
already do. This branch changes that. Let's not change it. The rule
in my mind is simply: icons, always theme names; user data, we take it
as high of resolution as we can get it and do as best we can. Things
like avatars are user data. User data never appears on the panel.

[Lars Karlitski (larsu)]

> > Yes it does, IndicatorNg turns the indicator menu models into
> > GtkMenus, and needs to convert icons into GtkImages in the process.
>
> I'm not sure what you're saying here. Sure, they need to be bitmaps to
> be on the screen at some point. But we still don't want them to be not
> from the theme. It's not that it's not technically possible, it's a
> non-feature and something that creates a worse user experience.

What I'm saying is that libindicator is responsible to turn any icon in any of the menus into a GtkImage now (through IndicatorNg). Themed icons will stay themed icons. File icons will stay file icons. The only change is that for bitmapped icons, we send a well-known format instead of something that relies on GdkPixbuf.

Bitmapped icons in the panel are a different topic, totally unrelated to this patch. Services can insert them now if they want to, but none of them does.

> > We probaby don't need it for indicator-image-helper (except maybe for
> > the case Will is describing), but I think we should keep it for
> > completeness sake. It would be confusing if we allowed custom icons in
> > the menus but not in the panel itself.
>
> Hmm, not sure why it's confusing. :-) And frankly, it's what we
> already do. This branch changes that. Let's not change it. The rule
> in my mind is simply: icons, always theme names; user data, we take it
> as high of resolution as we can get it and do as best we can. Things
> like avatars are user data. User data never appears on the panel.

Oh, now I understand where this misunderstanding is coming from. IndiciatorNg uses GIcon for every type of icon (themed, file, bitmapped). This patch doesn't change that, it simply changes the code to the new GIcon serialization API.

It would be confusing because the panel would support only a subset of the GIcon types that the menus support.

[lp:~attente/libindicator/gicon-serialization]

490. By William Hua on 2013-04-26

Remove changelog entry.

491. By William Hua on 2013-04-26

Don't free label and accessible_desc.

492. By William Hua on 2013-04-26

Clean up error checking.

493. By William Hua on 2013-05-19

Code clean-up.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/libindicator-ci/13/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-amd64-ci/13/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/libindicator-raring-armhf-ci/13/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/13/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:493
http://jenkins.qa.ubuntu.com/job/libindicator-ci/15/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libindicator-saucy-armhf-ci/1

Click here to trigger a rebuild:
http://s-jenkins:8080/job/libindicator-ci/15/rebuild

review: Approve (continuous-integration)

[Allan LeSage (allanlesage)]

Updated to saucy; Jenkins now approves :) .

[Lars Karlitski (larsu)]

Looks good to me now, thanks.

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2013-01-18 21:18:27 +0000
+++ configure.ac	2013-05-19 16:50:32 +0000
@@ -44,7 +44,7 @@
 
 GTK_REQUIRED_VERSION=2.18
 GTK3_REQUIRED_VERSION=3.6
-GIO_UNIX_REQUIRED_VERSION=2.22
+GIO_UNIX_REQUIRED_VERSION=2.37
 
 AC_ARG_WITH([gtk],
   [AS_HELP_STRING([--with-gtk],

=== modified file 'debian/control'
--- debian/control	2012-11-22 17:19:57 +0000
+++ debian/control	2013-05-19 16:50:32 +0000
@@ -12,7 +12,7 @@
                gtk-doc-tools,
                dbus-test-runner,
                xvfb,
-               libglib2.0-dev (>= 2.22),
+               libglib2.0-dev (>= 2.37),
                libgtk2.0-dev (>= 2.18),
                libgtk-3-dev (>= 2.91.3),
 Standards-Version: 3.9.2

=== modified file 'libindicator/indicator-image-helper.c'
--- libindicator/indicator-image-helper.c	2013-01-25 10:11:59 +0000
+++ libindicator/indicator-image-helper.c	2013-05-19 16:50:32 +0000
@@ -63,42 +63,64 @@
 		icon_filename = gtk_icon_info_get_filename(icon_info);
 	}
 
-	/* show a broken image if we don't have a filename */
+	if (icon_filename == NULL && !G_IS_BYTES_ICON (icon_names)) {
+		/* show a broken image if we don't have a filename or image data */
+		gtk_image_set_from_stock (image, GTK_STOCK_MISSING_IMAGE, GTK_ICON_SIZE_LARGE_TOOLBAR);
+		return;
+	}
+
+	/* Build a pixbuf */
+	GdkPixbuf * pixbuf = NULL;
+
 	if (icon_filename == NULL) {
+		GError * error = NULL;
+		GInputStream * stream = g_loadable_icon_load (G_LOADABLE_ICON (icon_names), icon_size, NULL, NULL, &error);
+
+		if (stream != NULL) {
+			pixbuf = gdk_pixbuf_new_from_stream (stream, NULL, &error);
+			g_input_stream_close (stream, NULL, NULL);
+			g_object_unref (stream);
+
+			if (pixbuf == NULL) {
+				g_warning ("Unable to load icon from data: %s", error->message);
+				g_error_free (error);
+			}
+		} else {
+			g_warning ("Unable to load icon from data: %s", error->message);
+			g_error_free (error);
+		}
+	} else {
+		GError * error = NULL;
+
+		pixbuf = gdk_pixbuf_new_from_file (icon_filename, &error);
+
+		if (pixbuf == NULL) {
+			g_warning ("Unable to load icon from file '%s': %s", icon_filename, error->message);
+			g_error_free (error);
+		}
+	}
+
+	if (pixbuf != NULL) {
+		/* Scale icon if all we get is something too big. */
+		if (gdk_pixbuf_get_height(pixbuf) > icon_size) {
+			gfloat scale = (gfloat)icon_size / (gfloat)gdk_pixbuf_get_height(pixbuf);
+			gint width = round(gdk_pixbuf_get_width(pixbuf) * scale);
+
+			GdkPixbuf * scaled = gdk_pixbuf_scale_simple(pixbuf, width, icon_size, GDK_INTERP_BILINEAR);
+			g_object_unref(G_OBJECT(pixbuf));
+			pixbuf = scaled;
+		}
+
+		/* Put the pixbuf on the image */
+		gtk_image_set_from_pixbuf(image, pixbuf);
+		g_object_unref(G_OBJECT(pixbuf));
+	} else {
 		gtk_image_set_from_stock (image, GTK_STOCK_MISSING_IMAGE, GTK_ICON_SIZE_LARGE_TOOLBAR);
-		return;
 	}
 
-	/* Build a pixbuf */
-	GError * error = NULL;
-	GdkPixbuf * pixbuf = gdk_pixbuf_new_from_file(icon_filename, &error);
-
 	if (icon_info != NULL) {
-		gtk_icon_info_free(icon_info);
-	}
-
-	if (pixbuf == NULL) {
-		g_warning("Unable to load icon from file '%s' because: %s", icon_filename, error == NULL ? "I don't know" : error->message);
-		g_clear_error (&error);
-		gtk_image_clear(image);
-		return;
-	}
-
-	/* Scale icon if all we get is something too big. */
-	if (gdk_pixbuf_get_height(pixbuf) > icon_size) {
-		gfloat scale = (gfloat)icon_size / (gfloat)gdk_pixbuf_get_height(pixbuf);
-		gint width = round(gdk_pixbuf_get_width(pixbuf) * scale);
-
-		GdkPixbuf * scaled = gdk_pixbuf_scale_simple(pixbuf, width, icon_size, GDK_INTERP_BILINEAR);
-		g_object_unref(G_OBJECT(pixbuf));
-		pixbuf = scaled;
-	}
-
-	/* Put the pixbuf on the image */
-	gtk_image_set_from_pixbuf(image, pixbuf);
-	g_object_unref(G_OBJECT(pixbuf));
-
-	return;
+		gtk_icon_info_free (icon_info);
+	}
 }
 
 /* Handles the theme changed signal to refresh the icon to make

=== modified file 'libindicator/indicator-ng.c'
--- libindicator/indicator-ng.c	2013-03-21 18:06:22 +0000
+++ libindicator/indicator-ng.c	2013-05-19 16:50:32 +0000
@@ -179,13 +179,12 @@
 }
 
 static void
-indicator_ng_set_icon_from_string (IndicatorNg *self,
-                                   const gchar *str)
+indicator_ng_set_icon_from_variant (IndicatorNg *self,
+                                    GVariant    *variant)
 {
   GIcon *icon;
-  GError *error = NULL;
 
-  if (str == NULL || *str == '\0')
+  if (variant == NULL)
     {
       if (self->entry.image)
         {
@@ -200,7 +199,7 @@
 
   gtk_widget_show (GTK_WIDGET (self->entry.image));
 
-  icon = g_icon_new_for_string (str, &error);
+  icon = g_icon_deserialize (variant);
   if (icon)
     {
       indicator_image_helper_update_from_gicon (self->entry.image, icon);
@@ -208,9 +207,10 @@
     }
   else
     {
-      g_warning ("invalid icon string '%s': %s", str, error->message);
+      gchar *text = g_variant_print (variant, TRUE);
+      g_warning ("invalid icon variant '%s'", text);
       gtk_image_set_from_stock (self->entry.image, GTK_STOCK_MISSING_IMAGE, GTK_ICON_SIZE_LARGE_TOOLBAR);
-      g_error_free (error);
+      g_free (text);
     }
 }
 
@@ -237,7 +237,7 @@
 {
   GVariant *state;
   const gchar *label = NULL;
-  const gchar *iconstr = NULL;
+  GVariant *icon = NULL;
   const gchar *accessible_desc = NULL;
   gboolean visible = TRUE;
 
@@ -254,12 +254,20 @@
   state = g_action_group_get_action_state (self->actions, self->header_action);
   if (state && g_variant_is_of_type (state, G_VARIANT_TYPE ("(sssb)")))
     {
+      gchar *iconstr = NULL;
+
       g_variant_get (state, "(&s&s&sb)", &label, &iconstr, &accessible_desc, &visible);
+
+      if (iconstr)
+        {
+          icon = g_variant_ref_sink (g_variant_new_string (iconstr));
+          g_free (iconstr);
+        }
     }
   else if (state && g_variant_is_of_type (state, G_VARIANT_TYPE ("a{sv}")))
     {
       g_variant_lookup (state, "label", "&s", &label);
-      g_variant_lookup (state, "icon", "&s", &iconstr);
+      g_variant_lookup (state, "icon", "*", &icon);
       g_variant_lookup (state, "accessible-desc", "&s", &accessible_desc);
       g_variant_lookup (state, "visible", "b", &visible);
     }
@@ -268,12 +276,14 @@
 
   if (label)
     indicator_ng_set_label (self, label);
-  if (iconstr)
-      indicator_ng_set_icon_from_string (self, iconstr);
+  if (icon)
+    indicator_ng_set_icon_from_variant (self, icon);
   if (accessible_desc)
     indicator_ng_set_accessible_desc (self, accessible_desc);
   indicator_object_set_visible (INDICATOR_OBJECT (self), visible);
 
+  if (icon)
+    g_variant_unref (icon);
   if (state)
     g_variant_unref (state);
 }