Re: Thoughts on the ComboBox framework

[Havoc Pennington <hp redhat com>]

Hi,

This is really great!

On Sat, Mar 29, 2003 at 02:47:45PM +0100, Kristian Rietveld wrote: 
> EggComboBox is the generic base class of the framework. It contains a
> chunk of code shared between the different ComboBoxes. Having a base
> class for the ComboBoxes is of course good for consistency if everybody
> bases their custom ComboBoxes on this base class. Though the problem I
> encountered later is that the individual implementations of the Combos
> need a lot of hacks outside of the shared code to get the feeling and
> behaviour right. This includes evil hacks to get around the inflexibility
> of EggComboBox, which basically renders the idea of having the base class
> useless.

I would generally agree that inheritance purely for implementation
reasons isn't great. has-a tends to be a nicer long-term idea than
is-a when the inheritance is just an implementation detail.

But inheritance may make sense if there's really shared interface
between all the widgets.

>   struct _EggComboBoxClass
>   {
>     GtkHBoxClass parent_class;

Deriving from hbox is an example of the above, coincidentally. ;-) 
It's more annoying, but really more cleanly opaque I think to derive
from GtkBin and then happen to contain an hbox, so your combo box 
doesn't support methods like pack_end().
                                                                                 
>     /* Key Binding signals */
>     gboolean (* popup)       (EggComboBox *combobox);

Is current practice to avoid putting these in the class struct?
 
>   void       egg_combo_box_popup             (EggComboBox *combobox);
>   void       egg_combo_box_popdown           (EggComboBox
>   *combobox);

These would need to take a timestamp etc. like gtk_widget_popup().
 
>  - Should the changed signal stay here? Or move it to the individual
>    combo implementations and provide more arguments (ie the new
>  item).

I'd typically be anti- having more arguments. It's better to have no
args then have combo_box_get_current_item().
 
>   void  egg_cell_view_set_background_color  (EggCellView     *cellview,
>                                              GdkColor
>   *color);

Can this pull from the theme and honor gtk_widget_modify_bg() by
default? If so, how do I set it back to that state?
 
> EggComboBoxPicker

If this is the standard/normal/prototypical combo box widget, I think
it should be called GtkComboBox. This means the base class would be 
GtkComboBoxBase or something if it continues to exist.
 
>   void egg_combo_box_picker_insert     (EggComboBoxPicker *picker,
>                                         gint index,
>                                         ...);
>   void egg_combo_box_picker_append     (EggComboBoxPicker *picker,
>                                         ...);
>   void egg_combo_box_picker_prepend    (EggComboBoxPicker *picker,
>                                         ...);
>   void egg_combo_box_picker_remove     (EggComboBoxPicker *picker,
>                                         gint index);
>   gint egg_combo_box_picker_get_index  (EggComboBoxPicker *picker,
>                                         ...);
>   void egg_combo_box_picker_clear      (EggComboBoxPicker *picker);

I think it'd be worthwhile to have special-case append/prepend/insert
that take a single string argument:

 gtk_combo_box_append_string (combo, "Foo");

As this is the 95% case.

Need to be sure we allow the following simple code:

 combo = gtk_combo_box_new ();
 gtk_combo_box_append_string (combo, "Foo");
 gtk_combo_box_append_string (combo, "Bar");
 gtk_combo_box_set_active (combo, 1);

 g_signal_connect (combo, "changed", my_callback, NULL);

void 
my_callback (GtkComboBox *combo)
{
  int active = gtk_combo_box_get_active (combo);
}

That's almost always what you want to do so it should be dead easy.
 
> _get_index takes a bunch of column number/value pairs, and returns the
> index of the item matching those pairs.

So I could do:

 int i = get_index (combo, 0, "Bar");

?

That seems to encourage some real i18n-breakage, though I can imagine
legitimate uses also with non-string types.

>   gint egg_combo_box_picker_get_sample (EggComboBoxPicker *picker);
>   void egg_combo_box_picker_set_sample (EggComboBoxPicker *picker,
>                                         gint index);
> 
> Functions to set/get the item displayed in the sample widget.

I don't think people would think about this as "item displayed in the
sample widget" but rather as "currently selected item" or something.

Perhaps get_active()/set_active() would be better names?

> * Questions and discussion
> 
>  - We might want to change the name. EggComboBoxChooser maybe?

I'd strongly vote for plain ComboBox for the name of this one.
 
> EggComboBoxGrid
> ---------------

For this one, it seems random to me that it exposes GtkMenu/GtkWidget
in the API, while the other combo doesn't. In fact the cleanest API to
me might be egg_combo_box_picker_set_grid_mode() rather than a
separate widget... 

I understand the implementation issues, it just seems confusing to
have a whole different widget/API purely for this visual layout
change.
 
>  - Do we need autolayouting of items?

