Re: GtkImageMenutItem

[Havoc Pennington <hp redhat com>]

Tim Janik <timj gtk org> writes:
> here are a couple comments regarding the newly added
> image menuitem. first, i think the name is a bit odd
> as that menu item can hold any kind of extra widget,
> not only an image.

Yes, but GtkWidgetMenuItem is blatantly a bad name (overlaps with
GtkWidget namespace if nothing else), and it will always contain an
image in practice. I'm open to better ideas.

> +static void
> +gtk_image_menu_item_destroy (GtkObject *object)
> +{
> +  GtkImageMenuItem *image_menu_item;
> +
> +  image_menu_item = GTK_IMAGE_MENU_ITEM (object);  
> +
> +  /* If you change forall to treat the image widget as
> +   * an internal, then you have to destroy the image widget
> +   * here.
> +   */
> +  
> +  (* GTK_OBJECT_CLASS (parent_class)->destroy) (object);
> +}
> 
> that comment is wrong. currently the image is treated as
> a non-internal widget (which is the right thing to do as
> it's supplied by the user):
> 

The comment is correct, though perhaps confusing. The point of the
comment is that the GtkContainer default implementation of destroy
(which we chain to) destroys all non-internal widgets. So we don't
need to do anything in destroy. But if you changed the image widget to
be internal, you would need to destroy it in destroy. I thought people
might suggest this change, so I put the comment as a reminder.

Basically this is an empty destroy, which always annoys Owen too, I
should take it out. ;-) But it's correct for it to be empty.
 
> but since it's not internal, it has to be destroyed
> in ::destroy.

It is, in GtkContainer::destroy which I chain up to.
 
> this shut be menu_item_set_image() since the menu item
> cannot handle multiple images. it should also handle NULL
> to take over the current remove functionality.

You just call gtk_container_remove() to remove. I thought this way
would be more consistent with e.g. gtk_box_pack_start(), which does
not accept NULL. I had set_image() originally.
 
> this is _way_ hackish. first, we don't overload public API in
> derived classes (here, gtk_container_remove), 

I'm not overloading it, I'm overriding it, look again. It does appear
I left off the "static," oops. Anyway, gtk_container_remove() should
work on the image widget, that is the purpose of this override.

> second, it breaks
> pair-ness of gtk_container_add/gtk_container_remove.
> get rid of this and use just gtk_image_menu_item_set_image().

I don't understand what you mean by "breaks pair-ness".

> +struct _GtkImageMenuItem
> +{
> +  GtkMenuItem menu_item;
> +
> +  /*< private >*/
> +  GtkWidget *image;
> +};
> 
> what's the point in /*<private>*/ here? there's no reason
> to make the image widget anymore private that say GtkBin.child.
> 

*shrug* no strong feelings. There's a get_image() accessor, so there's
no reason to make it public, private is my default setting.

Havoc