Re: Comments on toolbar

[Owen Taylor <otaylor redhat com>]

On Sun, 2003-06-29 at 21:00, Soeren Sandmann wrote:
> Owen Taylor <otaylor redhat com> writes:

> > Comments about relationship between properties for GtkToolButton

[....]

> This is a thing that I did totally different since your last
> comments.

OK, a point for me for being consistent in the things I comment
on, a point against me for not remembering that I had commented
on it earlier or noticing that you had fixed it.

> The way it is intended to work currently is like your first
> possibility with the exception that "icon_widget" and "icon_set" are
> mutually exclusive.
> 
> Here is a quote from the libegg ChangeLog:
> 
>         Properties are now interpreted like this:
> 
>               - icon_set and icon_widget are mutually exclusive.
> 
>               - if the tool button has an icon_set or an icon_widget,
>                 these will be used as icons. Otherwise, if the tool
>                 button has a stock id, the corresponding stock icon
>                 will be used. Otherwise, the tool button will not have
>                 an icon.
> 
>               - if the tool button has a label widget then that widget
>                 will be used as label. Otherwise, if the tool button
>                 has a label text, that text will be used as
>                 label. Otherwise, if the toolbutton has a stock id,
>                 the corresponding text will be used as
>                 label. Otherwise, the toolbutton will have an empty
>                 label.
>
>               - the use_underline property means that underlines in
>                 the label property are "elided".

Having this type of information commented in the code would be very
useful. (Perhaps in the docs, though it's hard to fit it into the
property doc strings. You could have hings 
"Overrides 'label'.t" in the 

> I don't remember if there was a reason I made the icon_set and
> icon_widget properties mutually exclusive.

That was probably the point that confused me in looking at your code.
It just doesn't make sense to me to have them be mutually exclusive
but have label and label_widget not mutually exclusive... they
seem very much parallel cases to me.

> > - Once the above is sorted out (and best documented in comments),
> >   various functions need careful review:
> 
> It should probably be documented in the API documentation.
> 
> >     gtk_tool_button_construct_contents()
> >     gtk_tool_button_create_menu_proxy()
> >     gtk_toggle_tool_button_create_menu_proxy()
> 
> These functions are supposed to follow the scheme above, but of course
> I may have made mistakes. A review should probably be done by someone
> else than me.

In a review of the code, the overrides in the creation function and the
property notifications all seem to work properly. But, of course,
I did find a few other things to comment on :-)

===
  if (GTK_BIN (button->button)->child)
    {
      gtk_container_remove (GTK_CONTAINER (button->button),
                            GTK_BIN (button->button)->child);
    }
===

* It would be better to use gtk_widget_destroy() here than  
gtk_container_remove(). Generally, we want to try to ensure that
destroy() is called on dying widgets whenever possible, since
it's the only thing that will break certain reference count cycles.

* I don't understand the handling of icon_widget in 
gtk_tool_button_construct_contents() ... it appears that in 
many cases you build a clone of the image widget, rather than
the image widget itself. This is not what people are going to
expect to happen and seems unnecessary.

* The blurb for the 'use_underline' property should certainly
mention that the mnemonic *is* used for the overflow menu, even
if it isn't used for the tool items.

* I'm wondering if some sort of gtk_image_clone() API would make
sense to allow copying images generically and avoid the switch
over storage type you have in create_proxy_menu(). My main
concern about adding that is that I can't think of any other
conceivable use case.

* Idle thought: the tool item and menu item are never displayed
at the same time, so theoretically, the image and label widgets
*could* be used directly in the menu instead of being cloned.
But that would require significant complication of the proxy
menu API.

Regards,
					Owen