Created attachment 32716 [details] [review]
gtk-image-icon-name.patch

Initial version of patch.

Looks good in general. A few comments:

+                                                        P_("The name of the
icon from the current icon theme"),

Sounds to me as if the name would depend on the theme. Maybe omit the "current",
and add a doc comment explaining that the actual image is always looked up in
the current icon theme ?

All doc comments need a "Since: 2.6" line.

+ * Return value: a new #GtkImage displaying the stock icon

s/stock icon/themed icon/ here, or just omit "stock"

      if (!gtk_icon_size_lookup_for_settings (settings,
+					      image->icon_size,
+					      &width, &height))
+	{
+	  g_warning ("Invalid icon size %d\n", image->icon_size);
+	  width = height = 24;
+	}

gtkiconfactory.c:render_icon_name_pixbuf() has more complicated code
to handle an invalid icon size. Maybe it is worth doing something
similar here. Actually, I wonder if it would be possible to reuse more
of the icon set code here, by making the icon name case use an icon set.
I guess you would still need to listen for theme changes, so it may not
be worth it...

I tried it now, and it works well. 
There is the question of whether it would be more convenient to have a pixel_size 
parameter instead of the symbolic icon_size. But what the patch has now fits
well with the rest of the GtkImage api. And we can always add a
::icon-pixel-size property which overrides the symbolic icon_size if it is set,
if it turns out to be desirable.

Thanks for the comments.  I'll look at making those documentation changes and do
a new version of the patch.

Switching to use GtkIconSet to do the rendering shouldn't be too difficult (just
create an icon name GtkIconSource, and add it to a GtkIconSet).  The only issue
with that is that it doesn't have a way to render an icon at a particular pixel
size, which could be a problem in implementing an icon_pixel_size property.

One other issue that I just thought of is that the code needs to reload the icon
if the GTK theme changes too, since the pixel size that a symbolic GtkIconSize
refers to might change.

Given the fact that GtkIconSet doesn't do the theme change updating on its own,
I don't think that switching to GtkIconSet is a priority. You are correct to
point out that we need to reload icon sets on theme change, since the sizes may
change. That also holds for GTK_IMAGE_ICON_SET and GTK_IMAGE_STOCK, btw, and
is a bug of GtkImage.

I think that the cleasest way to support pixel sizes may be to register
"well-known" sizes like

12x12 
16x16
20x20
22x22
24x24
32x32
36x36
48x48
64x64
72x72
96x96
128x128
192x192

(this is the union of all sizes from Bluecurve, gnome, hicolor)

Created attachment 32810 [details] [review]
gtk-image-icon-name-v2.patch

* Fixes the documentation issues.
* Picks the closest size to 48 as a fallback if icon_size == -1, like
GtkIconSet
  does
* Adds a GtkWidget::style_set handler that queues a resize if the storage type
  is stock, icon_set or icon_name.  Also releases the cached pixmap if it is
  a named icon.

Created attachment 32867 [details] [review]
new version

This version adds a ::pixel-size property which overrides the ::icon-size
property  for named icons, if it is set to a value != -1. It is unfortunately
not trivial to make it override ::icon-size for stock and icon-set images as
well

Created attachment 32868 [details]
a test 

Here is a small test app, which can be used to test the theming behaviour of
the various image types. Note that it also contains a test for image dnd api
which isn't committed yet...

Comment on attachment 32867 [details] [review]
new version

>Index: gtk/gtkimage.h
...
>-  /* Only used with GTK_IMAGE_STOCK, GTK_IMAGE_ICON_SET */
>+  /* Only used with GTK_IMAGE_STOCK, GTK_IMAGE_ICON_SET, GTK_IMAGE_ICON_NAME */
>   GtkIconSize icon_size;
>+  gint pixel_size;
> };

Won't adding a struct member like this break binary compatibility?

Yes, I'll move it to a private struct before committing this.

I just noticed one other thing.  Your latest patch is missing the style_set
handler I added in the previous patch.  Without it, the widget won't resize if a
GtkIconSize changes its pixel size.

2004-10-25  Matthias Clasen  <mclasen@redhat.com>

	* gtk/gtkimage.h: 
	* gtk/gtkimage.c (gtk_image_new_from_icon_name)
	(gtk_image_set_from_icon_name, gtk_image_get_icon_name)
	(gtk_image_set_pixel_size, gtk_image_get_pixel_size):  Add a
	new type GTK_IMAGE_ICON_NAME for named icons, update the size
	and content of stock, icon set and named icon images upon style
	changes, and allow to set a fixed pixel size for named icon
	images.  (#155688, James Henstridge)

	* tests/testimage.c: Test application for theming behaviour of
	different image types and for image dnd.

Going to reopen this since I had some of minor patch review comments
that I didn't get around to writing up before it was committed:

+    case PROP_PIXEL_SIZE:
+      priv->pixel_size = g_value_get_int (value);
+      break;

This doesn't properly queue a resize, etc. I'd suggest calling
gtk_image_set_pixel_size().

[ in gtk_image_set_from_icon_name() ]
+  /* in case stock_id == image->data.stock.stock_id */
+  new_name = g_strdup (icon_name);

Comment needs updating. 

+ void
+gtk_image_get_icon_name  (GtkImage        *image,
+                         gchar          **icon_name,
+                         GtkIconSize     *size)

This inherits problems from gtk_image_get_stock() ... the
gchar **icon_name definitely should be G_CONST_RETURN gchar **icon_name,
but beyond this, would it be better to have gtk_image_get_icon_name() 
and gtk_image_get_icon_size() separately? 

+static void
+gtk_image_reset_icon_theme_change_cb (GtkImage *image)

Name of this is a bit weird, but I think the logic is wrong as well. 
I don't see any reason why an unmapped or even unrealized widget
doesn't need to listen to a theme change.

The name => pixbuf mapping can change if the GtkScreen changes, or
if the GtkIconTheme for the GtkScreen changes. But as it turns out,
GTK+ fakes a theme change on icon theme change, so you really
don't need to listen for GtkIconTheme::changed.

(gtkicontheme.c:do_theme_change() calls gtk_rc_reset_styles())

So, what really is needed is a default handler for screen_changed
that unsets image->data.name.pixbuf and queues a resize. (Not a 
redraw like the current code ... you aren't guaranteed that a
different theme won't have a different resulting size for a 
particular icon/pixel_size combination.). 

+ * Sets the pixel size to use for named icons. If the pixel size is set
+ * to a value != -1, it is used instead of the ::icon-size property for
+ * images of type %GTK_IMAGE_ICON_NAME.

The ::icon-size notation here has traditionally only been used for
signals, not for properties. Maybe we should use it for properties as
well, I don't know. But it's not traditional. I'd probably not refer
to properties at all here and say 'the icon size set from 
gtk_image_set_from_icon_name' or something like that.

+ * Returns: the value of the :pixel-size property.

Here there is only one ':' :-). Again, I think I'd try to avoid mention
of properties.

Should be fixed.