Re: again a new snapshot of the GtkComboBox...

[Kristian Rietveld <kristian planet nl>]

> 
> Some general API thoughts:
> 
>  * It's pretty common for people to want to use non-editable text
>    comboboxes instead of option-menus. This is following the lead
>    of Windows, which has only combo-boxes and not option menus.
> 
>    What Qt and Swing do is have only combo-boxes, but, when 
>    editing is not allowed, they may draw them in a style that
>    is quite different from the way that editable option menus
>    look.
> 
>    The advantages of the option menu are:
> 
>     - It's not clear how to bring a Mac or Motif style option menu,
>       where the menu is a menu into the GtkComboBox framework. GTK+
>       menus are pretty specialized (as Alex can testify).
> 
>     - The most desirable API for the non-editable and editable
>       cases is different. For the editable case, referring to 
>       the strings in the history list by their values works well,
>       but it works much less well for selecting between choices
>       that are _not_ fundementally textual. In that case, you
>       probably want an index-based API like GtkOptionMenu
> 
>    The advantages of the combo-box are:
> 
>     - Possible to make very Windowsy themes
> 
>     - One API for programmers, though it would be a bit of
>       a compromise.
> 
>    I think what we should definitely avoid is having some apps
>    use option menus and some apps use non-editable comboboxes. 
>    This is bad from the programmers point of view, and bad from
>    the users point of view.   

I agree.
 
>    We either have to:
>    
>     - Spend some effort to get non-editable combo-boxes working
>       and looking really good and deprecate GtkOptionMenu
> 
>     - Not allow for non-editable combo boxes and force people
>       to use GtkOptionMenu for this.
> 
>    [ I just had an idea about using GtkTreeModel to store the   
>      data for a menu that may be a nifty way of handling this,
>      but it needs some more thought and fledging out. ]
> 
>  * Another less substantial basic design issue is the issue
>    of duplicates. Other toolkits, or at least Qt allow 
>    duplicates in a combobox, at least optionally. It's 
>    not clear what the point of this is, but if we want
>    to have the feature, then we'll have to go to an
>    index-based API rather than a string-value based API.
>

We can create an option to turn the allowance of duplicates on/off like Qt
does. But at this moment there won't appear any duplicates if the user
enters something in the entry, because of the autocompletion, if it
autocompletes something, it won't be added to the list. But, if the
programmer adds an item which is a duplicate, there will be duplicates.
But I still have to get the {tab,auto}completion right (more about this
later in this mail).

>  * We may want to have the ability to set a configurable
>    limit on history length
> 

I'll add this.

>  * Your widget has autocompletion, but for Unix-heads, it would
>    be nice to have the ability to do tab completion as well.
>    Tab completion is less desirable (for newbies) in a 
>    user interface because it is hidden, and also conflicts 
>    with tab navigation, but it definitely a win for those with
>    experience with it. 
> 
>    One thing that you might need to support tab completion
>    is the ability to set temporary filters on the contents
>    of the dropdown list; also we'd have to think about the
>    consequences of wanting to have the dropdown list displaying
>    completion candidates at the same time the user as typing.
>   
>    Configurability of this should be a user setting, not an 
>    app setting.
> 
>    (This item probably shouldn't be a first priority.)

The GtkComboBoxText now has some of both, actually it's a mess right
now. When typing in the entry you can use tab to get a completion. If you
press enter in the entry, the autocompletion will activate (if the
programmer switched it on), and will try to complete the item. I can
remove the {tab,auto}completion code for now, and add it later.

> 
>  * There should be some clean way in the GtkTextComboBox
>    to get access to the entry, so the programmer can do
>    things like set up validation hooks.
>

If GTK_ENTRY(GTK_COMBO_BOX_TEXT(cbox)->entry) isn't clean enough I can
add:

GtkEntry *gtk_combo_box_text_get_entry (GtkComboBoxText *cboxtext);

> 
> To turn to the code. First, three comments:
> 
>  * You've removed the authorship/copyright attributions! This is
>    something that need to be taken very seriously.  Please make sure
>    that gtkcombobox includes all the attributions from the
>    gnumeric/gal version.
>

I forgot to add them, oops. I'll add them as soon as possible (I'm doing
it right now :).
 
