Re: Thoughts on the ComboBox framework

[Kristian Rietveld <kris gtk org>]

On Sat, 2003-03-29 at 17:32, Havoc Pennington wrote:
> 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.

Agreed, at the moment there is some kind of shared interface between all
the widgets. But it's kinda useless and pointless.

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

Good idea.

>                                                                                  
> >     /* Key Binding signals */
> >     gboolean (* popup)       (EggComboBox *combobox);
> 
> Is current practice to avoid putting these in the class struct?

Yeah, GtkTreeView does it too (:.

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

Okay, but now if we drop the base class, would it make sense to give the
individual combos _popup/_popdown functions?

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

Alright.

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

Hmm, you bring up a good question here. I actually hardcoded (eeek) the
white color for the "Windows style". Of course the color should come
from the theme. By default a cellview does not have a background color,
but the EggComboBoxPicker does need the ability to set the background
color on it. I think it should be possible for the theme to set
different background colors for cellviews and the cellview used in the
combo box.

So I guess ditching this function and getting EggComboBoxPicker to use
gtk_widget_modify_bg is the best option. (And getting EggCellView to
pull the default from the theme).

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

Agreed.

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

Yeah, I am okay with adding special cases/convenience functions. But
there should be a limit for it. One could argue that having functions to
append/prepend/insert image/text pairs are useful too (I think that
might be a worthwhile addition actually). So the question is -- where do
we draw the line?.

>  
> > _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");
> 
> ?

Yeah.

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

Hrm, yes. The i18n breakge sounds pretty bad. Getting the insertion
functions to return indices isn't really useful I think, because the
indices can change when you insert more items, etc.

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

Agreed.

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

The problem is that the EggComboBoxPicker has to be able to switch
styles. And it is *impossible* to implement a grid in a list/treeview.
Also because we have to support row/column spanning items.

Even if we add _set_grid_mode(), we still need an attach function to
allow for row/column spanning items. I think adding an
egg_combo_box_picker_attach() is even messier than having a separate
combo...

Though, set_grid_mode() will work if we drop the row/column spanning
requirement and switch to autolayouting with an aspect ratio parameter
(as you pointed out below).

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

In theory I don't think they need more than this. Practice may be
different though.

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

Each requested completion requires an iteration on the GtkTreeModel. So,
yes.

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

Yeah.

> 
> >   gboolean egg_entry_completion_enabled (EggEntry          *entry);
> 
> (Might need to be _get_enabled())

Good point.

> 
> > 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 ();
>  }

Ok, that looks nice. And GTK+ would provide a convenience implementation
using GtkListStore? And this way we provide the users the option to
reimplement the EntryHistory interface for other TreeModels. Did I
understand it correctly?

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

Interesting.

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

Yeah, I think it's a good idea to add a GtkHistoryEntry which "is" an
EntryHistorySimple class and adds the _set_show_completion_popdown(). Of
course it derives from GtkEntry then.

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

Disabling/Enabling completion.

I would vote for dropping this TextCombo and having a GtkHistoryEntry
now.


Thanks for your insightful comments.



	-Kris

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