Re: revised image prop, icon patch

[Owen Taylor <otaylor redhat com>]

Looks basically fine - a couple of small comments below.

The one major concern I have is about including the ::icon_list
property ... I'm worried that it will pollute the namespace for
when we have some better way of handling icon list,s and isn't
all that useful.  (You mention in your comments the possibility
of connecting to icon_list::notify .. that seems to be the
main use.)

Regards,
                                        Owen

Havoc Pennington <hp redhat com> writes:

> +    case PROP_MASK:
> +      /* If current storage type doesn't support a mask,
> +       * we switch to one that does. This allows setting
> +       * mask first then the pixmap or image.
> +       */
> +      if (image->storage_type == GTK_IMAGE_PIXMAP)
> +        gtk_image_set_from_pixmap (image,
> +                                   image->data.pixmap.pixmap,
> +                                   g_value_get_object (value));
> +      else if (image->storage_type == GTK_IMAGE_IMAGE)
> +        gtk_image_set_from_image (image,
> +                                  image->data.image.image,
> +                                  g_value_get_object (value));
> +      else
> +        gtk_image_set_from_pixmap (image,
> +                                   NULL,
> +                                   g_value_get_object (value));
> +      break;

Is this still necessary? It means that you can have a pixmap storage
type with a null pixmap, which seems like a bad idea to me. Avoiding
this for icon-size is why I suggested the storage change, really.

I guess the downside is that if you had pixmap/mask and switched
to pixbuf without freeing the mask, the mask wouldnt' be freed.
I don't think this really matters ... people won't be changing
the storage type of GtkImage widgets at all often, if ever.

> +    case PROP_ICON_SIZE:
> +      /* If current storage type doesn't support icon size,
> +       * we switch to one that does. This allows setting
> +       * size first then the stock ID or icon set
> +       */
> +      if (image->storage_type == GTK_IMAGE_STOCK)
> +        gtk_image_set_from_stock (image,
> +                                  image->data.stock.stock_id,
> +                                  g_value_get_int (value));
> +      else if (image->storage_type == GTK_IMAGE_ICON_SET)
> +        gtk_image_set_from_icon_set (image,
> +                                     image->data.icon_set.icon_set,
> +                                     g_value_get_int (value));
> +      else
> +        gtk_image_set_from_stock (image,
> +                                  GTK_STOCK_MISSING_IMAGE,
> +                                  g_value_get_int (value));
> +      break;

Ditto here.

> @@ -427,8 +445,21 @@ gtk_window_class_init (GtkWindowClass *k
>                                                           FALSE,
>  							 G_PARAM_READWRITE));
>  
> -  /* Style props are set or not */
> +  g_object_class_install_property (gobject_class,
> +                                   PROP_ICON_LIST,
> +                                   g_param_spec_pointer ("icon_list",
> +							 _("Icon list"),
> +							 _("List of icons to represent the window"),
> +							 G_PARAM_READWRITE));
>  

I think we should avoid this ... we'll have better ways of handling
arrays later and this just polluts the namespace.

> @@ -651,6 +682,20 @@ gtk_window_set_property (GObject      *o
>      case PROP_DESTROY_WITH_PARENT:
>        gtk_window_set_destroy_with_parent (window, g_value_get_boolean (value));
>        break;
> +    case PROP_ICON_LIST:
> +      gtk_window_set_icon_list (window, g_value_get_pointer (value));
> +      break;
> +    case PROP_ICON:
> +      {
 > +        GdkPixbuf *pixbuf;
> +        pixbuf = g_value_get_object (value);
> +        if (pixbuf)
> +          gtk_window_set_icon (window, GDK_PIXBUF (pixbuf));
> +        else
> +          gtk_window_set_icon (window, NULL);

This looks like an historical hold-over -

 gtk_window_set_icon (window, pixbuf);

will, of course, work fine for both cases.

> +/**
> + * gtk_window_get_default_icon_list:
> + * 
> + * Gets the value set by gtk_window_set_default_icon_list().
> + * The list is a copy and should be freed with g_list_free(),
> + * but the pixbufs in the list have not had their reference count
> + * incremented.
> + * 
> + * Return value: default icon list 

You need to mention that you are copying the list here, since
it isn't obvious. Probably the same for gtk_window_get_icon_list().