Created attachment 173729 [details] [review]
Make the icon theme part of the StThemeContext

Add st_theme_context_set_icon_theme(); making the icon theme be part
of the theme context will be useful for allowing named icons to be
loaded by StThemeNode. It also means that we can just reuse
the ::style-changed signal to reload named icons on theme changes,
instead of having to do separate notification.

Created attachment 173730 [details] [review]
Evict cached icons when the icon theme changes

When there is a ::changed signal on the GtkIconTheme, because the
configured theme or it's contents have changed, blow away any icons
we have cached so the next lookup will get the right icons.

*** Bug 611949 has been marked as a duplicate of this bug. ***

Comment on attachment 173729 [details] [review]
Make the icon theme part of the StThemeContext

>+  if (context->icon_theme)
>+    {
>+      g_signal_handlers_disconnect_by_func (icon_theme,

should be context->icon_theme

>+  if (context->icon_theme)
>+    {
>+      g_object_ref (context->icon_theme);
>+      g_signal_connect (icon_theme, "changed",

and that one's not actually wrong, but it's inconsistent.

Also, shouldn't st_theme_context_set_icon_theme() call st_theme_context_changed()?

Comment on attachment 173730 [details] [review]
Evict cached icons when the icon theme changes

> static void
>+theme_context_evict_icons (StThemeContext *context)
>+{
>+  /* The hackiness is that though we set a #GtkIconTheme for the context,
>+   * when we actually do lookups in StTextureCache we are just using
>+   * gtk_icon_theme_get_default(). That's why we don't pass a GtkIconTheme
>+   * to st_texture_cache_evict_icons().
>+   */
>+  StTextureCache *cache = st_texture_cache_get_default ();
>+  st_texture_cache_evict_icons (cache);
>+}

The comment is weird because it suggests that "the hackiness" has already been referred to elsewhere, which afaict it hasn't.

But anyway, the coupling here seems weird. If StTextureCache isn't using the theme context's icon theme, shouldn't it just listen to GtkIconTheme::changed itself? Or, if we were going to fix StTextureCache to have an associated StThemeContext, it could listen to StThemeContext::changed. Or failing that, we should at least make StTextureCache and StThemeContext have the same scope rather than one being global and one being per-stage.

>+    context.set_icon_theme(Gtk.IconTheme.get_default());

does this belong in the previous patch?

(In reply to comment #5)
> (From update of attachment 173730 [details] [review])
> > static void
> >+theme_context_evict_icons (StThemeContext *context)
> >+{
> >+  /* The hackiness is that though we set a #GtkIconTheme for the context,
> >+   * when we actually do lookups in StTextureCache we are just using
> >+   * gtk_icon_theme_get_default(). That's why we don't pass a GtkIconTheme
> >+   * to st_texture_cache_evict_icons().
> >+   */
> >+  StTextureCache *cache = st_texture_cache_get_default ();
> >+  st_texture_cache_evict_icons (cache);
> >+}
> 
> The comment is weird because it suggests that "the hackiness" has already been
> referred to elsewhere, which afaict it hasn't.

Yeah, the wording is a bit odd can be improved.
 
> But anyway, the coupling here seems weird. If StTextureCache isn't using the
> theme context's icon theme, shouldn't it just listen to GtkIconTheme::changed
> itself? 

Hmm, so probably what this would look like is:

 - Add a signal to StTexturCache
 - Emit that signal *after* evicting icons
 - Make StThemeContext catch that signal and invalidate styles

You can't just have two independent signal connections because the ordering matters... we need to invalidate styles after we have invalidated icons. Maybe I'll give that approach a try and just skip setting the icon theme on the theme context.

> Or, if we were going to fix StTextureCache to have an associated
> StThemeContext, it could listen to StThemeContext::changed. Or failing that, we
> should at least make StTextureCache and StThemeContext have the same scope
> rather than one being global and one being per-stage.

Clearly, what I have here is half-baked and only works when everybody is using a single GtkIconTheme. I was sort of hoping to just sneak it through like that, and deal with figuring things out when we actually had a reason to...

I don't think it's conceptually right to have StTextureCache be per-stage ... in a multiple-toplevel application - the different stages should share the same cache of textures.

And it's not right for StThemeContext to be global - it's a lightweight object representing information shared about a tree of StThemeNodes.

I think what would be "right" is either:

 - Have StTextureCache be per GtkIconTheme (which basically means per GdkScreen). We could add st_widget_get_texture_cache() to avoid people having to juggle IconThemes at the JS level.

Or:

 - Pass the GtkIconTheme in to the named-icon lookup functions, and make the GtkIconTheme part of the cache key. I guess with %p and use weak pointers to get notification when an icon theme vanishes. 

Practically speaking, neither really helps the shell at all.

> >+    context.set_icon_theme(Gtk.IconTheme.get_default());
> 
> does this belong in the previous patch?

yep.

(In reply to comment #6)
> Practically speaking, neither really helps the shell at all.

yeah, i don't think it's worth putting *too* much work into it. I didn't notice the invalidation/redrawing ordering constraint when I was thinking about it before...

Created attachment 174370 [details] [review]
Remove _for_theme() variant of st_texture_cache_load_icon_name()

Now that we're using St.Icon in the Javascript, there is no reason
to have separate st_texture_cache_load_icon_name() and
st_texture_cache_load_icon_name_for_theme(), instead just add
the StThemeNode argument to st_texture_cache_load_icon_name().

Comment on attachment 174370 [details] [review]
Remove _for_theme() variant of st_texture_cache_load_icon_name()

Oops attached that patch to the wrong bug. Now pushed as part of bug 633865
where it belonged.

New testing methology with a gnome3 gnome-settings-daemon:

 gsettings set org.gnome.desktop.interface icon-theme HighContrast
 gsettings set org.gnome.desktop.interface icon-theme gnome

Created attachment 174371 [details] [review]
Handle icon theme changes

Here's a new simpler approach. I'm aware of the inconsistencies that:

 - For StTextureCache I call gtk_icon_theme_get_default() once
   and store it in a member variable.
 - Except for the usage of gtk_icon_theme_get_default() where
   I don't (in a thread, in a fallback case, StTextureCache isn't
   readily available)
 - For StThemeContext, I call st_texture_cache_get_default()
   both in init() and finalize() and assume they are the same.

And they don't worry me much :-)

Created attachment 174372 [details] [review]
Handle icon theme changes

Add an explanatory comment to the code.

Attachment 174372 [details] pushed as 8734a59 - Handle icon theme changes