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

Proposed by Balló György
Status: Work in progress
Proposed branch: lp:~ballogy/indicator-session/better-fallback-icon
Merge into: lp:indicator-session/13.04
Diff against target: 65 lines (+5/-22)
1 file modified
src/indicator-session.c (+5/-22)
To merge this branch: bzr merge lp:~ballogy/indicator-session/better-fallback-icon
Reviewer Review Type Date Requested Status
Lars Karlitski (community) Needs Information
Mathieu Trudel-Lapierre Needs Fixing
Review via email: mp+141608@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Mathieu Trudel-Lapierre (cyphermox) wrote :

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
Revision history for this message
Lars Karlitski (larsu) wrote :

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
Revision history for this message
Peter Hurley (phurley) wrote :

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

Revision history for this message
Lars Karlitski (larsu) wrote :

> 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

Revision history for this message
Peter Hurley (phurley) wrote :

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

Revision history for this message
Balló György (ballogy) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/indicator-session.c'
2--- src/indicator-session.c 2012-11-14 20:20:13 +0000
3+++ src/indicator-session.c 2013-01-02 12:47:24 +0000
4@@ -64,7 +64,6 @@
5 GDBusProxy * service_proxy;
6 GSettings * settings;
7 DbusmenuClient * menu_client;
8- GtkIconTheme * icon_theme;
9 };
10
11 GType indicator_session_get_type (void);
12@@ -79,7 +78,6 @@
13 DbusmenuClient * client,
14 gpointer user_data);
15 static void on_menu_layout_updated (DbusmenuClient * client, IndicatorSession * session);
16-static void indicator_session_update_icon_callback (GtkWidget * widget, gpointer callback_data);
17 static void indicator_session_update_icon_and_a11y (IndicatorSession * self);
18 static void indicator_session_update_users_label (IndicatorSession* self,
19 const gchar* name);
20@@ -129,13 +127,6 @@
21 self->entry.image = GTK_IMAGE (gtk_image_new());
22 self->entry.menu = GTK_MENU (dbusmenu_gtkmenu_new(INDICATOR_SESSION_DBUS_NAME,
23 INDICATOR_SESSION_DBUS_OBJECT));
24- /* We need to check if the current icon theme has the hard coded icons.
25- * If not, we'll fall back to a standard icon */
26- self->icon_theme = gtk_icon_theme_get_default();
27- g_signal_connect(G_OBJECT(self->icon_theme),
28- "changed",
29- G_CALLBACK(indicator_session_update_icon_callback), self);
30-
31 indicator_session_update_icon_and_a11y (self);
32 g_settings_bind (self->settings, "show-real-name-on-panel",
33 self->entry.label, "visible",
34@@ -408,13 +399,11 @@
35 else
36 icon = ICON_ALERT;
37
38- if (gtk_icon_theme_has_icon (indicator->icon_theme, icon) == FALSE)
39- icon = "gtk-missing-image";
40-
41- g_debug (G_STRLOC" setting icon to \"%s\"", icon);
42- gtk_image_set_from_icon_name (GTK_IMAGE(indicator->entry.image),
43- icon,
44- GTK_ICON_SIZE_BUTTON);
45+ GIcon * gicon = g_themed_icon_new_with_default_fallbacks (icon);
46+ gtk_image_set_from_gicon (GTK_IMAGE(indicator->entry.image),
47+ gicon,
48+ GTK_ICON_SIZE_BUTTON);
49+ g_object_unref(gicon);
50 }
51
52 static int
53@@ -448,12 +437,6 @@
54 }
55
56 static void
57-indicator_session_update_icon_callback (GtkWidget * widget, gpointer callback_data)
58-{
59- indicator_session_update_icon_and_a11y ((IndicatorSession *)callback_data);
60-}
61-
62-static void
63 indicator_session_update_icon_and_a11y (IndicatorSession * indicator)
64 {
65 const int disposition = calculate_disposition (indicator);

Subscribers

People subscribed via source and target branches