Looking at EggToolbar

[Owen Taylor <otaylor redhat com>]

I spent some time yesterday looking at EggToolbar; since it
is a prerequisite for the EggMenu code, it seems good to try
and get it into GTK+ quickly.

In general, the application API looked good - it's not 
complicated enough to be wrong, really :-)

I did have a various comments about it though:

 - It seems to me that expandable/homogeneous/pack_end should
   be child properties, not properties. While either would
   work, doing them as child properties is considerably
   more consistent with the rest of GTK+. (expand, not 
   expandable, BTW)

   The question then is whether we can keep

    egg_tool_item_set_pack_end (tool_item);

   Or need to switch to:

    egg_tool_bar_set_pack_end (tool_bar, tool_item)

   I think it's OK to keep them first way.

 - EggToolItem/EggToolButton have lots of properties with no
   corresponding getters/setters. Other than GtkCellRenderer*
   and GtkTextTag* all properties in GTK+ have corresponding
   getters/setters, so I think we should have the complete
   set here as well.

   (On the other hand, GtkCellRenderer* and GtkTextTag* have 
   _no_ setters/getters. No setters/getters would be better
   than a random subset.)

 - The names egg_toolbar_append/prepend/insert_tool_item()
   are pretty cumbersome for the primary API. Can we just
   go with plain append/prepend/insert()?

 - I suspect egg_tool_item_set_use_drag_window() should be 
   private; I can't see an application use for this function.

 - What's the purpose of egg_toggle_tool_button_toggled()?
   Yes, GtkToggleButton has it, but that's no reason to
   add if it doesn't have a useful purpose.

 - I'm not too crazy about egg_toolbar_get_tool_items();
   it's sort of consistent with some other parts of GTK+, 
   but it doesn't language bind well. The memory management
   now is wrong (must allocate new list for consistency),
   but the copied list is awkward as well. Perhaps
   n_tool_items(), nth_tool_item() would be better.

 - Why is the "clicked" signal on EggToolItem, not
   EggToolButton?
 
What I wasn't quite so comfortable with was the way
that implementing GtkToolItem  - the way it works now
is that there are a bunch of signals/virtual 
combinatins:

  GtkWidget *(* create_menu_proxy)   (EggToolItem    *tool_item);
  void       (* set_orientation)     (EggToolItem    *tool_item,
                                      GtkOrientation  orientation);
  void       (* set_icon_size)       (EggToolItem    *tool_item,
                                      GtkIconSize     icon_size);
  void       (* set_toolbar_style)   (EggToolItem    *tool_item,
                                      GtkToolbarStyle style);
  void       (* set_relief_style)    (EggToolItem    *tool_item,
                                      GtkReliefStyle  relief_style);
  void       (* set_tooltip)         (EggToolItem    *tool_item,
                                      GtkTooltips    *tooltips,
                                      const gchar    *tip_text,
                                      const gchar    *tip_private);

The 4 middle items here, in particular, strike me as a very
complex way of handling configuration changes to a GtkToolbar;
(I don't see actually how it works currently ... where do
these fields get set when the item is initially added to the
toolbar?)

what I might suggest instead is functions on EggToolItem
that look at the toolbar and get these configuration options:

 egg_tool_item_get_orientation()

And then a single signal:

 ::toolbar_reconfigured

For the set_tooltip(), is there any reason that there can't
at least be a default implementation that calls 
gtk_tooltips_set_tip () on bin->child?


Finally, just a few things I saw when browsing through the
code:

 - The inclusion of headers that are just pulled in because they 
   were pulled in before by gtktoolbar.h should be guarded with  
   DISABLE_DEPRECATED, so we have some hope of removing them
   some time in the future.
 
 - The comment /* Style functions */ in eggtoolbar.h should
   go away  but get_tool_items()/get_drop_index() probably
   should't be in that block (which is configuration)
   anyways.

 - Looks to me like the overflow menus are leaked.

 - egg_tool_item_map() does:

      gdk_window_raise (toolitem->drag_window);
      gdk_window_show (toolitem->drag_window);
    
  But gdk_window_show() already raises.

 - There are some broken boolean setters:
 
   ===
    if ((pack_end && tool_item->pack_end) ||
        (!pack_end && !tool_item->pack_end))
      return;
  
    tool_item->pack_end = pack_end;
    gtk_widget_queue_resize (GTK_WIDGET (tool_item));
   ===
 
   What if pack_end was '2'? THe right way looks like:
 
   ===
   pack_end = pack_end != FALSE;
   if (tool_item->pack_end != pack_end)
     {
       tool_item->pack_end = pack_end;
       gtk_widget_queue_resize (GTK_WIDGET (tool_item));
     }
   ===

Regards,
                                          Owen