[RFC] tui: add support for undercurl #9052

blueyed · 2018-09-25T20:15:15Z

TODO:

~~- [ ] tmux~~

justinmk · 2018-09-26T06:22:35Z

src/nvim/tui/tui.c

@@ -1820,6 +1838,21 @@ static void augment_terminfo(TUIData *data, const char *term,
      ut, "ext.enable_mouse", "\x1b[?1002h\x1b[?1006h");
  data->unibi_ext.disable_mouse = (int)unibi_add_ext_str(
      ut, "ext.disable_mouse", "\x1b[?1002l\x1b[?1006l");
+
+  if ((int)unibi_add_ext_bool(ut, "Su", 0)) {


DId you try unibi_find_ext_str like above? (Not sure if that makes sense, but it's surprising to use unibi_add_ext_bool here. Can you explain?)

That's just what I came up with - I want to query the "Su" boolean capability here (which is not defined in unibilium already I assumed).

Just tested this again, and it is true for kitty, but false for urxvt.
It seems the return value is the value reported by the terminal, i.e. it gets queried right there.
@bfredl can you confirm?

justinmk · 2018-09-26T06:24:16Z

src/nvim/tui/tui.c

+          ut, "ext.enter_undercurl_mode", "\x1b[4:3m");
+      data->unibi_ext.exit_undercurl_mode = (int)unibi_add_ext_str(
+          ut, "ext.exit_underline_mode", "\x1b[4:0m");
+      // TODO: copy from setab and replace 48 with 58?


Is this TODO just to avoid code duplication? If so, don't bother :) Otherwise the linter wants TODO(blueyed).

Yes, mainly to avoid duplication, but I have not really checked if it matches the other conditionals.
kitty's docs just said that it works like 38/48, but terminals appear to handle this differently.
It is likely ok though.

justinmk · 2018-09-26T06:26:37Z

src/nvim/tui/tui.c

+    underline = attr & HL_UNDERLINE;
+    undercurl = attr & HL_UNDERCURL;
+  } else {
+    underline = attr & HL_UNDERLINE || attr & HL_UNDERCURL;


I think we normally would use parens around the bitwise operations.

justinmk · 2018-09-26T06:29:10Z

runtime/doc/syntax.txt

@@ -4665,7 +4665,8 @@ cterm={attr-list}			*attr-list* *highlight-cterm* *E418*

 	Note that "bold" can be used here and by using a bold font.  They
 	have the same effect.
-	If running in a terminal, "undercurl" acts as an alias for "underline".
+	"undercurl" falls back to "underline" in a terminal that does not
+        support it.


indent should be <tab> on this line, like the one above it.

justinmk · 2018-09-26T06:31:53Z

src/nvim/tui/tui.c

  } else {
    return a1.cterm_fg_color != a2.cterm_fg_color
      || a1.cterm_bg_color != a2.cterm_bg_color
-      || a1.cterm_ae_attr != a2.cterm_ae_attr;
+      || a1.cterm_ae_attr != a2.cterm_ae_attr
+      || a1.rgb_sp_color != a2.rgb_sp_color;


~~Do we need cterm_sp_color ?~~

Looks like no, this is for guisp. But does it still make sense to use this in the attrs_differ() comparison? Is guisp used in 256-color terminals?

I would expect colored underline is only supported in terminals that support rgb anyway, so we could make this feature exclusive to rgb mode ('termguicolors').

I am not using termguicolors with kitty.

But then you would need to usectermfg/bg to configure text, and guisp to configure underlines. Seems a little weird (in the past cterm* and gui* have been mutually exclusive configurations), though there is no strict reason we need to forbid it.

Yeah.

I am not sure if adding ctermsp makes sense "just" for that though.
And if so, it would be nice to default to guisp then anyway, since this is defined in colorschemes already.

Sure, what we could do then is to check if a1.cterm_ae_attr matches HL_UNDERLINE|HL_UNDERCURL, because otherwise rgb_sp_color has no effect in cterm mode.

justinmk · 2018-09-26T06:53:38Z

Nice @blueyed ! What terminal are you using that supports this? I know of kitty, VTE, mintty.

Some other notes: from https://bugzilla.gnome.org/show_bug.cgi?id=721761

per kovidgoyal/kitty#226 (comment), mintty [and hterm] decided that 4:4 is dotted underline, and 4:5 is dashed.

bfredl · 2018-09-26T07:46:29Z

Nice addition! However, at least kitty seems to also support colored (straight) underlines, and not only undercurl. It would be nice if nvim also could support it. I e when guisp is set, and at least one of underline and undercurl attributes is set, 58 color should be transmitted.

blueyed · 2018-09-26T11:24:38Z

Thanks for the review!

I have tested this with kitty (but still use urxvt mainly), but playing more with kitty again.

decided that 4:4 is dotted underline, and 4:5 is dashed.

Would it make sense to allow configuring this?
After all undercurl is just a special mode of underline.
I do not think there should be underdotted and underdashed additionally, but maybe something like undercurl_mode (a setting)?!

blueyed · 2018-09-26T12:22:21Z

Pushed some fixups.
Also uses guisp for underline now!

I've noticed however that it does not work in tmux for me with kitty.
I've tried wrapping the codes, but without luck: it does not display undercurl mode at all, and printf '\ePtmux;\e\e[4:3m\e\\'; echo foo also does not work from inside tmux in kitty.

diff --git i/src/nvim/tui/tui.c w/src/nvim/tui/tui.c
index 5058f02f3..9c6253ff4 100644
--- i/src/nvim/tui/tui.c
+++ w/src/nvim/tui/tui.c
@@ -1377,6 +1379,7 @@ static void unibi_goto(UI *ui, int row, int col)
       memset(&vars, 0, sizeof(vars)); \
       data->cork = true; \
 retry: \
+      ILOG("out_str: %s", str); \
       unibi_format(vars, vars + 26, str, data->params, out, ui, NULL, NULL); \
       if (data->overflow) { \
         data->bufpos = orig_pos; \
@@ -1840,18 +1843,23 @@ static void augment_terminfo(TUIData *data, const char *term,
   data->unibi_ext.disable_mouse = (int)unibi_add_ext_str(
       ut, "ext.disable_mouse", "\x1b[?1002l\x1b[?1006l");
 
-  if ((int)unibi_add_ext_bool(ut, "Su", 0)) {
+  if ((int)unibi_add_ext_bool(ut, TMUX_WRAP(tmux, "Su"), 0)) {
       data->unibi_ext.enter_undercurl_mode = (int)unibi_add_ext_str(
-          ut, "ext.enter_undercurl_mode", "\x1b[4:3m");
+          ut, "ext.enter_undercurl_mode", TMUX_WRAP(tmux, "\x1b[4:3m"));
       data->unibi_ext.exit_undercurl_mode = (int)unibi_add_ext_str(
-          ut, "ext.exit_underline_mode", "\x1b[4:0m");
+          ut, "ext.exit_underline_mode", TMUX_WRAP(tmux, "\x1b[4:0m"));
       if (has_colon_rgb) {
+      ILOG("SET 1: %d", tmux);
         data->unibi_ext.set_underline_color = (int)unibi_add_ext_str(
-          ut, "ext.set_underline_color", "\x1b[58:2:%p1%d:%p2%d:%p3%dm");
+          ut, "ext.set_underline_color",
+          TMUX_WRAP(tmux, "\x1b[58:2:%p1%d:%p2%d:%p3%dm"));
       } else {
+      ILOG("SET 2: %d", tmux);
         data->unibi_ext.set_underline_color = (int)unibi_add_ext_str(
-          ut, "ext.set_underline_color", "\x1b[58:2:%p1%d:%p2%d:%p3%dm");
+          ut, "ext.set_underline_color",
+          TMUX_WRAP(tmux, "\x1b[58:2:%p1%d:%p2%d:%p3%dm"));
       }
+      ILOG("set_underline_color: %d", data->unibi_ext.set_underline_color);
   }
 }

