Re: GtkImage changes

[Havoc Pennington <hp redhat com>]

Darin Adler <darin@eazel.com> writes: 
> I see no accessor for getting at the representation_type.

representation_type is supposed to be internal (as are all those *Data
structs in the header). Sadly there is no way to enforce that.

The idea is, you can only _get_ a thing that you _set_. If you set a
pixbuf, you can get a pixbuf. If you set a pixmap, you can get a
pixmap. If you set a pixmap and try to get a pixbuf, we
g_return_if_fail.
 
> It's particularly inconvenient to use the to save and restore the image from
> a GtkImage with an arbitrary representation, but perhaps that's not a common
> thing to want to do.
> 

What do you mean by "save and restore"? To disk? Why would you do that?

> Here's another criticism of the way the get calls are defined. Even if you
> use set_from_image, get_image isn't guaranteed to work, because the
> representation_type is set to GTK_IMAGE_EMPTY if the image is NULL. Same is
> true for pixmap, pixbuf, stock, and icon_set as well. So the setters and
> getters are assymetric in an inconvenient way.
> 

True. So if the rep_type is GTK_IMAGE_EMPTY, all the getters should
work but return NULL. Good point.

> It's strange that _get_stock dup's the stock_id, but none of the other
> getters ref or dup their results.
>

This is the standard GTK way (though it's currently under discussion),
that strings are dup'd and other stuff isn't.
 
> It might be handy to have "" for the stock ID mean no icon for cases where
> the icon's name is read from some configuration file. Currently NULL means
> no icon, but "" is passed on to gtk_widget_get_icon.
>

At that point gtk_widget_get_icon() will return NULL, then you end up
with the same effect.
 
> I'd like to see more use of g_return_if_fail in these functions. Many of the
> new functions don't validate their parameters and it's an easy thing to add.
> 

I'll look over it. In most cases NULL is allowed though.

> I also have a strong preference for classes where most fields are private,
> but for a subclassable GtkObject that would mean a separate structure and a
> pointer and I know that's not the Gtk standard so there's no need to cater
> to my taste :-)
>

Those fields _are_ private, it just isn't enforced. Except that I get
to gratuitously renamed them. ;-) Hopefully that messy union thing
discourages direct access!
 
I hated the way people would do this with GnomePixmap:

GnomePixmap *pix = gnome_pixmap_new_from_file ("blah");
GdkPixmap *pixmap = pix->pixmap;
/* go on to ignore GnomePixmap and use the GdkPixmap */

Hopefully GdkPixbuf will remove the motivation to do this.

> > void
> > gtk_image_set_from_file   (GtkImage    *image,
> >                            const gchar *filename)
> 
> I notice that this function clears the image if you pass NULL for the file
> name, rather than doing a return_if_fail. Any particular reason? This does
> cause an inconsistency
> 

What's the inconsistency you mean? I guess there's an argument that
while you might intend to unset the pixbuf by passing NULL, you
probably don't intend to pass a NULL filename in order to unset.

> > if (image->data.stock.stock_id)
> >   g_free (image->data.stock.stock_id);
> 
> The common "if before g_free" mistake. No need for the if :-)
> 

I tend to do it anyway, it's just to make the intent clear.

Thanks for the comments!

Havoc