fix searching in terminal window

[norbusan]

The deprecated regexpg function from vte also broken search in the terminal. Replace with searches based on VteRegexp.

[rbuj]

I saw that only one occurrence per line was highlighted even if there were more.

Test:

$ ls -la /

[norbusan]

@rbuj concerning the finding only one instance - no idea for now, I will investigate further on.

[norbusan]

@rbuj First of all, I can confirm that gnome terminal has the same behavior. Then I read through the VTE api and it seems that is how it is, I don't see currently a way around.

I have opened an issue in the vte tracker for this, let us see what the vte devs answer https://gitlab.gnome.org/GNOME/vte/issues/220

Besides this, is there anything else necessary for the PR to be included? For now I don't see a way to improve it.

[norbusan]

@rbuj Ok, I got an answer: This has been the same since the origin of times, or at least since 2013 when the first time a bug was reported :-D So this is NOT new misbehavior, but was always like this. See the above issue and https://bugzilla.gnome.org/show_bug.cgi?id=694128

[sc0w]

Running mate-terminal inside xterm I see this warning every time I click in Find with this PR:

(mate-terminal:59848): VTE-WARNING **: 16:05:19.060: (../src/vtegtk.cc:2477):void vte_terminal_search_set_regex(VteTerminal*, VteRegex*, guint32): runtime check failed: (regex == nullptr || _vte_regex_has_multiline_compile_flag(regex))

[norbusan]

Hi @sc0w I tried to track that down, without success. The warning is issued in terminal-window.c, function search_find_response_callback, in the call to vte_terminal_search_set_regex.

The regex used there comes from terminal-search-dialog.c function terminal_search_dialog_get_regex.

I did check the flags there, and I don't see PCRE2_MULTILINE being set. And I also don't see that this is a nullptr.:

--- a/src/terminal-search-dialog.c
+++ b/src/terminal-search-dialog.c
@@ -378,6 +378,10 @@ terminal_search_dialog_get_regex (GtkWidget *dialog)
                        vte_regex_unref (priv->regex);

                /* TODO Error handling */
+               fprintf(stderr, "Compiling regext from %s with flags %d\n", pattern, compile_flags);
+               if ((compile_flags & PCRE2_MULTILINE) == PCRE2_MULTILINE) {
+                       fprintf(stderr, "=====> Found multi line flag\n");
+               }
                priv->regex = vte_regex_new_for_search(pattern, -1,
                                                       compile_flags, NULL);

I am currently out of my knowledge what this can be. I checked the vte source code for the warning message, and I don't see anything else they are doing ...

[sc0w]

It seems the warning comes from here:

https://github.com/GNOME/vte/blob/mainline/src/vtegtk.cc#L2477

[norbusan]

@sc0w yes, I have checked that and followed the code path there, and I don't understand why ...

[raveit65]

I added this to fedora 32 to fix searching. https://bodhi.fedoraproject.org/updates/FEDORA-2020-10e3c3abbe https://src.fedoraproject.org/rpms/mate-terminal/c/72f9af5f47804a01a662a03704de76565befff87?branch=master

[sc0w]

