GNOME Bugzilla – Bug 707086
Port the find bar to GtkSearchBar and fix the appearance of the buttons next/prev
Last modified: 2013-09-06 20:56:37 UTC
The patch. Note: The patch it is adding one string to translation.
Created attachment 253555 [details] [review] ephy-find-toolbar: Port to GtkSearchBar
Created attachment 253556 [details] Screenshot
Created attachment 253756 [details] [review] ephy-find-toolbar: Port to GtkSearchBar
Created attachment 253757 [details] [review] ephy-find-toolbar: Port to GtkSearchBar
Review of attachment 253757 [details] [review]: A few quick comments. ::: src/ephy-find-toolbar.c @@ +47,3 @@ WebKitFindController *controller; #endif + GtkWidget *search_entry; Is it really necessary to rename the variable? @@ +156,3 @@ + gtk_widget_set_sensitive (priv->prev, result != EPHY_FIND_RESULT_NOTFOUND); + gtk_widget_set_sensitive (priv->next, result != EPHY_FIND_RESULT_NOTFOUND); I see what you're doing here, but since it is an independent clean-up you should fix this in a separate patch. @@ +176,3 @@ + gtk_widget_set_sensitive (priv->prev, FALSE); + gtk_widget_set_sensitive (priv->next, FALSE); Same as commented above.
Created attachment 253785 [details] [review] ephy-find-toolbar: Port to GtkSearchBar I also fix the problem in the focus to the address bar (with the old patch, open new tab not focus to the address bar): - if (gtk_widget_get_visible (GTK_WIDGET (toolbar))) + if (gtk_search_bar_get_search_mode (GTK_SEARCH_BAR (toolbar)))
Created attachment 253786 [details] [review] ephy-find-toolbar: Port to GtkSearchBar In mistakenly I removed the GTK_WIDGET macro from toolbar variable: - gtk_widget_show_all (toolbar); + gtk_widget_show_all (GTK_WIDGET (toolbar));
Created attachment 253787 [details] [review] ephy-find-toolbar: cleanup
Review of attachment 253787 [details] [review]: Instead of writing "cleanup" write the text in the rest of the comment, but as follows "ephy-find-toolbar: Remove cast macros from places where it's not needed".
Review of attachment 253786 [details] [review]: From the rest of the patch can't comment much until I can test it. ::: src/ephy-find-toolbar.c @@ +486,3 @@ EphyFindToolbarPrivate *priv = toolbar->priv; + gtk_widget_grab_focus (priv->entry); This could go into the other patch, no?
Review of attachment 253786 [details] [review]: Looks good! ::: src/ephy-find-toolbar.c @@ +572,3 @@ + gtk_container_add (GTK_CONTAINER (toolbar), box); + + /* Find: |_____| */ I don't understand what this comment means?
(In reply to comment #9) > Review of attachment 253787 [details] [review]: > > Instead of writing "cleanup" write the text in the rest of the comment, but as > follows "ephy-find-toolbar: Remove cast macros from places where it's not > needed". OK. (In reply to comment #10) > Review of attachment 253786 [details] [review]: > > From the rest of the patch can't comment much until I can test it. > > ::: src/ephy-find-toolbar.c > @@ +486,3 @@ > EphyFindToolbarPrivate *priv = toolbar->priv; > > + gtk_widget_grab_focus (priv->entry); > > This could go into the other patch, no? Why? The line is already was there. (In reply to comment #11) > Review of attachment 253786 [details] [review]: > > Looks good! > > ::: src/ephy-find-toolbar.c > @@ +572,3 @@ > + gtk_container_add (GTK_CONTAINER (toolbar), box); > + > + /* Find: |_____| */ > > I don't understand what this comment means? Also I not understand. The comment already was there: - /* Find: |_____| */ - priv->entry = gtk_entry_new (); + /* Find: |_____| */ + priv->entry = gtk_entry_new (); So, I'll remove the comment.
Review of attachment 253786 [details] [review]: pushed as 91ee871fceccb61e084f15b1b2b8846d0861ee9b - ephy-find-toolbar: Port to GtkSearchBar
Review of attachment 253787 [details] [review]: pushed as 7c5b4005450fc59ac287b351c1158f29c36f96a7 - ephy-find-toolbar: Remove cast macros from places where it's not needed
Does the new find bar still disappear on focus change? I find that very convenient.