blueyed · 2018-09-26T12:35:16Z

Does not work in gnome-terminal (vte) inside of tmux, too - with and without the wrapping.
Funnily gnome-terminal handles printf '\ePtmux;\e\e[4:3m\e\\'; echo foo outside of tmux.

bfredl · 2018-09-27T11:05:14Z

I guess it doesn't work in tmux for the reason that tmux manages text and attributes internally in its own screen buffers. I e if you send a wrapped attribute and then some text, tmux makes no guarantee that text will be transmitted to the underlying terminal right after the wrapped escape code. Rather, what is needed is a patch to tmux to store the undercurl attribute in its own datastructure, so tmux can draw (and also redraw when switching windows etc) it reliably.

bfredl · 2018-09-27T11:15:02Z

src/nvim/tui/tui.c

@@ -521,6 +530,16 @@ static void update_attrs(UI *ui, HlAttrs attrs)
  if (italic) {
    unibi_out(ui, unibi_enter_italics_mode);
  }
+  if (undercurl || (underline && data->unibi_ext.enter_undercurl_mode)) {


underline && data->unibi_ext.enter_undercurl_mode looks a bit weird. Is the set of terminals which support 4:3 code identical to set of terminals that supports 58 color?

Regardless this could be changed to (undercurl || underline) && data->unibi_ext.set_underline_color (and undercurl only check need not be nested) in case the detection code becomes more flexible (for terminals that support one but not the other). Also shouldn't attrs.rgb_sp_color != -1 be checked?

Is the set of terminals which support 4:3 code identical to set of terminals that supports 58 color?

That's what I've assumed, but only based on https://sw.kovidgoyal.net/kitty/protocol-extensions.html?highlight=extensions#colored-and-styled-underlines.

justinmk · 2018-09-30T16:42:25Z

Let's not block this for tmux. I think the only remaining change is @bfredl's comment(s).

blueyed · 2018-09-30T18:08:54Z

Ok. I've created tmux/tmux#1492 for tmux, and pushed two fixups.

bfredl · 2018-09-30T18:22:50Z

src/nvim/tui/tui.c

+        UNIBI_SET_NUM_VAR(data->params[2], color & 0xff);          // blue
+        unibi_out_ext(ui, data->unibi_ext.set_underline_color);
+    }
+    unibi_out_ext(ui, data->unibi_ext.enter_undercurl_mode);


