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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 22 04:28:04 PDT 2010


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


Xan Lopez <xan.lopez at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #62186|review?                     |review-
               Flag|                            |




--- Comment #16 from Xan Lopez <xan.lopez at gmail.com>  2010-07-22 04:28:04 PST ---
(From update of attachment 62186)
>+    std::string m_searchedWord;

I don't think std::string is commonly used in WebKit, check CString in WTF.

>+    std::list<GList*> m_searchDomainList;

Same for this, mixing two list implementations seems a bit weird. Maybe just stick to GList?

>+// Test keypresses to see if they are to be passed to the event handler
>+bool PopupMenu::isKeyPressToZap(GdkEventKey* event)

The method name should be what the method does, not what it's used for IMHO. Since you seem to check if it's a modifier, the name should go along those lines.

>+{
>+    guint keyval = gdk_unicode_to_keyval(event->keyval);

So... isn't this already a key symbol? I'm confused.

>+
>+    // If the key is a modifier it should be zapped
>+    if (keyval == (event->keyval | 0x01000000))
>+        return true;

In any case you should mention that this is what the function returns if there's no corresponding symbol, otherwise it's difficult to know what's going on.

>+
>+    return false;
>+}
>+
>+// Search for a menuItem in a domain and returns the menuItem if found
>+// searchDomain: the domain searched
>+// word: the menuItem label
>+// resultItem: the menuItem if found
>+bool PopupMenu::search(GList* searchDomain, const std::string& word, GList*& resultItem)
>+{
>+    ASSERT(searchDomain);
>+
>+    std::string label;
>+    unsigned int size = g_list_length(searchDomain);
>+
>+    for (unsigned int i = 0; i < size; ++i) {
>+        label = gtk_menu_item_get_label(GTK_MENU_ITEM(searchDomain->data));
>+        if (!label.empty())
>+            label = pango_trim_string(label.c_str());
>+        if (label.size() > word.size())
>+            label.erase(word.size());
>+
>+        if ((label.c_str() && word.c_str()) && !strcasecmp(label.c_str(), word.c_str())) {
>+            resultItem = searchDomain;
>+            return true;
>+        }

I think you should use the GLib UTF8 collate functions here, and just compare by prefix using the word length, seems more straightforward.

>+        searchDomain = g_list_next(searchDomain);
>+    }

Micro-optimization time! You don't really need to go through the list once to get the size and then iterate again, just iterate until NEXT is NULL.

>+
>+    return false;
>+}
>+
>+// Build a search domain containing menuItems starting with 'word'
>+// startingFrom: starting position in the current search domain
>+// word: prefix of menuItem label looked for
>+// currentDomain: current search domain
>+// result: builded search domain if currentDomain has been reduced
>+bool PopupMenu::buildDomain(GList* startingFrom, const std::string& word, GList* currentDomain, GList*& result)
>+{
>+    ASSERT(startingFrom);
>+
>+    std::string label;
>+    GList* newDomain = 0;
>+    unsigned int size = g_list_length(startingFrom);
>+
>+    for (unsigned int i = g_list_index(startingFrom, startingFrom->data); i < size; ++i) {
>+        label = gtk_menu_item_get_label(GTK_MENU_ITEM(startingFrom->data));
>+        if (!label.empty())
>+            label = pango_trim_string(label.c_str());
>+        if (label.size() > word.size())
>+            label.erase(word.size());
>+
>+        if ((label.c_str() && word.c_str()) && !strcasecmp(label.c_str(), word.c_str()))
>+            newDomain = g_list_prepend(newDomain, startingFrom->data);
>+        startingFrom = g_list_next(startingFrom);
>+    }

All the comments for the previous method apply.

>+
>+    if (g_list_length(newDomain) < g_list_length(currentDomain)) {
>+        result = g_list_reverse(newDomain);

Mmm, why do we need to reverse it?

>+        return true;
>+    }
>+
>+    return false;
>+}
>+
>+// Search the word typed on the keyboard in the PopupMenu and select the
>+// correspnding menuItem if found

typo in 'correspnding'.

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



More information about the webkit-unassigned mailing list