Merge lp:~cyphermox/libappindicator/lp708118 into lp:libappindicator

[Mathieu Trudel-Lapierre]

[Ted Gould (ted)]

We really need to keep adding the -panel suffix on the icon name so that
an appindicator will look the same on the panel if it's using the status
area, or if it's using indicator-application. So, we can switch to
using icon-name if we can set the "use-fallback" property on the Image
in the GtkStatusIcon. I don't see a way to do that currently from the
GtkStatusIcon API though.

I think that we could patch the GtkStatusIcon implementation to set this
value on it's image always. It may change the behavior of some Status
Icons, but only so they don't have a broken image icon (which seems like
an improvement to me).

  review needsfixing

review: Needs Fixing

[Mathieu Trudel-Lapierre (cyphermox)]

Checking locally, I haven't been able to find any icons with a -panel suffix installed on my system. That along with creating the icons with fallbacks meant AFAICT that the suffix would get stripped out if no icon was found -- which yields the same result as not including the suffix.

I tested this with nm-applet and there doesn't seem to be any issues displaying the same icon in indicator mode or in the fallback to a GtkStatusIcon in the notification area.

As per the comments in bug 708118, another option might be to not unref the gicon immediately at the end of status_icon_changes, but that would likely mean leaking the icons to some amount.

[Ted Gould (ted)]

On Tue, 2011-03-15 at 13:30 +0000, Mathieu Trudel-Lapierre wrote:
> Checking locally, I haven't been able to find any icons with a -panel
> suffix installed on my system. That along with creating the icons with
> fallbacks meant AFAICT that the suffix would get stripped out if no
> icon was found -- which yields the same result as not including the suffix.

I've got about 54:

$ locate panel.png | grep icons | wc -l

But the issue is more that this is what is going to happen in
indicator-application, so we want to match the same behavior. It's true
that there aren't a huge number of them, but I think it's important to
be consistent.

> As per the comments in bug 708118, another option might be to not unref
> the gicon immediately at the end of status_icon_changes, but that would
> likely mean leaking the icons to some amount.

Just to be curious, did you try unref'ing it in an idle function? I'm
curious if that would make things better.

In general, I think adding the "use-fallback" to the GtkImage in
GtkStatusIcon is the best solution. Are you against that?

[Mathieu Trudel-Lapierre (cyphermox)]

I'm not at all against it; however, as per the bug and further discussion with Sebastien Bacher on IRC, seems like there's indeed something rather weird happening directly at the GTK level. That's why the bug was reassigned.

Since it's not quite clear whether a fix should be in GTK or libappindicator, I'm resetting this to Work In Progress, to be reviewed again at a later time if necessary.

review: Needs Fixing

[Ted Gould (ted)]

Don't know why I can't set superceeded, but that's my goal here:

 https://code.launchpad.net/~ted/libappindicator/lp708118v2/+merge/54570

Unmerged revisions

202. By Mathieu Trudel-Lapierre on 2011-03-11

fallback: set icons for the fallback GtkStatusIcon using icon names rather
than building a GThemedIcon.

Also drops the extra "-panel" suffix to the icons.

=== modified file 'src/app-indicator.c'
--- src/app-indicator.c	2011-03-08 21:58:16 +0000
+++ src/app-indicator.c	2011-03-14 15:35:36 +0000
@@ -48,8 +48,6 @@
 #include "dbus-shared.h"
 #include "generate-id.h"
 
-#define PANEL_ICON_SUFFIX  "panel"
-
 /**
 	AppIndicatorPrivate:
 
@@ -182,7 +180,6 @@
 static void status_icon_activate (GtkStatusIcon * icon, gpointer data);
 static void status_icon_menu_activate (GtkStatusIcon *status_icon, guint button, guint activate_time, gpointer user_data);
 static void unfallback (AppIndicator * self, GtkStatusIcon * status_icon);
-static gchar * append_panel_icon_suffix (const gchar * icon_name);
 static void watcher_owner_changed (GObject * obj, GParamSpec * pspec, gpointer user_data);
 static void theme_changed_cb (GtkIconTheme * theme, gpointer user_data);
 static GVariant * bus_get_prop (GDBusConnection * connection, const gchar * sender, const gchar * path, const gchar * interface, const gchar * property, GError ** error, gpointer user_data);
@@ -1471,34 +1468,28 @@
 status_icon_changes (AppIndicator * self, gpointer data)
 {
 	GtkStatusIcon * icon = GTK_STATUS_ICON(data);
-	GIcon *themed_icon = NULL;
 	gchar *longname = NULL;
-
-	switch (app_indicator_get_status(self)) {
+	guint32 status;
+
+	status = app_indicator_get_status (self);
+
+	if (status == APP_INDICATOR_STATUS_ATTENTION)
+		longname = g_strdup(app_indicator_get_attention_icon(self));
+	else
+		longname = g_strdup(app_indicator_get_icon(self));
+
+	gtk_status_icon_set_from_icon_name(icon, longname);
+
+	switch (status) {
+	case APP_INDICATOR_STATUS_ACTIVE:
+	case APP_INDICATOR_STATUS_ATTENTION:
+		gtk_status_icon_set_visible(icon, TRUE);
+		break;
 	case APP_INDICATOR_STATUS_PASSIVE:
-		longname = append_panel_icon_suffix(app_indicator_get_icon(self));
-		themed_icon = g_themed_icon_new_with_default_fallbacks (longname);
 		gtk_status_icon_set_visible(icon, FALSE);
-		gtk_status_icon_set_from_gicon(icon, themed_icon);
-		break;
-	case APP_INDICATOR_STATUS_ACTIVE:
-		longname = append_panel_icon_suffix(app_indicator_get_icon(self));
-		themed_icon = g_themed_icon_new_with_default_fallbacks (longname);
-		gtk_status_icon_set_from_gicon(icon, themed_icon);
-		gtk_status_icon_set_visible(icon, TRUE);
-		break;
-	case APP_INDICATOR_STATUS_ATTENTION:
-		longname = append_panel_icon_suffix(app_indicator_get_attention_icon(self));
-		themed_icon = g_themed_icon_new_with_default_fallbacks (longname);
-		gtk_status_icon_set_from_gicon(icon, themed_icon);
-		gtk_status_icon_set_visible(icon, TRUE);
 		break;
 	};
 
-	if (themed_icon) {
-		g_object_unref (themed_icon);
-	}
-
 	if (longname) {
 		g_free(longname);
 	}
@@ -1547,24 +1538,6 @@
 	return;
 }
 
-/* A helper function that appends PANEL_ICON_SUFFIX to the given icon name
-   if it's missing. */
-static gchar *
-append_panel_icon_suffix (const gchar *icon_name)
-{
-	gchar * long_name = NULL;
-
-	if (!g_str_has_suffix (icon_name, PANEL_ICON_SUFFIX)) {
-		long_name =
-		    g_strdup_printf("%s-%s", icon_name, PANEL_ICON_SUFFIX);
-        } else {
-           	long_name = g_strdup (icon_name);
-        }
-
-	return long_name;	
-}
-
-
 /* ************************* */
 /*    Public Functions       */
 /* ************************* */