Created attachment 197898 [details] [review]
st-texture-cache: Don't cache GIcons which cannot be serialized

For GIcons we use g_icon_to_string() in the key, but the function
will return NULL if the icon cannot be serialized. As a result,
all non-serializable GIcons of the same size end up with the same
cache key - an example for this are contacts with avatars, which
currently all end up with the same image.
To fix, opt out of caching for GIcons which cannot be serialized.

Review of attachment 197898 [details] [review]:

Makes sense, though you might say for the first sentence:

A return value of NULL indicates that the icon can not be serialized, so don't have a unique identifier for it as a cache key, and thus can't cache.

Attachment 197898 [details] pushed as e49a595 - st-texture-cache: Don't cache GIcons which cannot be serialized

(In reply to comment #2)
> Makes sense, though you might say for the first sentence:
> 
> A return value of NULL indicates that the icon can not be serialized, so don't
> have a unique identifier for it as a cache key, and thus can't cache.

Pushed with the suggested comment.

The problem still happens when there are pending requests, reopening.

Created attachment 198365 [details] [review]
texture-cache: Don't share requests for uncachable textures

Make create_texture_and_ensure_request() aware of the caching
policy to avoid returning the same texture for different
images.

Review of attachment 198365 [details] [review]:

Hm, can't we just skip create_texture_and_ensure_request if the policy is NONE?  I.e. just call

  *texture = (ClutterActor *) create_default_texture (cache);
  clutter_actor_set_size (*texture, size, size);

in that case.

Something like the following (untested)

$ git diff
diff --git a/src/st/st-texture-cache.c b/src/st/st-texture-cache.c
index 1e83a0d..243cb11 100644
--- a/src/st/st-texture-cache.c
+++ b/src/st/st-texture-cache.c
@@ -1203,7 +1203,12 @@ load_gicon_with_colors (StTextureCache    *cache,
     }
   g_free (gicon_string);
 
-  if (create_texture_and_ensure_request (cache, key, size, &request, &texture))
+  if (policy == ST_TEXTURE_CACHE_POLICY_NONE)
+    {
+      texture = (ClutterActor *) create_default_texture (cache);
+      clutter_actor_set_size (texture, size, size);
+    }
+  else if (create_texture_and_ensure_request (cache, key, size, &request, &texture))
     {
       g_free (key);
       return texture;

(In reply to comment #7)
> Something like the following (untested)
> 
> $ git diff
> diff --git a/src/st/st-texture-cache.c b/src/st/st-texture-cache.c
> index 1e83a0d..243cb11 100644
> --- a/src/st/st-texture-cache.c
> +++ b/src/st/st-texture-cache.c
> @@ -1203,7 +1203,12 @@ load_gicon_with_colors (StTextureCache    *cache,
>      }
>    g_free (gicon_string);
> 
> -  if (create_texture_and_ensure_request (cache, key, size, &request,
> &texture))
> +  if (policy == ST_TEXTURE_CACHE_POLICY_NONE)
> +    {
> +      texture = (ClutterActor *) create_default_texture (cache);
> +      clutter_actor_set_size (texture, size, size);
> +    }

Doesn't that mean that we never create a request to fill in the real image?

(In reply to comment #8)
> (In reply to comment #7)
> > Something like the following (untested)
> > 
> > $ git diff
> > diff --git a/src/st/st-texture-cache.c b/src/st/st-texture-cache.c
> > index 1e83a0d..243cb11 100644
> > --- a/src/st/st-texture-cache.c
> > +++ b/src/st/st-texture-cache.c
> > @@ -1203,7 +1203,12 @@ load_gicon_with_colors (StTextureCache    *cache,
> >      }
> >    g_free (gicon_string);
> > 
> > -  if (create_texture_and_ensure_request (cache, key, size, &request,
> > &texture))
> > +  if (policy == ST_TEXTURE_CACHE_POLICY_NONE)
> > +    {
> > +      texture = (ClutterActor *) create_default_texture (cache);
> > +      clutter_actor_set_size (texture, size, size);
> > +    }
> 
> Doesn't that mean that we never create a request to fill in the real image?

You're right, nevermind.  Original patch looks good then.

Attachment 198365 [details] pushed as 23a4d4c - texture-cache: Don't share requests for uncachable textures