Sure that looks good

Created attachment 343262 [details] [review]
Search Feature in inbuilt terminal

This is the first commit which implements search feature in the gb terminal.
The search is working correctly. Press ctrl+f when terminal is in focus to
reveal the search.

FIXMEs:
1) The close button is not hiding the search overlay. The GtkRevealer is not unrevealing
the child on close button click. But ctrl+f works fine when terminal is in focus.
2) I picked history part from gnome-terminal. I donot understand it completely.
   I am not able to access history_store.

Please guide me so that i can improve this patch.

Review of attachment 343262 [details] [review]:

first round, i'll take a look more closely at the code next round(s):

there's also five warnings in the build output (try always checking the output when finishing a patch), like:

gb-terminal.c:243:45: warning: passing argument 1 of ‘gtk_widget_get_ancestor’ from incompatible pointer type [-Wincompatible-pointer-types]

+ re-introduce the GdEntry (and all the changes in the build system to move gd from a static lib to a shared one, like rg)

for the close button to work you need at least bind it in clas_init with gtk_widget_class_bind_template_child

::: plugins/terminal/gb-terminal-view-private.h
@@ +53,3 @@
+  /* Cached regex */
+  gboolean            regex_caseless;
+  gchar               *regex_pattern;

search_text_changed and regex_caseless need to be aligned to the text, not the *

::: plugins/terminal/gb-terminal-view.c
@@ +19,2 @@
 #define G_LOG_DOMAIN "gb-terminal-view"
+#define I_(string) g_intern_static_string (string)

to remove, see comment in the signal declaration part

@@ +23,3 @@
 #include "config.h"
 
+#include <pcre2.h>

keep include alphabetical ordering

@@ +36,3 @@
 #include "gb-terminal-view-private.h"
 #include "gb-terminal-view-actions.h"
+#include "terminal-libgsystem.h"

keep a gb prefix like gb-terminal-ligbsystem.h (even if it comes from gnome-terminal)

We need to remove that and use standard GLib types, keeping only the necessary

@@ +93,3 @@
+
+#define HISTORY_MIN_ITEM_LEN (3)
+#define HISTORY_LENGTH (10)

i like to keep all the define at the file start

@@ +96,3 @@
+
+static gboolean
+history_remove_item (const char  *text)

only one space between char and *text here (can be different for other functions if we have multiples params)

@@ +98,3 @@
+history_remove_item (const char  *text)
+{
+  GtkTreeModel *model = GTK_TREE_MODEL (history_store);

use a static cast and an g_assert after params declarations
like:

GtkTreeModel *model = (GtkTreeModel *)history_store;

...

g_assert (GTK_IS_TRREE_MODEL (model));

@@ +123,3 @@
+
+static void
+history_clamp (int max)

gint

@@ +134,3 @@
+    while (1)
+      if (!gtk_list_store_remove (history_store, &iter))
+  break;

alignment

@@ +144,3 @@
+  GtkTreeIter iter;
+
+  if (text == NULL)

for string emptiness try using ide_str_empty0 (add the correspoonding include if needed)

@@ +177,3 @@
+static void
+perform_search (GbTerminalView *self,
+                gboolean backward)

alignment

same at many other places

@@ +196,3 @@
+}
+
+#if GTK_CHECK_VERSION (3, 16, 0)

we can remove this check as we link against 3.22

@@ +200,3 @@
+previous_match_cb (GtkWidget *widget,
+                  GbTerminalView *self)
+{

add g_assert for widget and view
do the same for other functions

use g_return for public functions

@@ +210,3 @@
+  perform_search (self, FALSE);
+}
+#endif /* GTK+ 3.16 */

to remove too

@@ +240,3 @@
+
+  if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->regex_checkbutton)))
+      pattern = g_strdup (search_text);

only two spaces here

@@ +242,3 @@
+      pattern = g_strdup (search_text);
+  else
+      pattern = g_regex_escape_string (search_text, -1);

same

