[Webkit-unassigned] [Bug 27443] [Gtk] Combobox should support "find-as-you-type" search

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27443





--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2010-08-07 14:20:54 PST ---
(From update of attachment 63496)
Thanks for working on this patch. I agree that this functionality is very
important to have and I'm glad you're tackling it.

>  #elif PLATFORM(GTK)
> +    GList* m_searchDomain;
> +    GString* m_searchedWord;

Please use GOwnPtr for these types and be sure to write template
specializations for them if they do not exist yet. This will reduce
the risk of memory leaks greatly.

I'm not sure I understand exactly why you are caching the menu items
here (m_searchDomain), is the performance gain worth it? If you didn't,
it seems like you could get rid of the filterDomain method. Take a look
at PopupListBox::typeAheadFind in the chromium directory for where they
implement this same behavior in one method. Maybe we should match that?

> +    void resetWord();
> +    bool isPrintable(GdkEventKey*);
> +    bool search(GList*, GString*&, GList*&);
> +    bool filterDomain(GList*, GString*&, GList*, GList*&);
> +    void searchItem(GdkEventKey*, PopupMenu*);
> +    static bool keyPressEventCallback(GtkWidget*, GdkEventKey*, PopupMenu*);

In my opinion, these method names are too general. There is no way to tell
by looking at them they pertain to find-as-you-type. I don't mean to nit
too hard, but "Domain" has a fairly overloaded meaning in web browsers.
Maybe it could be "PossibleTypeAheadFindResults." Bytes are very cheap! :)

> +        g_list_free(m_searchDomain);
> +        g_string_free(m_searchedWord, TRUE);

This kind of stuff can be omitted entirely if you switch to
GOwnPtr, or even switching to using WebCore types (though I
know that you just switched it to GObject types, so maybe that
kind of change could happen in a later cleanup).

>      g_list_free(children);
>      gtk_menu_popup(m_popup.get(), 0, 0, reinterpret_cast<GtkMenuPositionFunc>(menuPositionFunction), this, 0, gtk_get_current_event_time());
> +    m_searchedWord = g_string_new(NULL);

Isn't it possible at this point for m_searchWord to already exist?
Again using GOwnPtr here would prevent a memory leak.

> +    for (; searchDomain; searchDomain = g_list_next(searchDomain)) {
> +        g_string_assign(label, gtk_menu_item_get_label(GTK_MENU_ITEM(searchDomain->data)));
> +        if (label->len)
> +            g_string_assign(label, pango_trim_string(label->str));

In my opinion, it isn't worth using pango here, just to use pango_trim_string.
It probably makes much more sense to use WebCore::String and just call
stripWhiteSpace(). You would avoid any need for manual memory management that way.
Remember to handle the conversion from UTF8 properly (fromUTF8).

Including here:
> +            g_string_free(label, TRUE);
and here:
> +    g_string_free(label, TRUE);

:)

> +bool PopupMenu::filterDomain(GList* startingFrom, GString*& word, GList* currentDomain, GList*& result)

I'm still not convinced this is necessary. What happens if the menu changes while
the user is typing? Is that possible? Why is the "word" parameter a reference to a
pointer? That's very confusing and could lead to errors.

> +// Search the word typed on the keyboard in the PopupMenu and select the
> +// corresponding menuItem if found
> +void PopupMenu::searchItem(GdkEventKey* event, PopupMenu* that)

Why are you passing in "that" to the method here? that == this!

> +bool PopupMenu::keyPressEventCallback(GtkWidget* widget, GdkEventKey* event, PopupMenu* that)
> +{
> +    if (that->isPrintable(event)) {
> +        that->searchItem(event, that);
> +        return true;
> +    }
> +    // Reset the search on Delete key press
> +    if (event->keyval == GDK_Delete)
> +        that->resetWord();

I feel like these if statement should be folded into searchItem,
since this effectively splits up the logic of find-as-you-type
between the outside of the class and the inside. You could just
give the method a more general name, much like Chromium's typeAheadFind.

So you could just do:
> +bool PopupMenu::keyPressEventCallback(GtkWidget* widget, GdkEventKey* event, PopupMenu* that)
> +{
> +     return that->typeAheadFind(event);

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.