Comments on toolbar

[Owen Taylor <otaylor redhat com>]

Here are some comments I came up with when reading through all the new
GtkToolbar code last night. The comments get less detailed as they gone
on ... when after 40 or so double up pages you run into 
gtk_toolbar-size_allocate(), ones attention does tend to wander a
bit.

Basically one of three things should be done for each of the following
comments:

 - Fix it
 - File a bug to fix it later
 - Ignore it (might be nice to comment on why)

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.

Though generally speaking API docs are supposed to be a prerequisite
for adding to CVS these days... someone definitely needs to write
them RSN.

So, my suggestion would be to:

 - Read through the below, fix trivial stuff
 - File bugs about non-trivial stuff that should be fixed
   (one bug per issue or related issues)
 - Commit
 - Write docs

Regards,
						Owen

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

GtkToolItem API Comments
========================
- What's the use case for "visible_horizontal" "visible-vertical"?
- Would gtk_tool_item_unset_proxy_menu_item(ID) be useful?
- I'd do the ref/sink in gtk_tool_item_set_proxy_menu_item
  rather than in the create_proxy_menu_item() callbacks.
- Should GtkToolItem::set_tooltip use boolean_handled_accumulator?
- Could/should GtkToolItem hook up parent_set to reconfigured?

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.
- 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)
- gtk_tool_button_set_icon/label_widget() should probably
  ref/sink the widgets?
- 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)
- Should there be _with_label()/_with_mnemonic() constructors
  for the three button types to match GtkButton/GtkMenuItem?

GtkToolbar API comments
=======================
- Note that with insert_stock() we set a precendent for considering
  append/prepend superfluous .. should we continue that here?
- 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'd go ahead and deprecate 
  gtk_toolbar_[set/unset]_[icon_size/style]()
- Maybe combine focus_home/focus_end into a signal boolean-parametered
  (or enum parametered? Do we have a suitable enumeration) signal.

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

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?

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

- Once the above is sorted out (and best documented in comments),
  various functions need careful review:

    gtk_tool_button_construct_contents()
    gtk_tool_button_create_menu_proxy()
    gtk_toggle_tool_button_create_menu_proxy()

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)
- 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
- What is the difference bewteen move_focus() and focus()
  [ comments distinctly useful if there is a reason ]
- 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()

Things to remember to do
========================
- Include gtkintl.h where appropriate
- Should add Soeren to the copyright headers where appropriate
- Use global boolean handled accumulator instead of custom one
- gtktoolbar.c should use instance-private-data rather than
  g_object_set_data()
- can move "gtk-toolbar-settings" into gtktoolbar IPD
- 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.
- Don't forget to straighten out MAPPED checks for child_focus 
  if we change gtk_widget_child_focus() (bug 114635)
- Remember to use binding signals, and restore padding as appropriate
  [Note that the amount of padding removed currently is
  wrong ... class size changes with the current patch]
- Fix gtk_button_set_focus_on_click() FIXME in both places
- Fix radio button proxy menu items to look like radio buttons
  (Bug 111207)

Small bugs found
================
- Shouldn't gtk_tool_item_create_menu_item() simply return
  FALSE and do nothing if it doesn't know what to do?
- gtk_toolbar_set_style() needs property notification [old bug]
- gtk_tool_item_real_set_tooltip() needs to handle NULL bin->child
- gtk_tool_item_set_proxy_menu_item() needs to have the 
  the standard protection for setting the same item back.
- remove_weak_pointer() lefterover in gtktoggletoolbutton.c
- gtk_tool_button_set_icon_set() is in the headers twice
- Why is gtk_toolbar_button_press() done with a signal
  connection rather than as the default handler?
- 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.
- gtk_tool_item_size_request needs GTK_WIDGET_VISIBLE(bin->child)
- gtk_tool_[item/button]_size_request is buggy when child is null - 
  uses uninitialized values
- Should copy GtkToolButton::use_underline doc text from GtkButton,
  the text there is considerably better.
- gtk_tool_button_set_label/stock_id() doesn't work for s
  substrings of current label. In general, using 
  if (new == current) return; should be avoided for strings
  in favor of a delayed free of the old string after a copy.

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)

Minor code suggestions
======================
- In class structure definitions, there should be comments
  distinguishing blocks of signals, keybinding signals, 
  virtual functions
- the GTK_WIDGET_REALIZED in
   if (toolitem->drag_window && GTK_WIDGET_REALIZED(widget))
  is not necessary
- First setting of long_req in gtk_toolbar_size_request() is 
  superfluous
- handling of bin->child is gratiutiously different in
  gtk_tool_[item/button]_size_[request/allocate]
- The constant flip-flopping of the list in 
  gtk_toolbar_size_allocate() is a bit weird, since you can 
  iterate backwards over a GList.
- Perhaps getting ipadding should be encapsulated in gtktoolbar.c
  since we have functions for the other style properties?
- gtk_tool_button_parent_set()/button_clicked() have two parameters 
  on the same line
- Using gtk_tool_button_construct contents as the parent_reconfigured
  vfunction is a bit messy  in gtktoolbutton.c
- Should make the elide_underscores() function a private function
  somewhere that is shared between the two source files that use it.
- gtktoggletoolbutton.c:menu_item_activated() should
  use gtk_check_menu_item_get_active()
- FIXME about EXPOSURE_MASK in gtk_toolbar_realize() is right ...
  should be removed there and elsewhere.
- I'd make a function for destroying the tool item drag window


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