Created attachment 175105 [details] [review]
st-icon: Add support for -st-shadow property

Add a drop shadow to the icon texture if the -st-shadow property is
specified.

This will be used by the drop target to remove favorites.

Review of attachment 175105 [details] [review]:

Isn't this racy? If the icon image has to be loaded asynchronously, then wouldn't it be possible that we create the shadow material before the icon is loaded, and then not update it on load?

Created attachment 175162 [details] [review]
st-icon: Add support for -st-shadow property

Update shadow when the underlying pixbuf changes.

Review of attachment 175162 [details] [review]:

Two style points (ok to commit with those fixed), and one discussion of efficiency that isn't a blocker

::: src/st/st-icon.c
@@ +367,3 @@
+
+static void
+update_shadow_material (StIcon *icon)

This matches our normal conventions of not namespacing static functions but doesn't match the surrounding code - st_icon_update_size() vs. update_shadow_material() - which I generally consider more important.

@@ +440,3 @@
   if (priv->icon_texture)
+    {
+      update_shadow_material (icon);

Shadowing the 1x1 empty Cogl texture that the material starts off with is actually pretty of expensive ... the set up we have to do to read a texture is extensive even if we don't read many bytes. You could leave it lazy as it was before and make on_pixbuf_change() only null out the cached shadow material but async load completions usually run *after* we've painted a frame, so doesn't really help. Other than the gross hack of saying that 1x1 images don't have a shadow, this may not be fixable until we git rid of the ClutterTexture actor and have StTextureCache know about StIcon directly.

[ as opposed to lazy at paint time some advantages to computing the shadows before the frame - see the discussion at the end of http://bugzilla.clutter-project.org/show_bug.cgi?id=2414 ]

@@ +442,3 @@
+      update_shadow_material (icon);
+      clutter_actor_set_parent (priv->icon_texture, CLUTTER_ACTOR (icon));
+      g_signal_connect (priv->icon_texture, "pixbuf-change",

Might be a good to add a note here that pixbuf-changed is a misnomer for texture-changed so the reader isn't confused (see comment in clutter-texture.c:

  /* rename signal */
  g_signal_emit (texture, texture_signals[PIXBUF_CHANGE], 0);

)

Attachment 175162 [details] pushed as 4f7a288 - st-icon: Add support for -st-shadow property