Re: Ubuntu's network-manager-applet indicator patch

[Dan Williams <dcbw redhat com>]

On Wed, 2011-02-16 at 12:27 -0500, Mathieu Trudel-Lapierre wrote:
> Hi,
> 
> I started some work a few weeks ago to support showing nm-applet as an
> application indicator in Ubuntu.
> 
> Here's the current patch as it is in use. I know there's probably
> quite a few issues with it, hence why I'm submitting it here for
> review, comments, flames, and potential inclusion ;)

So initial comments; I feel like there's a better way to do the icon
stuff instead of making all the icon functions return gboolean if
indicators are in use, but GdkPixbuf when they aren't.  How about this?

We make all the device-class icon functions (like bt_get_icon or
cdma_get_icon etc) return gboolean, but we add GdkPixbuf **out_pixbuf
and const char **out_icon_name arguments to them.  That way we can use
the same function no matter whether it's an indicator or normal
GtkStatusIcon.  The functions will return TRUE if they have a specific
icon to use, and will place that icon in the out_pixbuf (if using
GtkStatusIcon) or out_icon_name (if using appindicator).  Something more
like the attached patch (not compile tested)?

Also, are the 3G signal icons *really* called "gsm-3g-xx"?  Clearly
there is more 3G than just GSM...  Nor is GSM 3G :)

In any case, I think the approach here is pretty good, and I think we
can get the usage of #ifdef ENABLE_INDICATOR reduced by some
reorganization of the code, which makes everything more maintainable for
both indicator and !indicator.

Thanks!
Dan

diff --git a/src/applet-device-bt.c b/src/applet-device-bt.c
index a6fc091..47132f4 100644
--- a/src/applet-device-bt.c
+++ b/src/applet-device-bt.c
@@ -209,17 +209,21 @@ bt_device_state_changed (NMDevice *device,
 	}
 }
 
-static GdkPixbuf *
+static void
 bt_get_icon (NMDevice *device,
              NMDeviceState state,
              NMConnection *connection,
-             char **tip,
+             GdkPixbuf **out_pixbuf,
+             char **out_indicator_icon,
+             char **out_tip,
              NMApplet *applet)
 {
 	NMSettingConnection *s_con;
-	GdkPixbuf *pixbuf = NULL;
 	const char *id;
 
+	g_return_if_fail (out_pixbuf && *out_pixbuf);
+	g_return_if_fail (out_indicator_icon && *out_indicator_icon);
+
 	id = nm_device_get_iface (NM_DEVICE (device));
 	if (connection) {
 		s_con = NM_SETTING_CONNECTION (nm_connection_get_setting (connection, NM_TYPE_SETTING_CONNECTION));
@@ -240,14 +244,15 @@ bt_get_icon (NMDevice *device,
 		*tip = g_strdup_printf (_("Requesting a network address for '%s'..."), id);
 		break;
 	case NM_DEVICE_STATE_ACTIVATED:
-		pixbuf = nma_icon_check_and_load ("nm-device-wwan", &applet->wwan_icon, applet);
-		*tip = g_strdup_printf (_("Mobile broadband connection '%s' active"), id);
+		if (out_pixbuf)
+			*out_pixbuf = nma_icon_check_and_load ("nm-device-wwan", &applet->wwan_icon, applet);
+		if (out_indicator_icon)
+			*out_indicator_icon = g_strdup ("nm-device-wwan");
+		*out_tip = g_strdup_printf (_("Mobile broadband connection '%s' active"), id);
 		break;
 	default:
 		break;
 	}
-
-	return pixbuf;
 }
 
 typedef struct {
diff --git a/src/applet.c b/src/applet.c
index 088de7d..c7baa20 100644
--- a/src/applet.c
+++ b/src/applet.c
@@ -2313,10 +2313,30 @@ foo_client_setup (NMApplet *applet)
 		g_idle_add (foo_set_initial_state, applet);
 }
 
-static GdkPixbuf *
-applet_common_get_device_icon (NMDeviceState state, NMApplet *applet)
+#ifndef ENABLE_INDICATOR
+static void
+check_and_load_connecting_icons (NMApplet *applet)
+{
+	int i, j;
+
+	for (i = 0; i < NUM_CONNECTING_STAGES; i++) {
+		for (j = 0; j < NUM_CONNECTING_FRAMES; j++) {
+			char *name;
+
+			name = g_strdup_printf ("nm-stage%02d-connecting%02d", i+1, j+1);
+			nma_icon_check_and_load (name, &applet->network_connecting_icons[i][j], applet);
+			g_free (name);
+		}
+	}
+}
+#endif
+
+static void
+applet_common_get_device_icon (NMDeviceState state,
+                               GdkPixbuf **out_pixbuf,
+                               const char **out_indicator_icon,
+                               NMApplet *applet)
 {
-	GdkPixbuf *pixbuf = NULL;
 	int stage = -1;
 
 	switch (state) {
@@ -2335,25 +2355,21 @@ applet_common_get_device_icon (NMDeviceState state, NMApplet *applet)
 	}
 
 	if (stage >= 0) {
-		int i, j;
-
-		for (i = 0; i < NUM_CONNECTING_STAGES; i++) {
-			for (j = 0; j < NUM_CONNECTING_FRAMES; j++) {
-				char *name;
-
-				name = g_strdup_printf ("nm-stage%02d-connecting%02d", i+1, j+1);
-				nma_icon_check_and_load (name, &applet->network_connecting_icons[i][j], applet);
-				g_free (name);
-			}
+		/* Don't overwrite pixbufs of names given by specific device classes */
+		if (out_pixbuf && !*out_pixbuf) {
+			check_and_load_connecting_icons (applet);
+			*out_pixbuf = applet->network_connecting_icons[stage][applet->animation_step];
+		}
+		if (out_indicator_icon && !*out_indicator_icon) {
+			*out_indicator_icon = g_strdup_printf ("nm-stage%02d-connecting%02d",
+			                                       stage + 1,
+			                                       applet->animation_step + 1);
 		}
 
-		pixbuf = applet->network_connecting_icons[stage][applet->animation_step];
 		applet->animation_step++;
 		if (applet->animation_step >= NUM_CONNECTING_FRAMES)
 			applet->animation_step = 0;
 	}
-
-	return pixbuf;
 }
 
 static char *
@@ -2392,12 +2408,14 @@ get_tip_for_device_state (NMDevice *device,
 	return tip;
 }
 
-static GdkPixbuf *
-applet_get_device_icon_for_state (NMApplet *applet, char **tip)
+static void
+applet_get_device_icon_for_state (NMApplet *applet,
+                                  GdkPixbuf **out_pixbuf,
+                                  const char **out_indicator_icon,
+                                  char **out_tip)
 {
 	NMActiveConnection *active;
 	NMDevice *device = NULL;
-	GdkPixbuf *pixbuf = NULL;
 	NMDeviceState state = NM_DEVICE_STATE_UNKNOWN;
 	NMADeviceClass *dclass;
 
@@ -2421,15 +2439,13 @@ applet_get_device_icon_for_state (NMApplet *applet, char **tip)
 		NMConnection *connection;
 
 		connection = applet_find_active_connection_for_device (device, applet, NULL);
-		pixbuf = dclass->get_icon (device, state, connection, tip, applet);
-		if (!*tip)
+		dclass->get_icon (device, state, connection, out_pixbuf, out_indicator_icon, tip, applet);
+		if (tip && !*tip)
 			*tip = get_tip_for_device_state (device, state, connection);
 	}
 
 out:
-	if (!pixbuf)
-		pixbuf = applet_common_get_device_icon (state, applet);
-	return pixbuf;
+	applet_common_get_device_icon (state, applet, out_pixbuf, out_indicator_icon);
 }
 
 static char *
@@ -2512,11 +2528,16 @@ applet_update_icon (gpointer user_data)
 		dev_tip = g_strdup (_("No network connection"));
 		break;
 	default:
-		pixbuf = applet_get_device_icon_for_state (applet, &dev_tip);
+#ifdef ENABLE_INDICATOR
+		applet_get_device_icon_for_state (applet, NULL, &indicator_name, NULL);
+#else
+		applet_get_device_icon_for_state (applet, &pixbuf, NULL, &dev_tip);
+#endif
 		break;
 	}
 
-	foo_set_icon (applet, pixbuf, ICON_LAYER_LINK);
+	if (pixbuf)
+		foo_set_icon (applet, pixbuf, ICON_LAYER_LINK);
 
 	/* VPN state next */
 	pixbuf = NULL;
diff --git a/src/applet.h b/src/applet.h
index aafdc4d..6df91b1 100644
--- a/src/applet.h
+++ b/src/applet.h
@@ -227,10 +227,12 @@ struct NMADeviceClass {
 	                                        NMDeviceStateReason reason,
 	                                        NMApplet *applet);
 
-	GdkPixbuf *    (*get_icon)             (NMDevice *device,
+	void           (*get_icon)             (NMDevice *device,
 	                                        NMDeviceState state,
 	                                        NMConnection *connection,
-	                                        char **tip,
+	                                        GdkPixbuf **out_pixbuf,
+	                                        char **out_indicator_icon,
+	                                        char **out_tip,
 	                                        NMApplet *applet);
 
 	void           (*get_more_info)        (NMDevice *device,