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:
> 
> > I don't think really anything below should block comitting the code
> > to CVS, except for the padding problem in GtkToolbarClass, which
> > is trivial to fix.
> 
> I have committed the new toolbar to CVS. Anything that I don't comment
> on below I just fixed. 
> 
> 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.

> > - Would gtk_tool_item_unset_proxy_menu_item(ID) be useful?
> 
> You can do set_proxy_menu_item (..., NULL);

Currently set_proxy_menu_item ("my-id", NULL) unconditionally 
NULLs out the current proxy item. What I was asking about was
a function that did:

 if (get_proxy_menu_item ("my-id"))
   set_proxy_menu_item ("my-id", NULL);

We could just make set_proxy_menu_item ("my-id", NULL); do that, but
I don't think that's consistent with the behavior of
set_proxy_men_item ("my-id", widget).

In the end, probably not worth the extra bloat in the API.

> > GtkToolButton API Comments
> > ==========================
> > - The ::icon-set property for GtkToolButton seems a bit
> >   unusual in comparison with other GTK+ API's. The only
> >   real place we have an icon-set API is GtkImage. I'm
> >   not really sure when you'd use that.
> 
> The idea is to add a custom icon that change size according to the
> ToolBar::icon_size setting. 

The question to me is whether anybody would actually go through
the effort to create an IconSet to do this. GtkIconSet is 
not a particularly convenient API to use.

(One thing which we need to sort out once it lands is the
interaction of this with named icons)

> > - GtkButton has use_stock rather than stock_id as the 
> >   property. Should we be consistent with that? (The
> >   nice thing about what GtkToolButton has is that 
> >   you can set the label and the stock_id and get 
> >   a stock button with a different label, something
> >   that has frequently been requested for GtkButton)
> 
> I think that is enough of a reason to be inconsistent with
> GtkButton. 

OK, fine with me.

> 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?
Perhaps, though consistency with GtkButton counts for something,
and we can't change that for GtkButton.

> > - gtk_tool_button_set_icon/label_widget() should probably
> >   ref/sink the widgets?
> 
> Done (review would be appreciated)

Looks good.

> > - Should there be _with_label()/_with_mnemonic() constructors
> >   for the three button types to match GtkButton/GtkMenuItem?
> 
> Adding _add_with_mnemonic() will probably just lead to questions like
> "why don't I get mnemonics when I used _add_with_mnemonic()?". We
> never show mnemonics on a toolbar button.
> 
> Adding _with_label() may be a good idea, but shouldn't there then also
> be "_with_icon" and "_with_icon_and_label"?
> 
> I have filed bug 116295 about that

Oh, for typesafe properties in C so we could just use g_object_new()
and forget about constructor variants...

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.

> > - Maybe combine focus_home/focus_end into a signal boolean-parametered
> >   (or enum parametered? Do we have a suitable enumeration) signal.
> 
> I combined them into gtk_toolbar_focus_ends () with a boolean
> parameter "home". I don't like the name focus_ends(), but I can't
> think of a better one.

focus_start_end (gboolean focus_start)? hmm. Maybe we should just
go back to the separate signals.

> > Separators and empty items
> > ==========================
> > - I don't see any reaosn to propagate "empty item is a separator
> >   into GtkToolbar" - I'd just draw GtkSeparatorToolbarItem as
> >   a separator ... could you then move the expose code for separators
> >   into the GtkSeparatorToolbarItem?
> 
> You mean replace 
> 
>         if (!GTK_BIN (item)->child) 
> 
> with
>         if (GTK_IS_SEPARATOR_TOOL_ITEM (item))
> 
> ?

Well, yes, that, if you need to implement code in GtkToolbar. It
seems that a lot of the code in GtkToolbar, GtkToolItem could
move into GtkSeparatorToolItem.

> Filed as 116299 because I'm not sure what you mean. It might be a
> problem that you need access to a toolbar to get at the "space_size"
> property.

Well, not different than other stuff we do - shouldbe easy enough to
write a get_space_size() convenience function that looks at the
parent and use that internally in GtkSeparatorToolItem.,

> > Comments about relationship between properties for GtkToolButton
> > ================================================================

I'll look at this/respond to it separately, since this mail is 
getting unmanageable long.

