Re: Autocompletion

[Havoc Pennington <hp redhat com>]

Hi,

So here are some quick comments on the code. I didn't go through in
too much detail, mostly this is nitpicky. Overall looking at all the
code I think it looks like a good candidate for the autocomplete
feature we wanted for GtkEntry.

I think it'd be awesome to get this working with anders's filesel
stuff in libegg, and get it sorted out with respect to kris's combo
stuff.

Havoc

general
===

GTK will want GNU-style indentation not Linux-style.

Need API documentation throughout. (If using Emacs, be sure to get the
nice elisp to create a doc comment with C-x 4 h)

The various methods should have g_return_if_fail
(EGG_IS_AUTOCOMPLETION(ac)) at the beginning of the method, before
using the "ac" pointer. (this means moving "p = ac->priv" later in the
function)

There are most likely accessibility issues to keep in mind, but we can
leave that to the ATK guys to tell us what's needed.

I'm not sure what G_SIGNAL_RUN_CLEANUP does, do we need that?

EggAutocompletionEntrySignals and similar are variables not types, so
lowercase_with_underscores

Include guards should be uppercase, __EGG_FOO_H__

egg-autocompletion.[hc]
===

EggAutocompletionMatch should probably be an opaque type with accessor
functions.

If installed as public API, EGG_AUTOCOMPLETION_USUAL_WEB_PREFIXES
should be returned from a function, not done as a macro. Otherwise to
add new prefixes you have to recompile all apps. However I'm not sure 
this should be in the API, it's more of an application-specific thing.
If it's in the API I would see it more as a GLib feature,
g_uri_has_web_prefix() or something.

Do you need separate get_matches() and get_matches_sorted_by_score()
or could you just always sort?

For get_num_matches(), I think GTK usually uses get_n_matches()
instead. Or get_match_count() perhaps.

egg_autocompletion_finalize_impl would not normally have the _impl in
GTK style.

For get_matches_sorted_by_score(), if the side effect that it sorts
the array owned by the autocompletion object is visible externally in
any way, the side effect is probably bad. You don't want an accessor
function to change the nature of an object. It looks like it would
change whether get_matches() returns sorted values or not for example.

Would it be nice to use GArray for the ACMatchArray?

egg-autocompletion-entry.[hc]
===

Again I'd subclass GtkEntry, so you can use its set_text/get_text
methods and so on.

The signal should be just "activated" without the full namespacing; 
or ideally, use "activate" from GtkEntry.

GtkCombo (and GnomeEntry) are sort of deprecated, it's fine to use the
combo we keep it private, but might be nice to be thinking about
getting rid of it. Clearly to go in GTK we need to lose the GnomeEntry
or cut-and-paste the relevant bits.

This hack breaks if the user edits their keybindings, right?
        if (((event->state & GDK_Control_L || event->state & GDK_Control_R) &&
             (keyval == GDK_a || keyval == GDK_b || keyval == GDK_c ||
              keyval == GDK_d || keyval == GDK_e || keyval == GDK_f ||
              keyval == GDK_h || keyval == GDK_k || keyval == GDK_u ||
              keyval == GDK_v || keyval == GDK_w || keyval == GDK_x)) ||
            (event->state == 0 && event->keyval == GDK_BackSpace))
Probably need a more elaborate solution there. Most likely there's a
pretty easy way to do it, at least if we are allowed to extend
GTK API which we are.
(Ditto for similar stuff later in the key press handler.)

This code doesn't look right, maybe you mean gtk_grab_focus()?

    void
    egg_autocompletion_entry_activate (EggAutocompletionEntry *w)
    {
            GtkWidget *toplevel;

            toplevel = gtk_widget_get_toplevel (w->priv->entry);

            gtk_editable_select_region (GTK_EDITABLE(w->priv->entry),
                                        0, -1);
            gtk_window_set_focus (GTK_WINDOW(toplevel),
                                  w->priv->entry);
    }


egg-autocompletion-window.[hc]
===

I would expect this to be an internal-to-GTK interface, if so it's not
something we have to worry about in a lot of detail, so all the
comments here are sort of "would be nice" stuff.

Why not subclass GtkWindow instead of GObject? would avoid the extra
show/hide stuff maybe.

gtk_get_current_event_time() is better than GDK_CURRENT_TIME for
grabbing pointer/keyboard here.

The color->red ^= 0x80000 trick is better done by lightening/darkening
colors probably, or by using widget->style fields.

Columns in the list store could use symbolic names:
 enum { COLUMN_MATCH, COLUMN_TITLE }
or whatever.

egg_autocompletion_window_key_press_hack should probably be de-hacked
(perhaps use GtkBindingSet)? We may need a GtkTreeView subclass here.


egg-filesystem-autocomletion.[hc]
===

This should probably be done in conjunction with the Egg file selector
work. (The file selector should use the autocompletion entry anyway, 
one important reason to have autocomplete in GTK; and the
implementation of this object should thus use the virtual filesystem
object from the egg file selector. To avoid circular deps you probably
have to put this file in the filesel subdir in libegg.) 


Is there a gnome-vfs bug that should be fixed revealed by this?
           if (!strcmp (path, "file://"))
                {
                        /* without this, gnome-vfs does not recognize it as a dir */
                        ret = g_strdup ("file:///");