@rbuj @raveit65 @mate-desktop/core-team are you agree with the merge and the warning? (https://github.com/mate-desktop/mate-terminal/pull/332#issuecomment-602057939)

[raveit65]

I didn't check if the warning does spam logs , eg. journal etc. In general i think it is better to have a warning instead of a broken search in distros. For this reason i added the commit to fedora before f32 release freeze. So yes, if it isn't fixable. I am still wondering how this is fixed by vte maintainers in gnome-terminal ( g-t and vte are maintained by the same people)?

[rbuj]

There is no multiline regex: https://github.com/mate-desktop/mate-terminal/blob/99f9ea27efe6376e3137f2942032f76937d6e603/src/terminal-screen.c#L167-L174

PCRE2_MULTILINE can be removed. https://github.com/mate-desktop/mate-terminal/blob/99f9ea27efe6376e3137f2942032f76937d6e603/src/terminal-screen.c#L584

diff --git a/src/terminal-screen.c b/src/terminal-screen.c
index 1ebe973..2722486 100644
--- a/src/terminal-screen.c
+++ b/src/terminal-screen.c
@@ -581,7 +581,7 @@ terminal_screen_class_init (TerminalScreenClass *klass)
                GError *error = NULL;

                url_regexes[i] = vte_regex_new_for_match(url_regex_patterns[i].pattern, -1,
-                                                        url_regex_patterns[i].flags | PCRE2_MULTILINE, &error);
+                                                        url_regex_patterns[i].flags, &error);
                if (error)
                {
                        g_message ("%s", error->message);

[rbuj]

There is no multiline regex: https://github.com/mate-desktop/mate-terminal/blob/99f9ea27efe6376e3137f2942032f76937d6e603/src/terminal-screen.c#L184-L188 PCRE2_MULTILINE can be removed. https://github.com/mate-desktop/mate-terminal/blob/99f9ea27efe6376e3137f2942032f76937d6e603/src/terminal-screen.c#L603

-                                                         PCRE2_MULTILINE, &error);
+                                                         PCRE2_UTF | PCRE2_NO_UTF_CHECK, &error);

[rbuj]

@sc0w Bad assert in: https://gitlab.gnome.org/GNOME/vte/-/blob/master/src/vtegtk.cc#L2171

It should be (or removed):

g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regex) == 0);

Other possible faults:

$ git grep _vte_regex_has_multiline_compile_flag
src/vtegtk.cc:        g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regex));
src/vtegtk.cc:                g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regexes[i]));
src/vtegtk.cc:        g_warn_if_fail(regex == nullptr || _vte_regex_has_multiline_compile_flag(regex));

[norbusan]

Hi @rbuj thanks for the detailed checks. I did the changes you suggested, please see the new commit. (I can squash them before you merge, or it can be squash-merged from here)

I still see warnings about MULTILINE, strange enough now that the whole code base does not contain any PCRE2_MULTILINE appearance.

First on start up, I guess from the URL init:

(mate-terminal:10180): VTE-WARNING **: 07:07:00.190: (../src/vtegtk.cc:2171):int vte_terminal_match_add_regex(VteTerminal*, VteRegex*, guint32): runtime check failed: (_vte_regex_has_multiline_compile_flag(regex))

and during each search

** (mate-terminal:10180): CRITICAL **: 07:08:04.224: search_find_response_callback: assertion 'regex != NULL' failed

and the search is broken. Will look into what was it

[norbusan]

@rbuj Switching to _PCRE2_LITERAL broke search again, I backed this change out for now.

From your comments I guess that the warning assertion in the vte code is wrong, and we should simply ignore the warning for now.

[rbuj]

Yes, I think I marked it as resolved but I did not comment... You're right, setting the PCRE2_LITERAL flag stops text search.

_vte_regex_has_multiline_compile_flag (regex) returns 0, which means PCRE2_MULTILINE flag is not set.

PCRE2_MULTILINE is 1024 (src/pcre2.h.in):

#define PCRE2_MULTILINE           0x00000400u  /* C       */

Next the warning is displayed:

[norbusan]

@rbuj Ah, that was the meaning of the resolved state - sorry, I misinterpreted that.

Yeah, I think the code as is is fine, the remaining fix is in the vte library fixing the assertion.

[raveit65]

So, a report needs to be file out at vte repo? I guess commits from PR can be squashed before merging, or not?

[norbusan]

So, a report needs to be file out at vte repo?

I guess that would be a good idea, but maybe some of the devs here are also there?

I guess commits from PR can be squashed before merging, or not?

Yes, that is possible (and that is what I normally do)

[raveit65]

I guess that would be a good idea, but maybe some of the devs here are also there?

Why should a vte dev at gitlab should receive notifications from this PR at github?

[norbusan]

@raveit65 yes, obviously no notifications, but maybe one or the other of the devs participating here is also active in gnome-terminal/vte, that what what I wanted to say.

[raveit65]

No, they don't participating here. This is only your wish or a dream :) There is no collaboration between MATE and GNOME. Sadly, without a report there this warning will never been fixed.

[norbusan]

