Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix url highlighting due to deprecation of vte_terminal_match_add_gregex #330

Merged
merged 1 commit into from
Mar 20, 2020

Conversation

norbusan
Copy link
Contributor

Fixes Issue 329

@sc0w
Copy link
Member

sc0w commented Mar 18, 2020

I see in configure.ac VTE_REQUIRED=0.48

I think you don't need the checks

@norbusan
Copy link
Contributor Author

Oh, so we could drop support for the old way completely, that would simplify the patch.

Should I update it?

@sc0w
Copy link
Member

sc0w commented Mar 18, 2020

yes, please

@norbusan
Copy link
Contributor Author

Ok, I force pushed now with the checks removed and all switched to VteRegexp

@sc0w sc0w requested a review from a team March 18, 2020 02:10
Copy link
Contributor

@rbuj rbuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test: type rpm -qi mate-desktop on a mate-terminal window and hover the mouse over an URL.

Screenshot at 2020-03-18 14-09-56

@rbuj rbuj requested a review from a team March 18, 2020 13:35
@sc0w
Copy link
Member

sc0w commented Mar 18, 2020

New warning detected in this PR:

terminal-screen.c: In function 'terminal_screen_class_init':
terminal-screen.c:596:15: warning: assignment to 'VteRegex **' {aka 'struct _VteRegex **'} from incompatible pointer type 'GRegex **' {aka 'struct _GRegex **'} [-Wincompatible-pointer-types]
  596 |  skey_regexes = g_new0 (GRegex*, n_skey_regexes);
      |               ^

@norbusan
Copy link
Contributor Author

@sc0w Thanks, I missed that one during the simplification (VTE check removal), pushed a new commit that changes the type.

@sc0w
Copy link
Member

sc0w commented Mar 19, 2020

now I noticed these warnings, but we have similar warnings in master

terminal-screen.c:186:2: warning: missing initializer for field 'flags' of 'TerminalRegexPattern' {aka 'const struct <anonymous>'} [-Wmissing-field-initializers]
  186 |  { "s/key [[:digit:]]* [-[:alnum:]]*",         FLAVOR_AS_IS },
      |  ^
terminal-screen.c:164:10: note: 'flags' declared here
  164 |  guint32 flags;
      |          ^~~~~

terminal-screen.c:187:2: warning: missing initializer for field 'flags' of 'TerminalRegexPattern' {aka 'const struct <anonymous>'} [-Wmissing-field-initializers]
  187 |  { "otp-[a-z0-9]* [[:digit:]]* [-[:alnum:]]*", FLAVOR_AS_IS },
      |  ^
terminal-screen.c:164:10: note: 'flags' declared here
  164 |  guint32 flags;
      |          ^~~~~

@norbusan
Copy link
Contributor Author

@sc0w these warnings have been there since before, because the initializers for these structs always were missing the flag value. I guess putting a simple 0 at the end of all would silence the warnings.

@sc0w
Copy link
Member

sc0w commented Mar 19, 2020

can you fix it here please?

@norbusan
Copy link
Contributor Author

Ok, I'll add a commit fixing this. Please wait a bit.

@norbusan
Copy link
Contributor Author

@sc0w I have pushed a trivial fix, but interestingly, I don't see the warning you pasted abvoe about missing initializers. Did you do something special when compiling?

Futhermore, there are more things to be fixed: The grexep stuff is still used in the search function, I will look into fixing the search, too.

@norbusan
Copy link
Contributor Author

@sc0w I have now fixed also the search problem - at least for me searching works again. There is a strange warning message issued while searching:

(mate-terminal:54775): VTE-WARNING **: 07:49:15.285: (../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))

I checked the API, and passing NULL should be correct for clearing the regex, according the the vte api document. And I don't see any multline compile flag set.

Anyway, functionality-wise I can at least search with default settings, didn't try to do anything fancy, though.

@sc0w
Copy link
Member

sc0w commented Mar 20, 2020

Did you do something special when compiling?

I just saw the travis logs (--enable-compile-warnings=maximum)

please, can you squash 8e434e6 90f80ad b171a43 into one unique commit?, and it will be ready to go.

I think 3727f7c is unrelated and it can be handled in new PR, please open new PR with it.

@norbusan
Copy link
Contributor Author

Hi @sc0w
Done all that. Search related PR is here #332

Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

No new warnings in the logs, it works as expected, and it fixes a lot of warnings running mate-terminal with xterm:

(mate-terminal:30692): VTE-CRITICAL **: 13:47:42.908: void vte_terminal_match_set_cursor_type(VteTerminal*, int, GdkCursorType): assertion 'tag >= 0' failed

@sc0w sc0w merged commit c21a7b3 into mate-desktop:master Mar 20, 2020
@norbusan
Copy link
Contributor Author

Thanks for merging!
Now I am looking into all the other deprecated warnings, there are quite a lot of them ...

@norbusan norbusan deleted the fix-url-vte60 branch March 20, 2020 13:01
@raveit65
Copy link
Member

@norbusan
Can this be used for distros with vte-0.58.3, eg. fedora 31?
https://koji.fedoraproject.org/koji/buildinfo?buildID=1472915
My plan is to update fedora 31 with MATE 1.24 after fedora 32 is released in a few weeks.

@norbusan
Copy link
Contributor Author

@raveit65 Yes, the functionality used is there since vte .48 afair, so if you have 0.58 then it should be no problem to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants