Merge lp:~ballogy/indicator-session/better-fallback-icon into lp:indicator-session/13.04

[Balló György]

This change reverts the commit 358 [1], and provides a better and simpler fallback icon handling.

Instead of monitoring theme changes and use a hard-coded "gtk-missing-image" as fallback, use g_themed_icon_new_with_default_fallbacks (). The gtk-missing-image is too generic, however, there is a "system" icon available in the gnome-icon-theme, which is a more appropriate icon.

[1] http://bazaar.launchpad.net/~indicator-applet-developers/indicator-session/trunk.13.04/revision/358

[Mathieu Trudel-Lapierre (cyphermox)]

This would break bug https://bugs.launchpad.net/indicator-appmenu/+bug/1088778 again; in the way that all g_themed_icon_new_with_default_fallbacks() does is shorten the name at dashes until it finds a match; but doesn't take into account the cases where there are none -- which should still be gtk_missing_icon.

review: Needs Fixing

[Lars Karlitski (larsu)]

This patch fixes two unrelated things:

(1) Revert r358, which doesn't seem to be necessary anymore. At least I cannot reproduce bug #1048348 when reverting r358: the "missing" icon is shown for all themes that don't have system-devices-panel. The icon is also properly updated when the theme changes. Maybe this was an issue in GtkImage that has since been fixed?

(2) Specifiy that the default icon fallbacks ("system-devices-panel", "system-devices", "system") should be used, so that themes that don't have "system-devices-panel" fall back to "system".

I like both changes, but would appreciate someone else confirming (1) before this gets merged.

Sorry for misleading you last Friday, Mathieu. I didn't have enough time to look at this in detail.

review: Needs Information

[Peter Hurley (phurley)]

> (2) Specifiy that the default icon fallbacks ("system-devices-panel", "system-
> devices", "system") should be used, so that themes that don't have "system-
> devices-panel" fall back to "system".

Some themes (including HighContrast, which makes this an accessibility issue) do not have default fallbacks from "system-devices-panel".

[Lars Karlitski (larsu)]

> Some themes (including HighContrast, which makes this an accessibility issue)
> do not have default fallbacks from "system-devices-panel".

Right, but this is unrelated to this patch: in HighContrast, the missing icon is shown both with and without this patch.

I suggest opening a bug agains gnome-accessibility-themes or making HighContrast fall back to Humanity (as suggested by seb128 at [1]).

[1] https://bugs.launchpad.net/unity/+bug/975563/comments/2

[Peter Hurley (phurley)]

> > Some themes (including HighContrast, which makes this an accessibility
> issue)
> > do not have default fallbacks from "system-devices-panel".
>
> Right, but this is unrelated to this patch: in HighContrast, the missing icon
> is shown both with and without this patch.

This branch is called 'better-fallback-icon' specifically because it proposes to fix bug #975563 by showing a better icon than 'gtk-missing-icon'. My comment notes that this branch does not fix that because there is no fallback icon.

> I suggest opening a bug agains gnome-accessibility-themes or making
> HighContrast fall back to Humanity (as suggested by seb128 at [1]).
>
> [1] https://bugs.launchpad.net/unity/+bug/975563/comments/2

Have a look at https://code.launchpad.net/~phurley/indicator-session/lp-957563
before pushing this off into some other package.

[Balló György (ballogy)]

I confirm that my patch tries to solve two things.

1. In my experience the indicator_session_update_icon_callback() is not needed, because the icon gets updated correctly on theme change.

2. I think that "system" is a generic enough name, and should be provided by every theme. And most of icon themes inherit the gnome-icon-theme, including the HighContrast icon theme[1], so it should not be a problem.

However, I think the best solution would be to use similar icon names as they called in gnome-icon-theme-symbolic (as it was reported[2]), so:
- "system-devices-panel" would be "system-shutdown-symbolic"
- "system-devices-panel-information" would be "system-shutdown-symbolic-information"
- "system-devices-panel-alert" would be "system-shutdown-symbolic-alert"

This would provides the best fallback names for all themes:
- GNOME Symbolic falls back to "system-shutdown-symbolic"
- GNOME, HighContrast and Humanity falls back to "system-shutdown"

[1] http://git.gnome.org/browse/gnome-themes-standard/tree/themes/HighContrast/icons/index.theme.in
[2] https://bugs.launchpad.net/indicator-session/+bug/903819

Unmerged revisions

381. By Balló György on 2013-01-02

Improve fallback icon handling

Instead of monitoring theme changes and use a
hard-coded "gtk-missing-image", use
g_themed_icon_new_with_default_fallbacks () to get a proper fallback
icon (e.g. "system").

=== modified file 'src/indicator-session.c'
--- src/indicator-session.c	2012-11-14 20:20:13 +0000
+++ src/indicator-session.c	2013-01-02 12:47:24 +0000
@@ -64,7 +64,6 @@
   GDBusProxy * service_proxy;
   GSettings * settings;
   DbusmenuClient * menu_client;
-  GtkIconTheme * icon_theme;
 };
 
 GType indicator_session_get_type (void);
@@ -79,7 +78,6 @@
                                DbusmenuClient * client,
                                gpointer user_data);
 static void on_menu_layout_updated (DbusmenuClient * client, IndicatorSession * session);
-static void indicator_session_update_icon_callback (GtkWidget * widget, gpointer callback_data);
 static void indicator_session_update_icon_and_a11y (IndicatorSession * self);
 static void indicator_session_update_users_label (IndicatorSession* self,
                                                   const gchar* name);
@@ -129,13 +127,6 @@
   self->entry.image = GTK_IMAGE (gtk_image_new());
   self->entry.menu = GTK_MENU (dbusmenu_gtkmenu_new(INDICATOR_SESSION_DBUS_NAME,
                                                     INDICATOR_SESSION_DBUS_OBJECT));
-  /* We need to check if the current icon theme has the hard coded icons.
-   * If not, we'll fall back to a standard icon */
-  self->icon_theme = gtk_icon_theme_get_default();
-  g_signal_connect(G_OBJECT(self->icon_theme),
-                   "changed",
-                   G_CALLBACK(indicator_session_update_icon_callback), self);
-
   indicator_session_update_icon_and_a11y (self);
   g_settings_bind (self->settings, "show-real-name-on-panel",
                    self->entry.label, "visible",
@@ -408,13 +399,11 @@
   else
     icon = ICON_ALERT;
 
-  if (gtk_icon_theme_has_icon (indicator->icon_theme, icon) == FALSE)
-    icon = "gtk-missing-image";
-
-  g_debug (G_STRLOC" setting icon to \"%s\"", icon);
-  gtk_image_set_from_icon_name (GTK_IMAGE(indicator->entry.image),
-                                icon,
-                                GTK_ICON_SIZE_BUTTON);
+  GIcon * gicon = g_themed_icon_new_with_default_fallbacks (icon);
+  gtk_image_set_from_gicon (GTK_IMAGE(indicator->entry.image),
+                            gicon,
+                            GTK_ICON_SIZE_BUTTON);
+  g_object_unref(gicon);
 }
   
 static int
@@ -448,12 +437,6 @@
 }
 
 static void
-indicator_session_update_icon_callback (GtkWidget * widget, gpointer callback_data)
-{
-  indicator_session_update_icon_and_a11y ((IndicatorSession *)callback_data);
-}
-
-static void
 indicator_session_update_icon_and_a11y (IndicatorSession * indicator)
 {
   const int disposition = calculate_disposition (indicator);