I would move this in front (just after if (italic)), in its own if-statement.

bfredl · 2018-09-30T18:23:54Z

src/nvim/tui/tui.c

@@ -521,6 +531,16 @@ static void update_attrs(UI *ui, HlAttrs attrs)
  if (italic) {
    unibi_out(ui, unibi_enter_italics_mode);
  }
+  if ((undercurl || underline) && data->unibi_ext.enter_undercurl_mode) {


set_underline_color

bfredl · 2018-09-30T18:24:58Z

src/nvim/tui/tui.c

@@ -521,6 +531,16 @@ static void update_attrs(UI *ui, HlAttrs attrs)
  if (italic) {
    unibi_out(ui, unibi_enter_italics_mode);
  }
+  if ((undercurl || underline) && data->unibi_ext.enter_undercurl_mode) {
+    int color = attrs.rgb_sp_color;
+    if (color) {


color != -1

blueyed · 2018-09-30T18:31:41Z

Fixed the last three comments from @bfredl and squashed/rebased it.

bfredl · 2018-10-01T09:00:36Z

runtime/doc/syntax.txt

@@ -4665,7 +4665,8 @@ cterm={attr-list}			*attr-list* *highlight-cterm* *E418*

 	Note that "bold" can be used here and by using a bold font.  They
 	have the same effect.
-	If running in a terminal, "undercurl" acts as an alias for "underline".
+	"undercurl" falls back to "underline" in a terminal that does not
+	support it.
 	It is set using |highlight-guisp|.


The color is set using |highlight-guisp|. (also remove linebreak)

bfredl · 2018-10-01T09:01:06Z

LGTM except lint failure + comment above.

Fixes neovim#7479.

blueyed · 2018-10-02T12:31:48Z

The problem with tmux is that unibi_add_ext_bool(ut, "Su", 0) reports support, but then it does not work there - i.e. it ends up without any decorations, but should at least be underlined maybe? (like in urxvt)

bfredl · 2018-10-02T13:09:07Z

So the issue is that tmux own terminfo is wrong? Ideally this is something then that tmux should fix, but if the terminfo is wrong in many existing versions on many distros, I suppose we could add an extra rule. But I agree with @justinmk, lets merge this for now, we could make a decision when we have got more feedback from tmux users.

blueyed · 2018-10-02T14:04:38Z

No, the code here was wrong - tried to push a fixup, but it did not show up anymore:

commit ec19843de (HEAD -> undercurl, blueyed/undercurl)                                                                                                                                         Author: Daniel Hahler <git@thequod.de>                                                                                                                                                          
Date:   Tue Oct 2 15:59:18 2018 +0200                                                                                                                                                                                                                                                                                                                                                           
    fixup! tui: add support for undercurl and underline color                                                                                                                                                                                                                                                                                                                                   
diff --git a/src/nvim/tui/tui.c b/src/nvim/tui/tui.c                                                                                                                                            index db01c0b3c..ffbc2d275 100644                                                                                                                                                               
--- a/src/nvim/tui/tui.c                                                                                                                                                                        +++ b/src/nvim/tui/tui.c                                                                                                                                                                        
@@ -1439,6 +1439,18 @@ static int unibi_find_ext_str(unibi_term *ut, const char *name)                                                                                                             return -1;                                                                                                                                                                                   
 }                                                                                                                                                                                                                                                                                                                                                                                              
+static int unibi_find_ext_bool(unibi_term *ut, const char *name)                                                                                                                               +{                                                                                                                                                                                              
+  size_t max = unibi_count_ext_bool(ut);                                                                                                                                                       +  for (size_t i = 0; i < max; i++) {                                                                                                                                                           
+    const char * n = unibi_get_ext_bool_name(ut, i);                                                                                                                                           +    if (n && 0 == strcmp(n, name)) {                                                                                                                                                           
+      return (int)i;                                                                                                                                                                           +    }                                                                                                                                                                                          
+  }                                                                                                                                                                                            +  return -1;                                                                                                                                                                                   
+}                                                                                                                                                                                              
+                                                                                                                                                                                               
 /// Patches the terminfo records after loading from system or built-in db.                                                                                                                     
 /// Several entries in terminfo are known to be deficient or outright wrong;                                                                                                                   
 /// and several terminal emulators falsely announce incorrect terminal types.                                                                                                                  
@@ -1840,7 +1852,8 @@ static void augment_terminfo(TUIData *data, const char *term,                                                                                                             
   data->unibi_ext.disable_mouse = (int)unibi_add_ext_str(                                                                                                                                      
       ut, "ext.disable_mouse", "\x1b[?1002l\x1b[?1006l");                                                                                                                                      
                                                                                                                                                                                                
-  if ((int)unibi_add_ext_bool(ut, "Su", 0)) {                                                                                                                                                  
+  int ext_bool_Su = unibi_find_ext_bool(ut, "Su");                                                                                                                                             
+  if (ext_bool_Su != -1 && unibi_get_ext_bool(ut, (size_t)ext_bool_Su)) {                                                                                                                      
       data->unibi_ext.enter_undercurl_mode = (int)unibi_add_ext_str(                                                                                                                           
           ut, "ext.enter_undercurl_mode", "\x1b[4:3m");                                                                                                                                        
       data->unibi_ext.exit_undercurl_mode = (int)unibi_add_ext_str(

blueyed · 2018-10-02T14:09:38Z

=> #9080

neovim/neovim#9052 (comment)

Besides the "visible" improvements, this release features numerous internal improvements to the UI/screen code and test infrastructure. Numerous patches were merged from Vim, which are not mentioned below. FEATURES: 07ad5d7 clipboard: Support custom VimL functions neovim#9304 725da1f neovim#9401 win/TUI: Improve terminal/console support 7a8dadb neovim#9077 startup: Use $XDG_CONFIG_DIRS/nvim/sysinit.vim if it exists feec926 neovim#9299 support <cmd> mapping in more places 0653ed6 neovim#9028 diff/highlight: Show underline for low-priority CursorLine bddcbbb signs: Add "numhl" argument neovim#9113 05f9c7c clipboard: support Wayland (neovim#9230) 14ae394 neovim#9052 TUI: add support for undercurl and underline color 4fa3492 neovim#9023 man.vim: soft (dynamic) wrap neovim#9023 API: 8b39e4e neovim#6920 API: implement object namespaces b1aaa0a API: Implement nvim_win_set_buf() neovim#9100 8de87c7 neovim#8180 API: virtual text annotations (nvim_buf_set_virtual_text) 2b9fc9a neovim#8660 API: add nvim_buf_is_loaded() API: buf_get_lines, buf_line_count handle unloaded buffers 88f77c2 API: nvim_buf_get_offset_for_line 94841e5 API/UI: neovim#8221 ext_newgrid, ext_hlstate (use line-based rather than char-based updates) UI b5cfac0 neovim#8806 TUI: use BCE again more often, (smoother resizes/scrolling) 77b5e9a neovim#9315 screen: add missing status redraw when redraw_later(CLEAR) was used 5f15788 TUI: clip invalid regions on resize (neovim#8779), fixes neovim#8774 c936ae0 neovim#9193 TUI: improvements for scrolling and clearing f204274 neovim#9143 UI: disable clearing almost everywhere f4b2b66 neovim#9079 TUI: always use safe cursor movement after resize d36afaf neovim#9211 ui_options: also send when starting or from OptionSet 67f80d4 TUI: Avoid reset_cursor_color in old VTE neovim#9191 e55ebae neovim#9021 don't erase screen on `:hi Normal` during startup c5790d9 neovim#8915 TUI: Hint wrapped lines to terminals. FIXES: 231de72 RPC: turn errors from async calls into notifications 907ad92 TUI: Restore terminal title via "title stacking" (neovim#9407) cb76a8a genappimage: Unset $ARGV0 at invocation neovim#9376 b48efd9 neovim#9347 TUI: FreeBSD: Improve support for BSD vt console c16529a TUI: Konsole 18.07.70 supports DECSCUSR (neovim#9364) aec096f os/lang: use the correct LC_NUMERIC also for OS X 5fee0be provider: improve error message (neovim#9344) 3c42d7a TUI: alacritty supports set_cursor_color neovim#9353 7bff9a5 TUI: Alacritty supports DECSCUSR (neovim#9048) 57acfce macOS: infer primary language if $LANG is empty neovim#9345 bc132ae runtime/syntax: Fix highlighting of augroup contents (neovim#9328) 715fdfe neovim#9297 VimL/confirm(): Show dialog even if :silent 799d9c3 clipboard: Prefer xclip (neovim#9302) 6dae777 provider/nodejs: fix npm,yarn detection 16bc1e9 neovim#9218 channel: avoid buffering output when only terminal and no callbacks are active 72fecad neovim#8804 Fix crash in lang_init() on macOS if lang_region = NULL d581398 ruby: detect rbenv shims for other versions (neovim#8733) e568ac7 neovim#9123 third-party/unibilium: Fix parsing of extended capability entries c4c74c3 jobstart(): Fix hang on non-executable cwd neovim#9204 1cf50cb provider/nodejs: Simultaneously query npm and yarn neovim#9054 6c496db undo: Fix infinite loop if undo_read_byte returns EOF neovim#2880 f8f8357 neovim#9034 'swapfile: always show dialog' CHANGES: c236e80 neovim#9024 --embed: wait for UI unless --headless 180b50d neovim#9248 python: 'neovim' module was renamed to 'pynvim' 2000b6a neovim#8589 VimL: Remove legacy aliases "v:errmsg", "v:shell_error", "v:this_session" deb18a0 defaults: background=dark neovim#2894 (neovim#9205) c1187d4 defaults: win: 'shellpipe' for cmd.exe (neovim#8827)

janlazo · 2020-03-05T03:19:07Z

Are 8.0.1358 and 8.0.1422 N/A because of this PR?

blueyed · 2020-03-05T06:02:13Z

@janlazo not necessarily only because of this PR, but also because the highlighting code differs in general IIRC (with regard to 8.0.1422).

marvim added the RFC label Sep 25, 2018

justinmk reviewed Sep 26, 2018

View reviewed changes

blueyed mentioned this pull request Sep 26, 2018

tui: support styled and colored underlines/undercurl #7479

Closed

bfredl reviewed Sep 27, 2018

View reviewed changes

blueyed mentioned this pull request Sep 30, 2018

Undercurl (special underline) tmux/tmux#1492

Closed

bfredl reviewed Sep 30, 2018

View reviewed changes

blueyed force-pushed the undercurl branch from ea83904 to 5d16053 Compare September 30, 2018 18:30

bfredl reviewed Oct 1, 2018

View reviewed changes

tui: add support for undercurl and underline color

c4006a6

Fixes neovim#7479.

blueyed force-pushed the undercurl branch from 5d16053 to c4006a6 Compare October 1, 2018 14:27

bfredl merged commit 14ae394 into neovim:master Oct 2, 2018

justinmk removed the RFC label Oct 2, 2018

fmoralesc mentioned this pull request Oct 4, 2018

Regression for undercurl in gnome-terminal #9083

Closed

kristijanhusak added a commit to kristijanhusak/neovim-config that referenced this pull request Oct 10, 2018

Use underline for SpellBad until it's fixed for Neovim in tmux.

52aebac

neovim/neovim#9052 (comment)

justinmk mentioned this pull request Nov 28, 2018

Underline not displayed in TUI #9270

Closed

gerion0 mentioned this pull request Mar 2, 2020

support undercurls for TERM=kitty (and TERM=xterm-kitty) #11973

Closed

bluz71 mentioned this pull request Jul 26, 2020

Feature request: undercurl support alacritty/alacritty#1628

Closed

Uh oh!

[RFC] tui: add support for undercurl #9052

[RFC] tui: add support for undercurl #9052

Uh oh!

Conversation

blueyed commented Sep 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justinmk Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Sep 26, 2018

Uh oh!

bfredl commented Sep 26, 2018

Uh oh!

blueyed commented Sep 26, 2018

Uh oh!

blueyed commented Sep 26, 2018

Uh oh!

blueyed commented Sep 26, 2018

Uh oh!

bfredl commented Sep 27, 2018

Uh oh!

bfredl Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmk commented Sep 30, 2018

Uh oh!

blueyed commented Sep 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed commented Sep 30, 2018

Uh oh!

bfredl Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfredl commented Oct 1, 2018

Uh oh!

blueyed commented Oct 2, 2018

Uh oh!

bfredl commented Oct 2, 2018

Uh oh!

blueyed commented Sep 25, 2018 •

edited

Loading

justinmk Sep 26, 2018 •

edited

Loading

justinmk Sep 26, 2018 •

edited

Loading

justinmk Sep 26, 2018 •

edited

Loading

bfredl Sep 27, 2018 •

edited

Loading

bfredl Oct 1, 2018 •

edited

Loading