@@ +246,3 @@
+  if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->entire_word_checkbutton)))
+    {
+      char *new_pattern;

empty line between declaration and code

@@ +257,3 @@
+
+  if (self->regex)
+    {

for one-line block we allow removing the curly braces

@@ +276,3 @@
+          (!vte_regex_jit (self->regex, PCRE2_JIT_COMPLETE, NULL) ||
+           !vte_regex_jit (self->regex, PCRE2_JIT_PARTIAL_SOFT, NULL)))
+      {

? no code ?

@@ +280,3 @@
+
+      if (self->regex != NULL)
+        {

for one-line block we allow removing the curly braces

@@ +285,3 @@
+    }
+  else
+    {

same

@@ +321,3 @@
+{
+  if (gtk_toggle_button_get_active (GTK_TOGGLE_BUTTON (self->reveal_button)))
+      gtk_widget_set_visible (GTK_WIDGET (self->search_options), TRUE);

two spaces

@@ +933,3 @@
+  VteRegex *regex;
+
+  if (G_UNLIKELY (terminal == NULL))

i don't think we gain a lot here for the compiler using G_UNLIKELY and it complicate the code lisibility

same at few places

@@ +1083,3 @@
 
+  signals[SEARCH] =
+    g_signal_new (I_("search"),

use "search", it's enough

@@ +1117,3 @@
+    g_param_spec_boxed ("regex", NULL, NULL,
+                        VTE_TYPE_REGEX,
+                        G_PARAM_READABLE | G_PARAM_STATIC_NAME | G_PARAM_STATIC_NICK | G_PARAM_STATIC_BLURB);

use G_PARAM_STATIC_STRINGS

@@ +1169,3 @@
   gtk_widget_set_can_focus (GTK_WIDGET (self->terminal_top), TRUE);
+
+  g_signal_connect (self, "search", G_CALLBACK (search_overlay_search_cb), self->terminal_top);

but with ctrl+t and vertical split i also want it to work in both split view, not only the top one

@@ +1177,3 @@
+  g_signal_connect (self, "notify::wrap-around", G_CALLBACK (search_overlay_notify_wrap_around_cb), self->terminal_top);
+
+#if GTK_CHECK_VERSION (3, 16, 0)

remove the check, we use 3.22

@@ +1218,3 @@
+terminal_search_get_regex (GbTerminalView *self)
+{
+  g_return_val_if_fail (GB_IS_TERMINAL_VIEW (self), NULL);

add an empty line

::: plugins/terminal/gb-terminal-view.h
@@ +31,3 @@
 void gb_terminal_view_set_pty (GbTerminalView *self,
                                VtePty         *pty);
+VteRegex *terminal_search_get_regex (GbTerminalView *self);

alignment of all part of the functions

::: plugins/terminal/gb-terminal.c
@@ +240,3 @@
+{
+  GtkWidget *parent_overlay;
+  GtkRevealer *search_revealer;

add an empty line after declarations

@@ +247,3 @@
+      GList *children = gtk_container_get_children (GTK_CONTAINER (parent_overlay));
+      while ((children = g_list_next(children)) != NULL)
+        {

we can remove the while block curly braces here

@@ +254,3 @@
+            }
+        }
+    }

add an empty line after the block

looking at the UI and with the inspector, i see a GtkBox not needed between the Paned and the overlay (see the attached screenshot, new layout at left, old at right )

Created attachment 343377 [details]
gb terminal new and old layout

extra GtkBox to remove

Created attachment 343481 [details] [review]
Search Feature in inbuilt terminal

second round.

I have changed GtkSearchEntry to GdTaggedEntry.
I had to add --no-as-needed LDFLAG to libide Makefile.am, without this flag
the libgd.la was not linking with libide.so hence undefines symbol error.

FIXME:
focus on search_entry when child-revealed: I tried connecting "notify::child-revealed"
signal to GtkRevealer, but i am getting assertion failure (GbTerminalView) in the
callback function.

I am making changes as we discussed on IRC, so please do not review it until I post another patch.

Created attachment 343509 [details] [review]
Search Feature in inbuilt terminal

round 3
I have implemented search for both terminals.
Selection search is also working correctly.

Todo:
use glib functions in place of ligbsystem functions.

Review of attachment 343509 [details] [review]:

Nice work! Sorry for the delay in reviews. Looking forward to another round.

::: contrib/gd/Makefile.am
@@ +5,3 @@
 
+pkglibdir = $(libdir)/gnome-builder
+pkglib_LTLIBRARIES = libgd.la

I'm tempted to say we should rename this libgd-private.la if we are going to install it.

::: plugins/terminal/gb-terminal-libgsystem.h
@@ +1,1 @@
+/*

We shouldn't need this file, it can be dropped.

::: plugins/terminal/gb-terminal-view.c
@@ +162,3 @@
+  const char *search_text;
+  gboolean caseless;
+  gs_free char *pattern;

g_autofree gchar *pattern = NULL;

We shouldn't need libgsystem, and we want to always initialize auto variables.

@@ +163,3 @@
+  gboolean caseless;
+  gs_free char *pattern;
+  gs_free_error GError *error = NULL;

g_autoptr(GError) error = NULL;

@@ +208,3 @@
+          if (self->regex_top != NULL)
+            {
+              gs_transfer_out_value (&self->regex_pattern_top, &pattern);

Use g_steal_pointer (&pattern)

@@ +1190,3 @@
+  signals[SEARCH_TOP] =
+    g_signal_new ("search-top",
+                  G_OBJECT_CLASS_TYPE (object_class),

G_TYPE_FROM_CLASS (klass) is what we use everywhere else (and if not, we should fix it).

@@ +1194,3 @@
+                  0,
+                  NULL, NULL,
+                  g_cclosure_marshal_VOID__BOOLEAN,

Just specify NULL for the marshal func. It gets optimized as appropriate by g_signal_new().

@@ -805,2 +1270,3 @@
   gtk_widget_init_template (GTK_WIDGET (self));
 
+  self->search_builder_top = gtk_builder_new_from_resource ("/org/gnome/builder/plugins/terminal/gb-terminal-search.ui");

I want to discourage the use of GtkBuilder directly. Instead, let's create a widget for that UI (subclassing GtkBin or similar) and use widget templates.

::: plugins/terminal/gb-terminal.c
@@ -235,1 +237,2 @@
 static void
+search_reveal_cb (GbTerminal *self,

This seems wrong, it doesn't take a GParamSpec parameter. Looking at the signal definition, it takes zero arguments.

Since it is a signal connection (rather than a pointer in the class vtable), it would have a "gpointer user_data" parameter, but we shouldn't do that.

Instead, call this gb_terminal_real_search_reveal and in class_init do:

  klass->search_reveal = gb_terminal_real_search_reveal;

@@ -236,0 +238,5 @@
+search_reveal_cb (GbTerminal *self,
+                  GParamSpec *pspec G_GNUC_UNUSED)
+{
... 2 more ...

Remove this

@@ -236,0 +238,6 @@
+search_reveal_cb (GbTerminal *self,
+                  GParamSpec *pspec G_GNUC_UNUSED)
+{
... 3 more ...

Precondition assertions, so:

  g_assert (GB_IS_TERMINAL (self));

@@ -236,0 +238,9 @@
+search_reveal_cb (GbTerminal *self,
+                  GParamSpec *pspec G_GNUC_UNUSED)
+{
... 6 more ...

Remove this block and do:

if (parent_overlay != NULL)
  {
    GtkRevealer *revealer = ide_widget_find_child_typed (parent_overlay, GTK_TYPE_REVEALER);

    if (revealer != NULL && !gtk_revealer_get_child_revealed (revealer))
      gtk_revealer_set_reveal_child (revealer, TRUE);
  }

@@ -316,2 +359,4 @@
       vte_terminal_match_set_cursor_type (VTE_TERMINAL (self), tag, GDK_HAND2);
     }
+    
+  g_signal_connect (self, "search-reveal", G_CALLBACK (search_reveal_cb), NULL);

Remove this. No need to use a signal connection when we have a vtable entry in the class.

Created attachment 343919 [details] [review]
Search Feature in Terminal

round 4

Created separate class for the search revealer ui. This helped in
making the code more compact and readable.

Review of attachment 343919 [details] [review]:

Well done! LGTM

Attachment 343919 [details] pushed as 4b1ecbe - Search Feature in Terminal