Re: Comments on toolbar

[Soeren Sandmann <sandmann daimi au dk>]

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+?

> Docs, docs docs
> ===============
> - Need API docs

Indeed. I seem to remember hearing about an Emacs package that should
make it easier to add API documentation. Does anybody know where it
is?

> GtkToolItem API Comments
> ========================
> - What's the use case for "visible_horizontal" "visible-vertical"?

Jody already answered this one

> - Would gtk_tool_item_unset_proxy_menu_item(ID) be useful?

You can do set_proxy_menu_item (..., NULL);

> - Could/should GtkToolItem hook up parent_set to reconfigured?

Yes, good idea. I made it so. It is one less thing for writers of
custom toolitems to worry about.

> 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. 

> - 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. Also using the "label" property for two conceptually
different things sounds dubious to me.

> - gtk_tool_button_set_icon/label_widget() should probably
>   ref/sink the widgets?

Done (review would be appreciated)

> - Should there there be radio_toolbutton_new[_with_stock]_from_widget()?
>   The experience we've had with radio button API's is that
>   the from_widget() API's are vastly more convenient than
>   the ones that take a group. (But GtkRadioMenuItem only
>   has the group functions)

I have added those. It may be a good idea to add
gtk_radio_menu_button_new_from_widget(), so I filed bug 116293 about
that.

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

> GtkToolbar API comments
> =======================
> - Note that with insert_stock() we set a precendent for considering
>   append/prepend superfluous .. should we continue that here?

I have removed them. append() was exactly equivalent to
gtk_container_add() anyway.

> - I think coordiantes for the popup_context_menu() are a 
>   good idea. Button number is *essential*, and probably
>   should be -1 for keynav. (Note need connection to the
>   GtkWidget keynav signal for popping up a context menu.) One question
>   is how positioning works if you pop up menu during keynav.

I have filed this as bug 116296.
     
> - 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.

> Drag and drop API/Bug comments
> ==============================
> - I don't see handling of pack_end in gtktoolbar.c:find_drop_pos();
>   in fact, there seems to be some conceptual difficulty with
>   drag-and-drop editing and pack_end. Perhaps easiest is to have an
>   additional boolean out and let the toolbar editor figure it out.
>   Dragging a pack end item before the first pack end item is
>   a tricky situation (it isn't distinguishable from dragging after
>   the last pack start position), and probably should just be
> impossible.

116297 

> - DND code for GtkToolbar should *not* handle arbitrary targets
>   or it will look like you can drop strings, etc, into the toolbar.
>   We probably need to define a target for dropping into the toolbar.

116298

Filed bugs on these, cc'ing James, because I haven't really looked at
the dnd code.

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

?

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.

> Comments about relationship between properties for GtkToolButton
> ================================================================
> - The relationship between the various properties for GtkToolButton
>   needs to be sorted out in detail to get notification right. There 
>   are many possible valid schemes, and we aren't consistent in
>   what we do in GTK+. But within GtkToolbutton we need to know
>   exactly how it works.
> 
>   I'll present two possible valid possibilties, thought they
>   aren't the only two possibilities.
> 
>   There is the "pure property value" option, where getting
>   a property always returns the last thing you set. What 
>   gets displayed depends on what properties override
>   what other properties. The obvious override sequence is:
> 
>    icon_widget overrides icon_set overrides stock_id
>    label_weidget overrides label overrides stock_id
> 
>   Or there is the option that conflicting properties are
>   mutually exclusive. For example, for label/label_widget:
> 
>    Setting label turns label_widget into a GtkLabel  
>    and notifies on it.
>    Setting label_widget turns label into the text
>    of label_widget if it is GtkLabel, to NULL otherwise.
> 
>   (GtkFrame works like this, and your current create_menu_proxy()
>   works a bit like this in that it retrieves the text from
>   a GtkLabel widget in some cases.)

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

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".

I don't remember if there was a reason I made the icon_set and
icon_widget properties mutually exclusive.
 
> - 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.

> 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 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.

I don't think gtk_toolbar_focus() should handle internal children
either.  The way keyboard navigation is supposed to work is that TAB
gives focus to the first item on the toolbar, and TAB again moves out
of the toolbar. If the first item on the toolbar contains two
focusable children, do you really want TAB to move between those two
children and _then_ out of the toolbar?

Having two focusable children is not necessarily completely wrong. You
can imagine things like combo-boxes where both the entry and the arrow
can get focus. I guess you could also imagine some of the children of
the toolbar being in an hbox. Handling internal children would make
TAB walk through all those children.

I have added comments in the code. I can file a bug if you still
disagree.

> - 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.

> - In focus_home()/focus_end() it doesn't look right to have the
>   the argument to child_focus() DIR_RIGHT for both rtl/ltr and
>   for both home/end()

I have fixed this so that in LTR mode End moves to the rightmost item,
and focuses it using dir=GTK_DIR_LEFT. In RTL mode it moves to the
leftmost item and focuses it using dir=GTK_DIR_RIGHT. Similarly (but
reversed) for Home.

> Things to remember to do
> ========================

> - Should add Soeren to the copyright headers where appropriate

Done. I also added Anders and James where they weren't listed already.

> - 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.

> - Don't forget to straighten out MAPPED checks for child_focus 
>   if we change gtk_widget_child_focus() (bug 114635)

I added a comment to that bug

> - Fix radio button proxy menu items to look like radio buttons
>   (Bug 111207)

Yes, that bug should be fixed :-)

> - gtk_toolbar_set_style() needs property notification [old bug]

It does notification in _real_set_style(), this way the notification
it is also emitted when _unset_style() is called.

> - Why is gtk_toolbar_button_press() done with a signal
>   connection rather than as the default handler?

Good question. I fixed this and some other weirdness with that function

> - Not clear to me why the pack_end items are always added
>   to the result of gtk_toolbar_size_request(), since they
>   can be overflowed as well as pack_start item.

Not clear to me either. I have changed it to request 

     MIN (front_size + end_size, arrow_size) 

> - 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.

> Code suggestion
> ===============
> - Please eliminiate 'apio' and 'apin' macros. Plus, it should
>   return immediately on failure. I'd probably just code it
>   as somthing like:
> 
>   if (!gtk_toolbar_check_old_api (toolbar))
>    return;
> 
>   Though a vervose macro like CHECK_OLD_API(toolbar); would 
>   also be OK.
>   though I think open coding is probably better)

Urgh. I never meant to let those weird macros survive past deciding
how the old/new API thing should work. I did what you suggested:

    if (!gtk_toolbar_check_old_api (toolbar))
       return;

> Minor code suggestions
> ======================
> - In class structure definitions, there should be comments
>   distinguishing blocks of signals, keybinding signals, 
>   virtual functions

We don't have any virtual functions or keybindings, so all the
functions were actually signals. I have added /* signals */ comments.

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

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

> - FIXME about EXPOSURE_MASK in gtk_toolbar_realize() is right ...
>   should be removed there and elsewhere.

Filed as 116303 because it involves non-toolbar code

> 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.

> - A comment indicating the relationship between menu_item_activated()
>   and button_toggled() might be useful ... it takes a while
>   to figure out how that works and why you don't get double
>   emissions.

I have added a comment explaining the logic in gtktoggletoolbutton.c


Soeren