Re: [PATCH] Support 24-bit terminal colors.

[Eli Zaretskii]

> From: Rami Ylimäki <address@hidden>
> Date: Tue, 30 Aug 2016 23:11:46 +0300
> 
> Based on previous work by Rüdiger Sonderfeld, Christian Hopps and
> Charles Strahan.

Some of these people don't have copyright assignments on file, so how
much of the code is actually taken from what they wrote?

I also don't find your assignment, but maybe once you adjudicate the
comments below, the number of changes will be small enough to accept
without an assignment.  Nevertheless, I encourage you to start your
legal paperwork rolling, so that we could continue receiving your
contributions even if this one doesn't need it.

> Currently it's not possible to detect whether terminal supports 24-bit
> colors. Therefore following methods can be used to force 24-bit mode on:
> 
> emacs -nw --color=rgb
> emacsclient -nw -F "((tty-color-mode . rgb))"

That is a screw.  Is there really no way of detecting true-color
support?  Not even a way that's specific to each kind of terminal
emulator that supports this mode?

Asking the user to specify a command-line argument each time is so
annoying that I'm tempted to say this feature is not yet ready for a
complex multi-terminal program such as Emacs, and we should wait for
some kind of standard to emerge before we dump this on our users.

Can we devise some customizable data structure that stores names of
terminals that support true-color?  Then users could set this list
from their ~/.emacs init files, and have any frame open on those
terminals automatically support 24-bit colors.  (If we go this way,
the data structure will have to be consulted by startup.el at the
right place, so that the initial frame is already set up correctly.)

In any case, if we do agree to specify this via a command-line
argument, the same command-line argument should IMO be supported by
emacsclient; asking users to use Lisp is less user-friendly, IMO.

> * lisp/server.el (server-process-filter): Pass 'tty-color-mode frame
>   parameter from command line to server-create-tty-frame.
>   (server-create-tty-frame): Pass 'tty-color-mode frame parameter to
>   make-frame.

This part is IMO wrong: the true-color support is a property of the
terminal, not of a frame.  So we should use terminal-parameters for
communicating the support, and each frame on that terminal should
thereafter automatically be set to use 24-bit colors.

> diff --git a/lisp/term/tty-colors.el b/lisp/term/tty-colors.el
> index a886950..b9b69cd 100644
> --- a/lisp/term/tty-colors.el
> +++ b/lisp/term/tty-colors.el
> @@ -764,7 +764,8 @@
>      (auto . 0)
>      (ansi8 . 8)
>      (always . 8)
> -    (yes . 8))
> +    (yes . 8)
> +    (rgb . 16777216))
>    "An alist of supported standard tty color modes and their aliases.")

I think 'rgb' is too general to be useful mnemonically.  How about
'24bit' or 'true-color' instead?

