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

[Owen Taylor <otaylor redhat com>]

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.   

   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 may want to have the ability to set a configurable
   limit on history length

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

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


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.

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

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

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.

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

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

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.

   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.

   void         gtk_combo_box_set_display          (GtkComboBox *cbox,
                                                    GtkWidget *display_widget);

Why is there no gtk_combo_box_set_popdown?

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

    void       gtk_combo_box_text_add_string        (GtkComboBoxText   *cboxtext,
                                                     const gchar       *string);

    void       gtk_combo_box_text_construct         (GtkComboBoxText   *cboxtext);

Shouldn't be necessary

    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.

    void       gtk_combo_box_text_remove_matches    (GtkComboBoxText   *cboxtext,
                                                     const gchar       *pattern);

What is the motivation for this?

    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?

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


=== gtkcomboboxgrid.h ===
   struct _GtkComboBoxGridItem
   {
           /* private */
           int           index;
           GtkWidget    *button;
           GtkWidget    *widget;
   };

The actual structure definition should not be in the public header.

   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.

   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?

Regards,
                                        Owen