After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 707086 - Port the find bar to GtkSearchBar and fix the appearance of the buttons next/prev
Port the find bar to GtkSearchBar and fix the appearance of the buttons next/...
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Interface
3.9.x
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-08-29 21:15 UTC by Yosef Or Boczko
Modified: 2013-09-06 20:56 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ephy-find-toolbar: Port to GtkSearchBar (10.77 KB, patch)
2013-08-29 21:18 UTC, Yosef Or Boczko
none Details | Review
Screenshot (217.58 KB, image/png)
2013-08-29 21:22 UTC, Yosef Or Boczko
  Details
ephy-find-toolbar: Port to GtkSearchBar (11.21 KB, patch)
2013-09-01 14:43 UTC, Yosef Or Boczko
none Details | Review
ephy-find-toolbar: Port to GtkSearchBar (11.21 KB, patch)
2013-09-01 14:48 UTC, Yosef Or Boczko
needs-work Details | Review
ephy-find-toolbar: Port to GtkSearchBar (8.74 KB, patch)
2013-09-01 22:53 UTC, Yosef Or Boczko
none Details | Review
ephy-find-toolbar: Port to GtkSearchBar (8.75 KB, patch)
2013-09-01 23:03 UTC, Yosef Or Boczko
committed Details | Review
ephy-find-toolbar: cleanup (1.40 KB, patch)
2013-09-01 23:03 UTC, Yosef Or Boczko
committed Details | Review

Description Yosef Or Boczko 2013-08-29 21:15:41 UTC
The patch.

Note:
The patch it is adding one string to translation.
Comment 1 Yosef Or Boczko 2013-08-29 21:18:30 UTC
Created attachment 253555 [details] [review]
ephy-find-toolbar: Port to GtkSearchBar
Comment 2 Yosef Or Boczko 2013-08-29 21:22:04 UTC
Created attachment 253556 [details]
Screenshot
Comment 3 Yosef Or Boczko 2013-09-01 14:43:19 UTC
Created attachment 253756 [details] [review]
ephy-find-toolbar: Port to GtkSearchBar
Comment 4 Yosef Or Boczko 2013-09-01 14:48:05 UTC
Created attachment 253757 [details] [review]
ephy-find-toolbar: Port to GtkSearchBar
Comment 5 Claudio Saavedra 2013-09-01 21:34:35 UTC
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.
Comment 6 Yosef Or Boczko 2013-09-01 22:53:01 UTC
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)))
Comment 7 Yosef Or Boczko 2013-09-01 23:03:27 UTC
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));
Comment 8 Yosef Or Boczko 2013-09-01 23:03:43 UTC
Created attachment 253787 [details] [review]
ephy-find-toolbar: cleanup
Comment 9 Claudio Saavedra 2013-09-02 08:37:25 UTC
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".
Comment 10 Claudio Saavedra 2013-09-02 08:50:07 UTC
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?
Comment 11 Claudio Saavedra 2013-09-02 09:18:03 UTC
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?
Comment 12 Yosef Or Boczko 2013-09-02 13:33:23 UTC
(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.
Comment 13 Yosef Or Boczko 2013-09-02 13:43:16 UTC
Review of attachment 253786 [details] [review]:

pushed as 91ee871fceccb61e084f15b1b2b8846d0861ee9b - ephy-find-toolbar: Port to GtkSearchBar
Comment 14 Yosef Or Boczko 2013-09-02 13:45:20 UTC
Review of attachment 253787 [details] [review]:

pushed as 7c5b4005450fc59ac287b351c1158f29c36f96a7 - ephy-find-toolbar: Remove cast macros from places where it's not needed
Comment 15 Reinout van Schouwen 2013-09-06 20:56:37 UTC
Does the new find bar still disappear on focus change? I find that very convenient.