>  * You need to reindent all the code from Linux style to 
>    GNU/GTK+ style (2 space indents) - other than that,
>    the coding style looks generally good.
>

I'll reindent the code after I found out how /usr/bin/indent
works. Shouldn't be that difficult :).

>  * You've removed the doc comments from the sources! You need
>    to go the other way and make sure that all public API 
>    points have documentation comments. (We'll also need
>    signal and argument documentation, but that can be done 
>    later.) [ Actually, Its possible the doc comments were
>    added after you cut-and-pasted the code. But in any
>    case, gtk-combo-box.c in GAL has doc comments, and your
>    version doesn't. ]
>

I didn't add any of the doc comments because the API's of the GAL
GtkComboBox and this GtkComboBox aren't exactly the same. But I was
planning to add documentation when the widget nears completion.

> Here are some comments on the header files. I haven't had a chance
> yet to go through all the C files in detail; I'll try to do that
> fairly soon.
> 
> The GtkComboBoxText needs a fair bit of fixing up to work with
> the current tree code. A few things to point out with the GtkTreeView
> usage are:
> 
>  1) There has been a massive change from GtkTreeNode to GtkTreeIter *.
> 
>  2) The easiest way to get a string out of a model column is to
>     do:
> 
>     char *str;
>     gtk_tree_store_get (tree, column, &str, -1);
> 
>  3) Whether you get a string that way or by using gtk_tree_model_get_value(),
>     the string you retrieve is newly allocated.
> 
>  4) You probably should be using the list store.
> 

When I started working on the GtkComboBoxText I found the liststore and
tried it. But I couldn't get it to work, so I decided to use the TreeStore
instead. I'll update my cvs GTK+ and try the list store again.

> Some additional comments on the header files:
>     
> === gtkcombobox.h ===
>    typedef struct  _GtkComboBoxClass   GtkComboBoxClass;
>    typedef struct  _GtkComboBox        GtkComboBox;
> 
>    struct _GtkComboBox
>    {
>            GtkContainer  container;
> 
>            GtkWidget    *pop_down_widget;
>                          ^^^^
> 
> Everywhere else, you seem to use "popup", and to use popdown
> to mean "remove the popup".

I've been messing around with popup and popdown... I'll fix the naming
problem :).

> 
>            GtkWidget    *display_widget;
> 
>            GtkWidget    *frame;
>            GtkWidget    *arrow_button;
>            GtkWidget    *toplevel;
>            GtkWidget    *tearoff_window;
>            gboolean      torn_off;
>            guint         use_arrows:1;
> 
>            GtkWidget    *tearable;
>            GtkWidget    *popup;
>    };
> 
> If any fields in this structure are meant to be publically
> accessible, you need to add:
> 
>  /*< public >*/
> 
>  /*< private >*/
> 

Ok.

> Comments around them for gtk-doc use.
> 
>    void         gtk_combo_box_construct           (GtkComboBox *cbox,
>  	   					   GtkWidget *display_widget,
> 						   GtkWidget *pop_down_widget);
> 
> You should get rid of this and instead just make the widget work
> reliably, if not usefully with these widgets not there. In fact,
> most of that work seems to have already been done for this.
>

In the GAL GtkComboBox the widgets of the ComboBox were constructed in
this function. Is it better if I move the code in
gtk_combo_box_construct() to gtk_combo_box_init()?

>    void         gtk_combo_box_set_arrow_relief     (GtkComboBox *cbox,
>                                                     GtkReliefStyle relief);
> 
> This function almost certainly should not be part of the application
> API. There is no reason why one application should set this different
> from another application.

I'll ditch this function.

> 
>    void         gtk_combo_box_set_display          (GtkComboBox *cbox,
>                                                     GtkWidget *display_widget);
> 
> Why is there no gtk_combo_box_set_popdown?

There wasn't one in GAL's ComboBox. I'll add it.

