Created attachment 352456 [details] [review]
devhelp-search: documentation view needs find support

Review of attachment 352456 [details] [review]:

Once again, excellent work! Everything works as expected. Just some style nits to work through so that we keep the codebase as consistent as we can.

Also, the encoding seems wrong for the author field of the git patch. It needs to be UTF-8 encoded.

Something like this should fix that.

  git config --global user.name 'Lucie Charvat'
  git config --global user.email 'luci.charvat@gmail.com'

Thanks!

::: plugins/devhelp/gbp-devhelp-search.c
@@ +37,3 @@
+
+  webkit_find_controller_search_finish(self->web_controller);
+  gtk_revealer_set_reveal_child(self->search_revealer, FALSE);

Add a space after function names and before (, like "foo_finish (self->bar)". There are a few of these here, so make sure to fix them all.

@@ +63,3 @@
+  webkit_find_controller_search(self->web_controller,
+                                search_text,
+                                WEBKIT_FIND_OPTIONS_BACKWARDS | WEBKIT_FIND_OPTIONS_WRAP_AROUND

For long bitwise enumerations like this, do:

  (FIRST_ENUM |
   SECOND_ENUM |
   THIRD_ENUM),
  more_params,
  ...

Although, it doesn't both me much if they are all on one line either. We do that in our class_init functions just because it gets really annoying otherwise.

@@ +76,3 @@
+  if (gtk_revealer_get_child_revealed (search_revealer))
+    {
+      self->selected_text = gtk_clipboard_wait_for_text (self->clipboard);

I think this leaks the previous value of self->selected_text. Add g_free (self->selected_text) before hand?

::: plugins/devhelp/gbp-devhelp-view.c
@@ -113,0 +120,6 @@
+gbp_devhelp_real_search_reveal (GbpDevhelpView *self,
+                                GdkEventKey    *event)
+{
... 3 more ...

Space after if and before (

I think it's fine to do this in the key_press_event for now, but in the future we would want to use keybindings to activate this so that shortcut themes can override. We can address that when the new shortcut branch arrives.

The way to do this until then would be to create a new signal that can be activated by keybindings like:

  signals [FOCUS_SEARCH] =
    g_signal_new_class_handler ("focus-search",
                                G_TYPE_FROM_CLASS (klass),
                                G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION,
                                G_CALLBACK (do_focus_search),
                                NULL, NULL, NULL,
                                G_TYPE_NONE, 0);

Then, also in class_init, add the binding entry:

  gtk_binding_entry_add_signal (gtk_binding_set_find_by_class (klass),
                                GDK_KEY_f,
                                GDK_MODIFIER_CONTROL,
                                "focus-search", 0);

Review of attachment 352456 [details] [review]:

and also:

::: plugins/devhelp/gbp-devhelp-search.c
@@ +129,3 @@
+gbp_devhelp_search_get_revealer (GbpDevhelpSearch *self)
+{
+  g_return_val_if_fail (GBP_IS_DEVHELP_SEARCH (self), FALSE);

return NULL, not FALSE

::: plugins/devhelp/gbp-devhelp-view.c
@@ -113,0 +120,8 @@
+gbp_devhelp_real_search_reveal (GbpDevhelpView *self,
+                                GdkEventKey    *event)
+{
... 5 more ...

space after the function name

@@ -113,0 +120,19 @@
+gbp_devhelp_real_search_reveal (GbpDevhelpView *self,
+                                GdkEventKey    *event)
+{
... 16 more ...

space after the function name

@@ +174,3 @@
 
+  self->search = g_object_new (GBP_TYPE_DEVHELP_SEARCH, NULL);
+  self->search_revealer = gbp_devhelp_search_get_revealer  (self->search);

only one space after the function name

Created attachment 352531 [details] [review]
devhelp-search: documentation view needs find support

Thank you for the feedback I hope I got the encoding finally right. Is there a tool I can use for checking coding style?

Review of attachment 352531 [details] [review]:

When I apply the patch, `git log` looks like: "Author: ucie Cha1 <°ô^]~ðU>" which I suspect is incorrect. Otherwise looks good. We don't have a tool to verify style, because admittedly its fairly esoteric style.

I can just adjust the author locally before merging, but it would be helpful going forward to save some time.

Attachment 352531 [details] pushed as 41abc86 - devhelp-search: documentation view needs find support

Thanks again for working on this, great work!