We have a GSoC student working on this over the summer.

https://wiki.gnome.org/Apps/Builder/SearchAndReplace

Created attachment 330735 [details] [review]
editor-frame: Add search-and-replace to the search UI

This commit implements Allan's designs for the search-and-replace UI,
which add a replace text entry field, Replace and Replace All buttons,
and search option check boxes to the existing search UI. Project-wide
replace has not been implemented yet, and the "Search selection" option
was not included because the current selection is the default search text.

Created attachment 330736 [details] [review]
editor-frame: Add actions to make search-and-replace work

This commit adds actions so the "Replace" and "Replace All" buttons
work, using the GtkSourceView API. It also adds actions to show or hide
the Replace widgets, to show or hide the search options, and to exit the
search, which are all buttons in the search box.

Created attachment 330737 [details] [review]
editor-frame: Only enable replace/replace-all at appropriate times

When the search entry field is empty, the replace and replace-all
actions don't make sense, so the buttons should be insensitive.
Similarly, if no search occurrence is selected, the replace action
doesn't make sense. This commit sets up callbacks to try to ensure the
buttons are only sensitive (clickable) at appropriate times.

Created attachment 330738 [details] [review]
command-bar: Allow command bar actions to steal focus

Currently, gb_command_bar_hide grabs focus on whatever was focused
before the command bar popped up, or on the workbench. But if a command
bar action causes another widget to appear, we want to leave that in
focus. For example, the substitute action with the 'c' option will have
that behavior (:%s/this/that/gc).

Created attachment 330739 [details] [review]
command-bar: Implement the 'c' option for vim substitute commands

In vim, if the 'c' option is specified at the end of a substitute
command (like :%s/this/that/gc), the user is prompted to confirm or deny
replacement on each occurrence of the search term. This commit makes
Builder recognize that option and respond by opening the replace UI with
the search entry and replace entry fields filled in.

Because the search_revealer animation doesn't happen immediately (it
waits for the frame clock to increment), we can't directly trigger the
"next-search-result" action in ide_editor_frame_actions_replace_confirm.
The selection would be cleared once the search_entry widget is mapped.
So instead we setup a callback on the buffer's has-selection property,
and use that to trigger next-search-result if there's a pending
replace-confirm action.

Review of attachment 330738 [details] [review]:

LGTM

Review of attachment 330739 [details] [review]:

In general looks good, few things to fix

::: libide/editor/ide-editor-frame-actions.c
@@ +365,3 @@
+{
+  IdeEditorFrame *self = user_data;
+  g_autofree const gchar **strv;

= NULL;

@@ +367,3 @@
+  g_autofree const gchar **strv;
+
+  g_assert (IDE_IS_EDITOR_FRAME (self));

Add additional assertions for:

 g_assert (state != NULL);
 g_assert (g_variant_is_of_type (state, G_VARIANT_TYPE_STRING_ARRAY));

@@ -361,0 +361,11 @@
+static void
+ide_editor_frame_actions_replace_confirm (GSimpleAction *action,
+                                          GVariant      *state,
... 8 more ...

Store the length of the array instead of NULL, and ensure length is >= 2 before dereferencing below.

::: libide/editor/ide-editor-frame-private.h
@@ -58,2 +58,3 @@
   guint                auto_hide_map : 1;
   guint                show_ruler : 1;
+  guint                pending_replace_confirm;

I tend to put whole/half word things together, so that we have one group of bitfields in the structure. So in this case, put pending_replace_confirm before the bitfields (next to the whole-word gulong field).

It's not a big deal here since we don't create a bunch of these structures, but it's good hygiene to practice.

::: plugins/command-bar/gb-vim.c
@@ +1074,3 @@
+            case 'c':
+              confirm_replace = TRUE;
+              break;

newline after break

@@ +1089,3 @@
 
+  if (confirm_replace)
+  {

We follow gtk+/gnu conventions, which indents the {} 2 spaces

@@ +1096,3 @@
+    g_variant_builder_add (builder, "s", search_text);
+    g_variant_builder_add (builder, "s", replace_text);
+    variant = g_variant_builder_end (builder);

This leaks. Calling g_variant_builder_end() is only enough if you are using a stack allocated builder. (Otherwise you still need to unref the builder too).

I'd just switch this to use a stack allocated builder and switch g_variant_builder_new() to g_variant_builder_init() and add appropriate &builder.

Review of attachment 330737 [details] [review]:

The action setting reminds me that we should probably make a generic helper for this. In other classes we have a variadic helper that lets you do something like:

  action_set (self, "foo-action",
              "enabled", TRUE,
              NULL);

We could probably make that more generic and resolve the action from the widget hierarchy.

Either way, feel free to push after a couple small nits.

::: libide/editor/ide-editor-frame.c
@@ -407,0 +407,6 @@
+static void
+on_buffer_has_selection_changed (IdeEditorFrame *self,
+                                 GParamSpec     *pspec,
... 3 more ...

These declarations can probably be moved to the inner scope.

@@ +417,3 @@
+
+  if (!gtk_text_buffer_get_has_selection (GTK_TEXT_BUFFER (buffer)))
+  {

Mind the indentation (2 spaces for {})

Review of attachment 330735 [details] [review]:

LGTM

Review of attachment 330736 [details] [review]:

Just a few things to fix

::: libide/editor/ide-editor-frame-actions.c
@@ -179,0 +180,10 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 7 more ...

You might consider using gtk_widget_set_visible() here with the boolean value you determined for the branch. ½ the code

@@ -179,0 +180,36 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 33 more ...

We don't use gtk_widget_show_all() in this code-base. Instead, toggle the visibility of the container (and leave the children always visible).

@@ -179,0 +180,78 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 75 more ...

I think this is getting leaked?

@@ -179,0 +180,89 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 86 more ...

Do keep in mind that g_return_if_fail() can be compiled out (generally we do not do that though). So if you intend for these to be more than "non-crashing assertions", they need to be changed.

@@ -179,0 +180,125 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 122 more ...

Leaked here too

@@ -179,0 +180,144 @@
+static void
+ide_editor_frame_actions_toggle_search_replace (GSimpleAction *action,
+                                                GVariant      *state,
... 141 more ...

Potentially returning while completion is blocked. If we want the conditional checks, we need a `goto cleanup` or something.

Pushed.

commit 4fa4c667c348c7e9e0b0f73abb26e5d42cb9ace0
commit 8d870b6e693df9d2aca94e92d684af9884d94cfc
commit f8ae0b4a1c51bb4df66fc1caaec593390f9ba564
commit 32117770db5b9933bf3035bedfbc7a71d3b925eb
commit 50f6f7b5491cc9e1989662d001ae6e33b7efdafb