> 
>    void         gtk_combo_box_set_title            (GtkComboBox *cbox,
>                                                     const gchar *title);
>
> === gtkcomboboxtext.h ===
>    #include <gtk/gtktreestore.h>
>    #include <gtk/gtktreeview.h>
>    #include <gtk/gtkcellrenderer.h>
>    #include <gtk/gtkcellrenderertext.h>
>    #include <gtk/gtktreeselection.h>
> 
> I don't think that all these are necessary (you may need a few more
> than you would need from your header file since gtk_tree_store_new()
> now returns a GtkTreeStore, but you should only include the bare
> minimum required in the header file and the rest in the C file.)

I'll remove those when I'm rewriting the TreeView code for the
ComboBoxText.

> 
>     void       gtk_combo_box_text_add_string        (GtkComboBoxText   *cboxtext,
>                                                      const gchar       *string);
> 
>     void       gtk_combo_box_text_construct         (GtkComboBoxText   *cboxtext);
> 
> Shouldn't be necessary

I'll move the code to _class_init like the GtkComboBox.

> 
>     GSList    *gtk_combo_box_text_get_strings       (GtkComboBoxText   *cboxtext);
> 
> I think it would be better here to return a gchar ** as from g_strsplit()
> and friends.
>

Ok.

>     void       gtk_combo_box_text_remove_matches    (GtkComboBoxText   *cboxtext,
>                                                      const gchar       *pattern);
> 
> What is the motivation for this?

It was in the GtkClueHunter code, you can remove the matches of the given
pattern out of the list.

> 
>     void       gtk_combo_box_text_select            (GtkComboBoxText   *cboxtext,
>                                                      const gchar       *string);
> Why this as a public function?
> 
>     gchar     *gtk_combo_box_text_try_complete      (GtkComboBoxText   *cboxtext);
> 
> Why this as a public function?
> 

Both were also public in the GtkClueHunter, so I thought it was better if
I kept them public.

> Also, you should order your functions in the standard order - _get_type(),
> followed by _new(), and then the rest.
> 

Ok.

> 
> === gtkcomboboxgrid.h ===
>    struct _GtkComboBoxGridItem
>    {
>            /* private */
>            int           index;
>            GtkWidget    *button;
>            GtkWidget    *widget;
>    };
> 
> The actual structure definition should not be in the public header.
> 

I thought I had to put it above the definition of _GtkComboBoxGrid,
because _GtkComboBoxGrid needs it.

>    struct _GtkComboBoxGrid
>    {
>            GtkComboBox           cbox;
> 
> The standard for this is that such a member should be called parent_instance,
> partly to discourage people from ever accessing it directly.
> 

Ok.

>    void       gtk_combo_box_grid_add_item            (GtkComboBoxGrid *cboxgrid,
>                                                       GtkWidget       *widget);
>    GtkType    gtk_combo_box_grid_get_type            (void);
>    GtkWidget *gtk_combo_box_grid_new                 (GtkWidget *widget);
> 
>    void       gtk_combo_box_grid_remove_item         (GtkComboBoxGrid *cboxgrid,
>                                                       GtkWidget       *item);
>    void       gtk_combo_box_grid_select_item         (GtkComboBoxGrid *cboxgrid,
>                                                       GtkWidget       *item);
> 
> The names for these should be a bit more consistent with the names in GtkComboBoxText,
> I think. Also, how do you find out the currently selected item? Do you have
> to watch the :select signal?
>

For example, I got in GtkComboBoxGrid: gtk_combo_box_grid_add_item() and
in GtkComboBoxText: gtk_combo_box_text_add_item(). Which name should I
give to them? gtk_combo_box_text_add() and gtk_combo_box_grid_add()? or 
gtk_combo_box_text_add_item() and gtk_combo_box_grid_add_item()? Which one
do you like the most?

I agree watching the :select signal isn't very handy... I'll add
gtk_combo_box_grid_get_selected() and gtk_combo_box_text_get_selected().


I already want to thank everybody who helped me, because I learned a lot
of all the comments and suggestions which were given. It's the first time
for me that I'm working on something for projects like GTK+. And I'm glad
that I'm learning a lot about how GTK+ is working internally ...

	Kris

-- 
"Running Windows on a Pentium is like having a brand new Porsche but only
be able to drive backwards with the handbrake on."
	(Unknown source - taken from fortune-mod-1.0-11)