I'd expect you almost always want autolayout, though perhaps being
able to set an "aspect ratio" (in terms of number of items not pixels)
would be useful so you could tweak the autolayout for non-square
items.

 
> EggEntry: history and completion for GtkEntry
> ---------------------------------------------
... 
> For completion, the completion code from Galeon was discussed on the mailing
> list earlier. In my opinion the Galeon code is too bloated and big to put
> in GTK+ as a simple completion mechanism. Personally I see *much* more in
> a small, well designed mechanism as I will present here. During the design
> I got some help from Marco (from Epiphany fame). According to us, the
> design presented here should be advanced/featureful enough for most uses
> of completion.
> 
> Also the e-completion code was considered. And personally I think that's
> too featureful too for usage in GTK+. Marco and I really think that the
> small API discussed below is good enough.

The key question seems to be: will the current users of e-completion
etc. agree with you? One of our goals should be for Evolution,
nautilus, etc.  to use this widget.

>   typedef gboolean (* EggCompletionFunc) (const gchar *key,
>                                           const gchar *item,
>                                           GtkTreeIter *iter,
>                                           gpointer     user_data);
> 
> The idea of function is passing the key and an item, provided as an iter
> and as the string which is in the given list_column of the model. The user
> should return a gboolean indicating whether item/iter is a match or
> not.

This locks us in to linear searching for the completions, doesn't it?

It seems "safer" if we have an escape hatch; the base functionality is
a function "get the completion for the key" and then we provide
convenience functionality that does that by scanning the ListStore.

>   void     egg_entry_enable_completion  (EggEntry          *entry,
>                                          GtkListStore      *model,
>                                          gint               list_column,
>                                          gint               entry_column,
>                                          EggCompletionFunc  func,
>                                          gpointer           func_data,
>                                          GtkDestroyNotify
>   func_destroy);

(GtkDestroyNotify might be deprecated in favor of GLib something.)

>   gboolean egg_entry_completion_enabled (EggEntry          *entry);

(Might need to be _get_enabled())

> The second function is pretty much trivial, the first one needs some more
> explaination. @entry is the EggEntry we want to enable completion on and
> @model is a GtkListStore providing a possible list of completions. This has
> to be a list store because the history code needs write access to the
> model. This might be inflexible, but I don't think this is a big
> problem.

The ListStore would feel less inflexible to me if it was just
convenience functionality, and we had an escape hatch. 
I guess the escape hatch is two callbacks then; "get completion for
key" and "add history entry"
 
>   void   egg_entry_history_set_max  (EggEntry     *entry,
>                                      gint          max);
>   gint   egg_entry_history_get_max  (EggEntry     *entry);

These methods are really only on the convenience ListStore feature.

Perhaps the way to do the "escape hatch" is:

 interface EntryHistory
 {
    void add_item (string item);
    bool complete (string key, out string item);
 }

 class EntryHistorySimple : EntryHistory
 {
    void set_completion_func (...);
    void set_max (int max);
    int  get_max ();
 }
 
> In the current design the EggComboBoxText is just an extra layer around
> the EggEntry, providing the user with an option to display the full list
> which is kept in the background. This full list would contain all
> possible completions/items in the history.

What if we just add gtk_entry_set_show_completion_popdown()?

This widget really seems to encourage confusion with
ComboBoxPicker. Perhaps it should be named GtkHistoryEntry or
something like that (no combo box in the name).

> Because of this the EggComboBoxText class does not contain any functions
> to add/remove items from the list. This should be done by using the
> GtkListStore directly. But we *do* provide functions to add items in the
> EggComboBoxPicker class, so this might be inconsistent.
> 
> * API discussion
> 
>   struct _EggComboBoxTextClass
>   {
>     EggComboBoxClass parent_class;
>   };

It makes more sense to me if this is_a GtkEntry. Seems like the
ComboBox base class is distorting the right conceptual hierarchy for
implementation reasons.
 
>   gint   egg_combo_box_text_get_sample_index (EggComboBoxText *textcombo);
>   gchar *egg_combo_box_text_get_sample_text  (EggComboBoxText *textcombo);
>   void   egg_combo_box_text_set_sample       (EggComboBoxText *textcombo,
>                                               gint index);

Again the "sample" terminology is weird to me.
 
>  - Do we need something to toggle completion?

What do you mean by toggle completion?
 
>  - EggComboBoxPicker has functions for adding/removing items. EggComboBoxText
>    has not, and expects the user to do this via GtkListStore. Is this
>    inconsistent?

If they both are combo boxes, it's strange, but I don't think they
both should be. Why not completely separate the combo and
history/completion APIs? They really have very little in common; 
I don't think "there's a popdown thing" is the most useful dimension
of widget categorization.

To summarize the suggestions I made, I think it'd be nice to drop the
ComboBox base class, rename ComboBoxPicker to plain ComboBox, merge
ComboBoxGrid into ComboBoxPicker, merge ComboBoxText into Entry or
alternatively name it HistoryEntry. Then you have a lot fewer
implementation details floating around and avoid confusion about which
widget to use when.

Havoc