[Webkit-unassigned] [Bug 117631] [GTK] Provide search functionality to MiniBrowser

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 18 02:54:20 PDT 2013


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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #206667|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com>  2013-07-18 02:54:16 PST ---
(From update of attachment 206667)
View in context: https://bugs.webkit.org/attachment.cgi?id=206667&action=review

Looks good, but I've noticed some weird things after trying it out.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:45
> +    if (!g_strcmp0(text, ""))
> +        return;

I guess we should finish the search operation in this case.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:77
> +    case GTK_ENTRY_ICON_PRIMARY:
> +        searchNext(searchBar);
> +        break;

This is very weird, what I expect of a primary button like this one is a menu with search options, not just another button to jump to the next search result. So, I would simply remove it, it duplicates the next button functionality and makes the entry more cluttered.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:82
> +        gtk_entry_set_text(GTK_ENTRY(searchBar->entry), "");
> +
> +        WebKitFindController *controller = webkit_web_view_get_find_controller(searchBar->webView);
> +        webkit_find_controller_search_finish(controller);

Do we really need this? I think setting the entry text should be enough, since the changed signal will be emitted

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:123
> +    GtkWidget *closeButton = gtk_button_new_from_stock(GTK_STOCK_CLOSE);
> +    gtk_box_pack_start(GTK_BOX(hBox), closeButton, FALSE, FALSE, 0);

The find bar is open, but also closed with the button in the toolbar, I would not duplicate the functionality. I agree it's confusing because the button in the toolbar works as a toggle button but it's not a toggle button. So I would either convert the tool button into a toggle button and remove this one or leave this one (using only the icon and moving it to the right of the find bar) and make the tool button do nothing when the find bar is already shown.

> Tools/MiniBrowser/gtk/BrowserSearchBar.c:138
> +    GtkWidget *prevButton = gtk_button_new_from_stock(GTK_STOCK_GO_BACK);
> +    gtk_box_pack_start(GTK_BOX(hBox), prevButton, FALSE, FALSE, 0);
> +
> +    GtkWidget *nextButton = gtk_button_new_from_stock(GTK_STOCK_GO_FORWARD);
> +    gtk_box_pack_start(GTK_BOX(hBox), nextButton, FALSE, FALSE, 0);

Using stock icons for these is a bit confusing, you see a huge buttons saying Back/Forward, I would use buttons without labels and up/down icons instead of back/forward.

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