Re: Autocompletion

[Ricardo Fernández Pascual <ric users sourceforge net>]

El dom, 06-10-2002 a las 01:38, Havoc Pennington escribió:
> 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)
> 
I will fix those.


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

Yes, this is one of the reasons that I think that having this as part of
Gtk (or any other standard lib) is important. I'm sure that the current
galeon entry is not accesible.

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

If you don't know what it does it probably is not needed. I don't know
either, I just copied it the first time that I wrote a g_signal_new and
have been uding it for ever.

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

Right.


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

It should not be part of the API. It is a leftover from galeon.

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


The get_matches should return the sorted array directly. I don't think
that the unserted array is useful anymore. It was used before in galeon,
but now it isn't.

> Would it be nice to use GArray for the ACMatchArray?
> 
It is only a small internal type. I could use GArray if you think it
will make things a lot clearer. 

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

Tight, this will be solved if I make it a subclass of 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.)

All those things come from galeon1. Those are the ugly hacks needed to
add autocompletion work with GtkEntry. I'm not sure that they are all
needed with Gtk2. 

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

I should have removed this. It is used in galeon to highlight the entry
when the user hits C-l


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

All agreed.

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

This class is only a sample autocompletion source. It would be very easy
to make the EggFileSystem class implement EggAutocompletionSource.

Personally, I would implement a subclass of EggFilesystem using GnomeVFS
instead of unix calls and use that. One thing that I don't like about
the EggFileSystem class is that it does not seem to allow asynchronous
use, which is very useful in this case.

> 
> 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:///");


Not sure, because I don't know if "file://" is a valid uri at all.


-- 
Ricardo Fernández Pascual
ric users sourceforge net
Murcia. España.