Re: properties for GtkImage

[Owen Taylor <otaylor redhat com>]

Havoc Pennington <hp redhat com> writes:

> Index: gtk/gtkcontainer.c
> ===================================================================
> RCS file: /cvs/gnome/gtk+/gtk/gtkcontainer.c,v
> retrieving revision 1.86
> diff -u -p -u -r1.86 gtkcontainer.c
> --- gtk/gtkcontainer.c	2001/08/23 23:38:32	1.86
> +++ gtk/gtkcontainer.c	2001/08/26 03:55:16
> @@ -858,7 +858,14 @@ gtk_container_add (GtkContainer *contain
>    if (widget->parent != NULL)
>      {
>        g_warning ("Attempting to add a widget with type %s to a container of "
> -                 "type %s, but the widget is already inside a container of type %s",
> +                 "type %s, but the widget is already inside a container of type %s, "
> +                 "what you want to do is:\n"
> +                 "   g_object_ref (G_OBJECT (child));\n",
> +                 "   gtk_container_remove (GTK_CONTAINER (old_container),\n"
> +                 "                         child);\n"
> +                 "   gtk_container_add (GTK_CONTAINER (new_container),\n"
> +                 "                      child);\n"
> +                 "   g_object_unref (G_OBJECT (child));",
>                   g_type_name (G_OBJECT_TYPE (widget)),
>                   g_type_name (G_OBJECT_TYPE (container)),
>                   g_type_name (G_OBJECT_TYPE (widget->parent)));

As well intentioned as this is, well, 90% of the time when you get this message
it's just a stupid cut-and-paste bug and you didn't actually mean to reparent
the widget. Perhaps an URL to the FAQ would be OK.... 

> +  g_object_class_install_property (gobject_class,
> +                                   PROP_FILE,
> +                                   g_param_spec_string ("file",
> +                                                        _("Filename"),
> +                                                        _("Filename to load and siplay."),
                                                                                   ^

> +static GtkIconSize
> +get_current_size (GtkImage *image)
> +{
> +  GtkIconSize size;
> +        
> +  if (image->storage_type == GTK_IMAGE_STOCK)
> +    size = image->data.stock.size;
> +  else if (image->storage_type == GTK_IMAGE_ICON_SET)
> +    size = image->data.icon_set.size;
> +  else
> +    size = DEFAULT_ICON_SIZE;
> +
> +  return size;
> +}

I really think it makes sense to separate out the icon_size out of union and
just make it stick around. The current dependence is a little funky and doesn't
seem worth the small memory saving.

> @@ -1070,6 +1381,9 @@ gtk_image_expose (GtkWidget      *widget
>  static void
>  gtk_image_clear (GtkImage *image)
>  {
> +  if (image->storage_type != GTK_IMAGE_EMPTY)
> +    g_object_notify (G_OBJECT (image), "storage_type");

I don't think this works ... you need to notify _after_ you've finished
going to the new state, otherwise you get an off-by-one error. 

freeze/thaw notifies is one way to achieve this. freeze/thaw around
the notification of multiple properties may be slightly more efficient
anyways. It looks like to me that when switching the type, you'll end
up with two notifies of "storage_type".

Regards,
                                        Owen