Re: Comments on toolbar

[Owen Taylor <otaylor redhat com>]

On Tue, 2003-07-01 at 16:40, Soeren Sandmann wrote:
> Owen Taylor <otaylor redhat com> writes:
> 
> I'm attaching a patch that fixes the things below plus a bug pointed
> out by Morten Welinder on IRC.
> 
> > > There is a fairly extensive ChangeLog in libegg. Do we want that
> > > somewhere in gtk+?
> > 
> > What I've done on other large merges is to write a fairly elaborate
> > ChangeLog entry for GTK+ describing what is new *for GTK+*, but
> > not try to go into the details of the a=>b=>c work that happened
> > in the extra module. If someone wants that, they can go look
> > in libegg CVS.
> 
> The attached patch changes the ChangeLog entry from the merge to be
> more elaborate. Is it bad form to change an existing ChangeLog entry?

No, it's fine.

> > > Also using the "label" property for two conceptually
> > > different things sounds dubious to me.
>
> > Don't quite understand what you mean here. Are you saying
> > that you don't think it would be broken if the
> > "label" property affected the icon as well as the label?
> 
> No, just that in the GtkButton case
> 
>    - when use_underline is TRUE label is an identier that is used as
>      a key in an internal table of stock items.
> 
>    - when use_underline is FALSE label is text that is actually shown
>      to the user.
> 
> That's conceptually two different things.

Read the GtkButton code again. use_underline is different from
use_stock.

> > Maybe we should just make the no argument constructor be
> > 
> >  gtk_tool_button_new (const char *label
> >                       GdkPixbuf  *icon);
> > 
> > and allow NULL for both? The empty constructor for GtkButton has
> > to do with the original "a button is just a container" philosophy
> > which doesn't hold for GtkToolButton.
> 
> Good idea. Attached patch has this.

I really didn't mean literally GdkPixbuf *icon if we don't offer
a GdkPixbuf API elsewhere. With the current API, GtkWidget *icon_widget
seems most sensible.

> > I think TAB_FORWARD definitely should have a reversed since for
> > RTL. That it doesn't for gtkcontainer is very much a bug,.
> 
> I have done this for gtktoolbar in the attached patch. If the patch
> is applied, I'll close 116301 and open a new one about RTL focusing
> in gtkcontainer.c and elsewhere.

Hmm, the handling in your patch is I think not quite what I would
expect - the double flip of the list is perhaps confusing? 
Also, should TAB_BACKWARD be special cased along with TAB_FORWARD.
And does focus() handle RTL correctly now?

> > Is there any reason to have this property at all, since the only
> > conceivable us case would be for stock ID's, and you have a separate
> > stock-id property?
> 
> If we removed it, the toolbar would have to act like use_underline was
> always TRUE, because of the overflow menu which needs the
> mnemonics. That might be confusing, but on the other hand, with it
> defaulting to FALSE, we might see applications that do not have
> mnemonics on the overflow menu items.

Certainly it can't always be true. It doesn't seem awful to me if
the overflow menu doesn't have mnemonics, but I suppose it's sort
of a nice touch.

Other than the comments above, your patch looks fine to me.

Regards,
					Owen