Although the icon is still not loaded before the previous is hidden, I cannot reproduce it anymore after commit 21ac22598101c25f1705fa6dea263a16ffe49be1.

Improving this further would mean a massive refactoring to StTextureCache (making texture loads asyncronous, rather than immediately returning an empty ClutterTexture actor and filling it later), which I'd like to avoid.

I've just checked with an up-to-date Shell, and it's still visible for me.

We can fix this at the StIcon level; if the texture cache returns a ClutterTexture with opacity 0, that means it's being loaded asychronously. In that case, StIcon could continue to display the old icon until the new one emits notify::opacity

Created attachment 175537 [details] [review]
StIcon: don't show the icon until fully loaded

When updating the texture, keep the old one until the new one is
loaded.
Also fix StIcon to remove st_icon_map / st_icon_unmap, not needed
as of clutter 1.5.8

Comment on attachment 175537 [details] [review]
StIcon: don't show the icon until fully loaded

mostly good

>+  self->priv->icon_texture = NULL;
>+  self->priv->pending_texture = NULL;
>+  self->priv->opacity_handler_id = 0;

The existing code was inconsistent about this, but generally we only initialize fields that have non-0/NULL values. (GObject initializes the structs to all-bits-0, and we don't support any platforms where either 0.0 or NULL is not all-bits-0.)

>+      priv->pending_texture = g_object_ref_sink (st_texture_cache_load_gicon (cache,

ugh, too long. do the ref_sink separately after the assignment

>+      priv->pending_texture = g_object_ref_sink (st_texture_cache_load_icon_name (cache,

likewise

>+      if (clutter_actor_get_opacity (priv->pending_texture) != 0 || priv->icon_texture == NULL)
>+        /* This icon is ready for showing, or nothing else is already showing */
>+        st_icon_real_update (icon);
>+      else
>+        /* Will be shown when fully loaded */
>+        priv->opacity_handler_id = g_signal_connect (priv->pending_texture, "notify::opacity", G_CALLBACK (opacity_changed_cb), icon);

Use {}s around the then/else bodies please even though it's not syntactically necessary. It looks weird the way it is now.

Created attachment 175658 [details] [review]
StIcon: don't show the icon until fully loaded

When updating the texture, keep the old one until the new one is
loaded.
Also fix StIcon to remove st_icon_map / st_icon_unmap, not needed
as of clutter 1.5.8

Review of attachment 175658 [details] [review]:

Looks very good to me, just a few comments.

In a git commit message "also" universally means "I should have split this out into a separate patch" :-)

::: src/st/st-icon.c
@@ +345,3 @@
   self->priv->icon_type = DEFAULT_ICON_TYPE;
 
+  self->priv->shadow_material = COGL_INVALID_HANDLE;

separate patch as well

@@ +387,2 @@
 static void
+st_icon_real_update (StIcon *icon)

In a GObject implementation "real" has a specific meaning ... default virtual function implementation that would otherwise clash with the public wrapper method

 clutter_actor_class->map = clutter_actor_real_map;

I'd probably call this st_icon_finish_update.

@@ +414,3 @@
+
+static void
+opacity_changed_cb (GObject *object, GParamSpec *pspec, gpointer user_data)

Parameters lined up on separate lines

@@ +434,1 @@
   /* Try to lookup the new one */

This comment doesn't really make sense without the old /* Get rid of the old one */ comment that it was paired with, I'd just drop it too.

@@ +435,3 @@
   theme_node = st_widget_peek_theme_node (ST_WIDGET (icon));
   if (theme_node == NULL)
     return;

Should remove priv->pending_texture as the first thing... doesn't make sense to keep loading it if the theme node vanished.

@@ +441,3 @@
+      if (priv->opacity_handler_id)
+        {
+          g_signal_handler_disconnect (priv->pending_texture, priv->opacity_handler_id);

You don't need this - clutter_actor_destroy() will disconnect all signal handlers

(In reply to comment #7)
> Review of attachment 175658 [details] [review]:
> 
> @@ +441,3 @@
> +      if (priv->opacity_handler_id)
> +        {
> +          g_signal_handler_disconnect (priv->pending_texture,
> priv->opacity_handler_id);
> 
> You don't need this - clutter_actor_destroy() will disconnect all signal
> handlers

But I'm not calling clutter_actor_destroy anywhere (of course, because I don't want to destroy the actor I have just loaded), and I don't want that setting opacity on the actor from outside breaks everything.

(In reply to comment #8)
> (In reply to comment #7)
> > Review of attachment 175658 [details] [review] [details]:
> > 
> > @@ +441,3 @@
> > +      if (priv->opacity_handler_id)
> > +        {
> > +          g_signal_handler_disconnect (priv->pending_texture,
> > priv->opacity_handler_id);
> > 
> > You don't need this - clutter_actor_destroy() will disconnect all signal
> > handlers
> 
> But I'm not calling clutter_actor_destroy anywhere (of course, because I don't
> want to destroy the actor I have just loaded), and I don't want that setting
> opacity on the actor from outside breaks everything.

The disconnection in opacity_changed_cb() is of course necessary, but the disconnection in st_icon_update() when you are destroying to overwritten pending texture doesn't look necessary to me

Created attachment 176137 [details] [review]
StIcon: don't show the icon until fully loaded

When updating the texture, keep the old one until the new one is
loaded.

Created attachment 176138 [details] [review]
StIcon: remove implementation of :map and :unmap

They're not needed since Clutter 1.5.8

Created attachment 176139 [details] [review]
StIcon: fill the structure corretly in _init

GSlice already fills with zeros when allocating, but we need to
set the shadow_material appropriately.

Review of attachment 176137 [details] [review]:

Looks good

Review of attachment 176138 [details] [review]:

Looks good (I might say "since Clutter 1.5.8; Clutter now internally keeps a list of children and uses that in the default map and unmap implementations." in the commit message)

Review of attachment 176139 [details] [review]:

perhaps 'set the shadow_material _field_ appropriately'

Pushed with commit message changes.