Re: [PATCH] Support true colours via termcap interface

[Daniel Tameling <tamelingdaniel@gmail.com>]

On Mon, Feb 04, 2019 at 09:11:04AM +0100, Sebastian Gniazdowski wrote:
> On Mon, 4 Feb 2019 at 07:20, Daniel Tameling <tamelingdaniel@gmail.com> wrote:
> > Btw. I did that deliberately. I didn't want to make the if even more
> > complicated and I wanted to use the same code path as for the
> > %F{16763955} syntax. Making a small change to match_colour felt nicer
> > than to hack around in set_colour_attribute.
> 
> Ahso,,ok, but I don't see the *_TERMCAP code being returned here,
> could you explain?

The code doesn't return from the function but continues and enters
if (!named && tccan(tc)) {
and there the else branch where
on |= is_fg ? TXT_ATTR_FG_TERMCAP : TXT_ATTR_BG_TERMCAP;
is executed. This is the same as for the %F{16763955} syntax.

> > Before the patch, the hardcoded escape sequence is used:
> > $ TERM=xterm-direct print -P %F{'#ffcc00'} | cat -v
> > ^[[38;2;255;204;0m
> >
> > Afterwards the terminfo entry is used:
> > $ TERM=xterm-direct print -P %F{'#ffcc33'} | cat -v
> > ^[[38:2::255:204:51m
> >
> > The former works because xterm understands both escape sequences, but
> 
> Ahso, OK. Let me just point the difference: the second code has two
> clons after "38:2".
> 

There are currently three definitions floating around (and also
present in ncurses' terminfo):
\e38;2;red;green;blue;m
\e38:2:red:green:blue:m
\e38:2::red:green:blue:m
The comments in ncurses terminfo briefly describe the history of these
sequences [1], which boils down to the following: The first one is a
natural extension of xterm's 256 colour sequences. The last one is
specified in a document from the nineties that tried to standardize
terminals. The second one stems from overlooking the a colour space
identifier in the document. If you want to know more about this you
should look at [2] and the comments to [3]. There is currently an
argument, whether the first or the last sequence should be used, but
it seems that at the moment all terminal support the syntax with
semicolons and some support additionally the one with colons.

[1]: https://invisible-island.net/ncurses/terminfo.src.html#tic-xterm_direct2
[2]: https://bugs.kde.org/show_bug.cgi?id=107487
[3]: https://gist.github.com/XVilka/8346728

> 
> That said: does the new X04 test pass with your patch?
> 

Yes, it does. But I noticed that's misleading as the test sets TERM to
xterm-256color. If I set it to xterm-direct the test fails even
without the patch
--- /tmp/zsh.ztst.75451/ztst.out        2019-02-04 20:11:21.000000000
+0100
+++ /tmp/zsh.ztst.75451/ztst.tout       2019-02-04 20:11:21.000000000
+0100
@@ -1 +1 @@
-0m27m24mCDE|3232|trueCDE|39|
+0m27m24mtrue
Test ./X04zlehighlight.ztst failed: output differs from expected as
shown above for:
zpty_start
zpty_input 'zmodload zsh/nearcolor'
zpty_input 'rh_widget() { BUFFER="true"; region_highlight+=( "0 4 fg=#040810" ); }'
zpty_input 'zle -N rh_widget'
zpty_input 'bindkey "\C-a" rh_widget'
zpty_enable_zle
zpty_input $'\C-a'  # emits newline, which executes BUFFER="true" command
zpty_line 1 p       # the line of interest, preserving escapes ("p")
zpty_stop
Was testing: basic region_highlight with near-color (hex-triplets at input)

But I also noticed that my patch breaks the is_default_zle_highlight =
0 code. It doesn't set use_truecolor for the #ffcc00 syntax. But that
is not the only problem: with a true colour terminal, one expects
colours beyond 255 to work, e.g. via fg=273. In that case one also has
to use the true colour escape sequence. Then also colours in the range
from 8 to 255 should use this sequence. But colours from 0 to 7 should
probably still be the standard ansi sequence, like ncurses does it.

One could achieve that by modifying the if(use_truecolor) checks when
the escape sequence is constructed manually. Alternatively, the patch
below should accomplish the same. Now zlehighlight.ztst fails at the
same point as without the patch for xterm-direct. It seems that this
is because the non-termcap code emits only the standard true colour
escape sequence, which cannot be modified like the other ones.

-- 
Best regards,
Daniel


diff --git a/Src/prompt.c b/Src/prompt.c
index f2b3f161e..bef53b148 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1652,6 +1652,13 @@ match_colour(const char **teststrp, int is_fg, int colour)
 	    colour = runhookdef(GETCOLORATTR, &color) - 1;
 	    if (colour == -1) { /* no hook function added, try true color (24-bit) */
 		colour = (((color.red << 8) + color.green) << 8) + color.blue;
+                /*
+                 * If we have a true colour termcap entry and colour > 7
+                 * use termcap; for colours 0-7 termcap usually emits the
+                 * standard ANSI sequences; we don't want that.
+                 */
+                if (tccolours == 0x1000000 && colour > 7)
+                        on |= is_fg ? TXT_ATTR_FG_TERMCAP : TXT_ATTR_BG_TERMCAP;
 		return on | (is_fg ? TXT_ATTR_FG_24BIT : TXT_ATTR_BG_24BIT) |
 			(zattr)colour << shft;
 	    } else if (colour <= -2) {
@@ -1668,7 +1675,7 @@ match_colour(const char **teststrp, int is_fg, int colour)
 	}
 	else {
 	    colour = (int)zstrtol(*teststrp, (char **)teststrp, 10);
-	    if (colour < 0 || colour >= 256)
+	    if (colour < 0 || colour >= tccolours)
 		return TXT_ERROR;
 	}
     }
@@ -1692,6 +1699,16 @@ match_colour(const char **teststrp, int is_fg, int colour)
 	     */
 	    on |= is_fg ? TXT_ATTR_FG_TERMCAP :
 		TXT_ATTR_BG_TERMCAP;
+            /*
+             * If our terminal supports more than 256 colours it is
+             * most likely a true colour terminal (it's not always
+             * 256*256*256 colours: sometimes tccolours get truncated
+             * to the largest short, which is significantly smaller);
+             * if we don't use termcap, we want to emit true colour
+             * escape sequences for colour > 7.
+             */
+            if (tccolours > 256 && colour > 7)
+                    on |= is_fg ? TXT_ATTR_FG_24BIT : TXT_ATTR_BG_24BIT;
 	}
     }
     return on | (zattr)colour << shft;
@@ -2039,8 +2056,7 @@ set_colour_attribute(zattr atr, int fg_bg, int flags)
      * highlighting variables, so much of this shouldn't be
      * necessary at this point, but we might as well be safe.
      */
-    if (!def && !use_truecolor &&
-	(is_default_zle_highlight && (colour > 7 || use_termcap)))
+    if (!def && (is_default_zle_highlight && (colour > 7 || use_termcap)))
     {
 	/*
 	 * We can if it's available, and either we couldn't get