> +(defun xterm-rgb-support-p ()
> +  "Check whether 24-bit colors have been forced on."
> +  (or
> +   ;; emacs -nw --color=rgb
> +   (eql (cdr (assoc 'tty-color-mode default-frame-alist)) 'rgb)
> +   ;; emacsclient -nw -F "((tty-color-mode . rgb))"
> +   (eql (cdr (assoc 'tty-color-mode (frame-parameters))) 'rgb)))

Not sure why this function is needed.  Why not simply look at the
number of supported colors, i.e. what display-color-cells returns?

>  (defun xterm-register-default-colors (colors)
>    "Register the default set of colors for xterm or compatible emulator.
>  
> @@ -930,6 +938,14 @@ versions of xterm."
>      ;; are more colors to support, compute them now.
>      (when (> ncolors 0)
>        (cond
> +       ((xterm-rgb-support-p) ; 24-bit xterm
> +       ;; all named tty colors
> +       (let ((idx (length xterm-standard-colors)))
> +         (mapc (lambda (color)
> +                 (unless (assoc (car color) xterm-standard-colors)
> +                   (tty-color-define (car color) idx (cdr color))
> +                   (setq idx (1+ idx))))
> +               color-name-rgb-alist)))

Why don't you use the RGB spec of the color, encoded in an integer as
the value of IDX here?  Won't that make your job easier in other
places, and avoid special-casing the true-color terminal?

> +/* Color index bit indicating absence of palette.  */
> +#define FACE_TTY_NONINDEXED_COLOR ((unsigned long) (1 << 24))

Once again, not sure why this bit is needed, since you have the number
of colors that can be used for the same purpose.

> +static char *
> +tparam_color (struct tty_display_info *tty, unsigned long color, bool 
> is_foreground)
> +{
> +  const char *ts = is_foreground
> +                  ? tty->TS_set_foreground
> +                  : tty->TS_set_background;
> +  return ts ? tparam (ts, NULL, 0, color, 0, 0, 0) : NULL;
> +}
> +
> +static char *
> +tparam_rgb (struct tty_display_info *tty, unsigned long color, bool 
> is_foreground)
> +{
> +  const char *ts = is_foreground
> +                  ? tty->TS_set_rgb_foreground
> +                  : tty->TS_set_rgb_background;
> +  const int red = (color >> 16) & 0xFF;
> +  const int green = (color >> 8) & 0xFF;
> +  const int blue = color & 0xFF;
> +  return ts ? tparam (ts, NULL, 0, red, green, blue, 0) : NULL;
> +}
> +
> +static void
> +turn_on_color (struct tty_display_info *tty, unsigned long color, bool 
> is_foreground)
> +{
> +  if (face_tty_specified_color (color))
> +    {
> +      char *p = color & FACE_TTY_NONINDEXED_COLOR
> +               ? tparam_rgb (tty, color, is_foreground)
> +               : tparam_color (tty, color, is_foreground);
> +      if (p)
> +       {
> +         OUTPUT (tty, p);
> +         xfree (p);
> +        }
> +    }
> +}
> +
>  /* Turn appearances of face FACE_ID on tty frame F on.
>     FACE_ID is a realized face ID number, in the face cache.  */
> 
> @@ -1930,24 +1967,8 @@ turn_on_face (struct frame *f, int face_id)
> 
>    if (tty->TN_max_colors > 0)
>      {
> -      const char *ts;
> -      char *p;
> -
> -      ts = tty->standout_mode ? tty->TS_set_background : 
> tty->TS_set_foreground;
> -      if (face_tty_specified_color (fg) && ts)
> -       {
> -          p = tparam (ts, NULL, 0, fg, 0, 0, 0);
> -         OUTPUT (tty, p);
> -         xfree (p);
> -       }
> -
> -      ts = tty->standout_mode ? tty->TS_set_foreground : 
> tty->TS_set_background;
> -      if (face_tty_specified_color (bg) && ts)
> -       {
> -          p = tparam (ts, NULL, 0, bg, 0, 0, 0);
> -         OUTPUT (tty, p);
> -         xfree (p);
> -       }
> +      turn_on_color (tty, fg, !tty->standout_mode);
> +      turn_on_color (tty, bg, tty->standout_mode);
>      }
>  }

Is this refactoring really needed?  You now have two extra function
call levels, in a function that is called _a_lot_ in the inner-most
loops of the display engine.

AFAIU, the only difference in the true-color case is that the 3 last
arguments of tparam need to be computed by bitwise operations rather
than set to zero.  Adding 2 extra levels of indirection just for that
sounds excessive to me.  I'd keep that inline.

> @@ -2067,8 +2089,9 @@ tty_default_color_capabilities (struct tty_display_info 
> *tty, bool save)
>        dupstring (&default_orig_pair, tty->TS_orig_pair);
>        dupstring (&default_set_foreground, tty->TS_set_foreground);
>        dupstring (&default_set_background, tty->TS_set_background);
> +      dupstring (&default_set_rgb_foreground, tty->TS_set_rgb_foreground);
> +      dupstring (&default_set_rgb_background, tty->TS_set_rgb_background);

Why do we need 2 new attributes here?  Can't we reuse
TS_set_foreground and TS_set_background instead?  They seem to be
unused in the 24-bit color case.

> -      default_max_pairs = tty->TN_max_pairs;
[...]
> @@ -4137,7 +4175,6 @@ use the Bourne shell command 'TERM=...; export TERM' 
> (C-shell:\n\
>         }
 
>        tty->TN_max_colors = tgetnum ("Co");
> -      tty->TN_max_pairs = tgetnum ("pa");
[...] 
> -  /* "pa" -- max. number of color pairs on screen.  Not handled yet.
> -     Could be a problem if not equal to TN_max_colors * TN_max_colors.  */
> -  int TN_max_pairs;
> -

If deletion of this attribute is not necessary for the patch to work,
it should be done in a separate commit.

> +      case 16777216:
> +       tty->TS_orig_pair = "\033[0m";
> +       tty->TS_set_foreground = tty->TS_set_background = NULL;
> +#ifdef TERMINFO
> +       tty->TS_set_rgb_foreground = "\033[38;2;%p1%d;%p2%d;%p3%dm";
> +       tty->TS_set_rgb_background = "\033[48;2;%p1%d;%p2%d;%p3%dm";
> +#else
> +       tty->TS_set_rgb_foreground = "\033[38;2;%d;%d;%dm";
> +       tty->TS_set_rgb_background = "\033[48;2;%d;%d;%dm";
> +#endif
> +       tty->TN_max_colors = 16777216;
> +       tty->TN_no_color_video = 0;
> +        break;

Do all true-color terminals support the same escape sequences for
24-bit colors?  Is there any standard documents we could rely upon for
hard-coding them here?
 
> +static bool
> +tty_supports_rgb (struct frame *f)
> +{
> +  return f->output_method == output_termcap
> +        && f->output_data.tty->display_info->TS_set_rgb_foreground
> +        && f->output_data.tty->display_info->TS_set_rgb_background;
> +}

Once again, why not just look at the number of supported colors?

> +static bool
> +parse_and_encode_rgb_list (Lisp_Object rgb_list, XColor *color)
> +{
> +  if (!parse_rgb_list (rgb_list, color))
> +    return false;
> +
> +  color->pixel = FACE_TTY_NONINDEXED_COLOR
> +                | (color->red / 256) << 16
> +                | (color->green / 256) << 8
> +                | (color->blue / 256);
> +  return true;
> +}
> +
> +
> +static bool
> +tty_lookup_rgb (Lisp_Object color, XColor *tty_color, XColor *std_color)
> +{
> +  if (NILP (Ffboundp (Qtty_color_standard_values)))
> +    return false;
> +
> +  if (!NILP (Ffboundp (Qtty_color_canonicalize)))
> +    color = call1 (Qtty_color_canonicalize, color);
> +
> +  color = call1 (Qtty_color_standard_values, color);
> +  if (!parse_and_encode_rgb_list (color, tty_color))
> +    return false;
> +
> +  if (std_color)
> +    *std_color = *tty_color;
> +
> +  return true;
> +}
> 
>  /* Lookup on frame F the color described by the lisp string COLOR.
>     The resulting tty color is returned in TTY_COLOR; if STD_COLOR is
> @@ -844,6 +883,9 @@ tty_lookup_color (struct frame *f, Lisp_Object color, 
> XColor *tty_color,
>  {
>    Lisp_Object frame, color_desc;
> 
> +  if (tty_supports_rgb (f))
> +    return tty_lookup_rgb (color, tty_color, std_color);
> +
>    if (!STRINGP (color) || NILP (Ffboundp (Qtty_color_desc)))
>      return false;
> 
> @@ -5707,8 +5749,12 @@ map_tty_color (struct frame *f, struct face *face,
>           CONSP (def)))
>      {
>        /* Associations in tty-defined-color-alist are of the form
> -        (NAME INDEX R G B).  We need the INDEX part.  */
> -      pixel = XINT (XCAR (XCDR (def)));
> +        (NAME INDEX R G B).  We need the (R G B) or INDEX part.  */
> +      XColor tty_color;
> +      if (tty_supports_rgb (f) && parse_and_encode_rgb_list (XCDR (XCDR 
> (def)), &tty_color))
> +       pixel = tty_color.pixel;
> +      else
> +       pixel = XINT (XCAR (XCDR (def)));
>      }

Sorry, I don't understand why this complexity is needed.  AFAIU, a
true-color terminal can produce color directly from its RGB
components, without the need to look up that RGB descriptor via
tty-color-desc.  In addition, support for named colors, such as
"burlywood1", is still required for these terminals, since most color
specifications in Emacs are of that variety.  Is my understanding
correct?

If I'm right, then you could either (a) extend tty-color-desc, such
that it would cons the (NAME INDEX R G B) list for a true-color
terminal and hand it back to tty_lookup_color; or (b) if you wanted to
be more efficient, generate the pixel value directly on the C level by
parsing the color spec if it is of the form "#RRGGBB", and otherwise
calling tty-color-desc as usual.  Assuming that tty-color-alist is set
correctly for the true-color terminal, and stores the RGB values for
each color, that will do the job, eliminating the need for all these
support functions: parse_and_encode_rgb_list, tty_supports_rgb, and
tty_lookup_rgb, and also save you 2 extra calls to Lisp.  Am I missing
something?

Thanks.