Re: GtkImage changes

[Darin Adler <darin eazel com>]

on 6/28/00 3:51 PM, Havoc Pennington at hp@redhat.com wrote:

> void       gtk_image_get_pixmap   (GtkImage         *image,
>                                    GdkPixmap       **pixmap,
>                                    GdkBitmap       **mask);
> void       gtk_image_get_image    (GtkImage         *image,
>                                    GdkImage        **gdk_image,
>                                    GdkBitmap       **mask);
> GdkPixbuf* gtk_image_get_pixbuf   (GtkImage         *image);
> void       gtk_image_get_stock    (GtkImage         *image,
>                                    gchar           **stock_id,
>                                    GtkIconSizeType  *size);
> void       gtk_image_get_icon_set (GtkImage         *image,
>                                    GtkIconSet      **icon_set,
>                                    GtkIconSizeType  *size);

I see no accessor for getting at the representation_type. So you'd go
directly at the struct? I'm not sure under what circumstances people want to
get at the old value of the image, so this may be no big deal. It does seem
that if you're going directly to the struct for the representation_type you
could go into the union directly too. I think that adding an accessor for
representation_type would make it clear that it's normal to get the field
but not normal to set it.

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.

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.

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

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.

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 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 :-)

> 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

> 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 :-)

    -- Darin