> > Comments about focusing code in gtktoolbar.c
> > ============================================
> > - gtk_toolbar_list_child_in_focus_order() doesnt' seem to
> >   properly handle RTL - DIR_TAB_FORWARD/DIR_DOWN/DIR_TAB_BACKWARD/
> >   DIR_UP, but not DIR_RIGHT. So, in focus_home/focus_end
> >   you should just use DIR_TAB_FORWARD/DIR_TAB_BACKWARD, and
> >   list_children_in_focus_order() will take care of the flipping.
> >   (or flip yourself and use DIR_RIGHT/DIR_LEFT)
> 
> I am a little confused about what is actually right here. Suppose we
> have a toolbar with three child widget, a, b and c. In LTR they are
> displayed like this:
> 
>           a b c
> 
> When you TAB_FORWARD into these, widget a should be focused. When the
> widget sees LEFT and RIGHT you should move left and right.
> 
> In RTL they are displayed like this:
> 
>          c b a
> 
> How should key directions be interpreted? The way it currently works:
> 
>         TAB_FORWARD forcuses c
> 
>         LEFT moves to the left (from b to c)
>         
>         RIGHT moves to the right (from c to b) 
> 
>         HOME moves to a
> 
>         END moves to c
> 
> This seems right to me, except for TAB_FORWARD which I would think
> should go to a. That is not how everything else works though, so I
> didn't do that for the toolbar either. In fact the focus handler in
> gtkcontainer.c does not consider RTL at all.

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 filed this as bug 116301.
> 
> > - gtk_toolbar_move_focus()/gtk_toolbar_focus() don't properly 
> >   handle internal focus states? You need to first try moving the 
> >   focus on the current widget before going to the next
> 
> Well, move_focus() shouldn't handle internal focus states, because at
> the time it is called the keystroke will have propagated up through
> all the child widgets.

Hmm. It's a bit of a messy situation, but I think it does
need to handle internal focus states. If you have a widget inside
the toolbar with internal focus states, if you hit:

 Tab - widget after toolbar is focused
 Control-Tab - next widget is focused

Since Tab is handled at the toplevel and propagated downwards

So, Control-Tab needs to cycle internal focus states before going
to the next widget on the toolbar.

(The usage of Tab and Control-Tab here seem exactly backwards
from what happens in say, GtkNotebook, but I guess this issue
has been hashed over enough. I think also that bad things
will happen if you have a GtkNotebook inside a GtkToolbar, but
then again, if you put a GtkNotebook inside a Toolbar, you
get what you deserve.)

> > - What is the difference bewteen move_focus() and focus()
> >   [ comments distinctly useful if there is a reason ]
> 
> move_focus() is a keybinding handler, while focus() is the usual focus
> method. move_focus() handles arrow keys and Ctrl TAB to move inside
> the toolbar while focus() is the usual focus handler.
> 
> I added comments to the code.

The comments you added our quite helpful. Might not hurt
to have a comment for move_focus() as well as one for focus()
to allow compare-and-contrast.

> > - For new widgets, we should use instance-private-data for
> >   all fields. The way we are doing this is to have a 
> >   ->priv field in the instance structure to optimize private
> >   data lookups.
> 
> Filed as bug 116304. Note that the "button" field of GtkToolButton is
> used in all subclasses. There may be other fields that are used in
> more than one place. New API "get_button" may be useful.

One reason for avoiding public members is precisely so that we
have more control over what is part of the API and not. If we
add a public member, then anybody can use it and we can never
change what we do with it. An accessor can either be 
_gtk_tool_button_get_button() gtk_tool_button_get_button() and
there is a lot of flexibility with what we do behind the scenes.

> > - Should copy GtkToolButton::use_underline doc text from GtkButton,
> >   the text there is considerably better.
> 
> The text from GtkButton would be misleading, because use_underline on
> a ToolButton means more or less "ignore underlines unless followed by
> another underline". Bug 116302.

OK, then the property doc *definitely* needs improvement, since:

 "Interpret underlines in the item label"

doesn't say that to me :-). 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?

> > - handling of bin->child is gratiutiously different in
> >   gtk_tool_[item/button]_size_[request/allocate]
> 
> Do you mean 
> 
>    child = GTK_BIN (widget)->child;
>    [use child]
> 
> vs
> 
>    bin = GTK_BIN (widget)
>    [use bin->child]
> 
> ?  I have changed it to be child = GTK_BIN (widget)->bin everywhere

Yes, that's what I meant. (This was the "Minor code suggestions"
section...)

> > - Should make the elide_underscores() function a private function
> >   somewhere that is shared between the two source files that use it.
> 
> Filed as 116307 because I'm not sure what the function should be called

Doesn't matter much ... _gtk_tool_button_elide_underscores() or
something would be fine.

> > Comments that could be useful in the code
> > =========================================
> > - Need comments in the source code about the handling
> >   of xthickness/ythickness around children of GtkToolItem;
> >   we discussed this, but even now, I dont' remember the
> >   rational.
> 
> Filed as 116304. I don't think we actually came to any conclusion when
> we discussed this.

116306 actually. I put a relevant IRC log portion onto the bug;
doesn't really answer anything but does discuss the considerations
some.

Regards,
					Owen