@raveit65 Hopeless optimist I am ... ok, still, I think the one who should report is @rbuj since he discovered it, and is most competent to answer questions.

[norbusan]

Anyway, I created an issue there: https://gitlab.gnome.org/GNOME/vte/-/issues/223

[raveit65]

Thanks, can you please squash commits? Than i will test again and approve PR.

[norbusan]

@raveit65 thanks, done so now.

[raveit65]

Last question. Min require vte version for this change is 0.48? Same like other PR?

[sc0w]

latest version isn't better

now I see in xterm this warning starting mate-terminal several times:

(mate-terminal:94782): VTE-WARNING **: 20:04:56.221: (../src/vtegtk.cc:2171):int vte_terminal_match_add_regex(VteTerminal*, VteRegex*, guint32): runtime check failed: (_vte_regex_has_multiline_compile_flag(regex))

and when I commented in https://github.com/mate-desktop/mate-terminal/pull/332#issuecomment-602057939 I didn't see these warnings

[norbusan]

@raveit65 yes 0.48

@sc0w that is the same bug coming from vte. I confirm that fixing vte with the following patch

--- vte2.91-0.60.1.orig/src/vtegtk.cc
+++ vte2.91-0.60.1/src/vtegtk.cc
@@ -2168,7 +2168,7 @@ vte_terminal_match_add_regex(VteTerminal
        g_return_val_if_fail(VTE_IS_TERMINAL(terminal), -1);
        g_return_val_if_fail(regex != NULL, -1);
         g_return_val_if_fail(_vte_regex_has_purpose(regex, vte::base::Regex::Purpose::eMatch), -1);
-        g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regex));
+        g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regex) == 0);

         auto impl = IMPL(terminal);
         return impl->regex_match_add(vte::base::make_ref(regex_from_wrapper(regex)),
@@ -2292,7 +2292,7 @@ vte_terminal_event_check_regex_simple(Vt
         g_return_val_if_fail(regexes != NULL || n_regexes == 0, FALSE);
         for (gsize i = 0; i < n_regexes; i++) {
                 g_return_val_if_fail(_vte_regex_has_purpose(regexes[i], vte::base::Regex::Purpose::eMatch), -1);
-                g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regexes[i]));
+                g_warn_if_fail(_vte_regex_has_multiline_compile_flag(regexes[i]) == 0);
         }
         g_return_val_if_fail(matches != NULL, FALSE);

@@ -2474,7 +2474,7 @@ vte_terminal_search_set_regex (VteTermin
 {
         g_return_if_fail(VTE_IS_TERMINAL(terminal));
         g_return_if_fail(regex == nullptr || _vte_regex_has_purpose(regex, vte::base::Regex::Purpose::eSearch));
-        g_warn_if_fail(regex == nullptr || _vte_regex_has_multiline_compile_flag(regex));
+        g_warn_if_fail(regex == nullptr || _vte_regex_has_multiline_compile_flag(regex) == 0);

         IMPL(terminal)->search_set_regex(vte::base::make_ref(regex_from_wrapper(regex)), flags);
 }

makes all warnings go away in mate-terminal!

[norbusan]

I opened a PR https://gitlab.gnome.org/norbert/vte/-/merge_requests/1

[norbusan]

Ok, discussion with vte seems to show that we have to initialize all regex USING PCRE2_MULTILINE ... I will update this PR ...

[norbusan]

PR updated, no warnings with this one. All regex are now compiled with PCRE2_MULTILINE

[sc0w]

@raveit65

Last question. Min require vte version for this change is 0.48?

yes, vte_terminal_search_set_regex requires vte 0.46, but mate-terminal requires 0.48 in configure.ac

[rbuj]

Removes the flood of warnings about PCRE2_MULTILINE (/m pattern modifier in perl), which should be enabled even if it's not used on url_regex_patterns or skey_regex_patterns, pending removal in the future once the vte mantainers remove PCRE2_MULTILINE check.

test.sh

cat $0
echo -ne "def\na\nb\nc"
# ^a$

Test 1

Test 2

[raveit65]

Confirmed, warnings are gone :) Thanks.

[norbusan]

Thanks for merging!