From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Support 24-bit terminal colors. Date: Wed, 31 Aug 2016 17:21:35 +0300 Message-ID: <83wpixkoi8.fsf@gnu.org> References: <1472587906-29711-1-git-send-email-rjy@iki.fi> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1472653327 11370 195.159.176.226 (31 Aug 2016 14:22:07 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 31 Aug 2016 14:22:07 +0000 (UTC) Cc: emacs-devel@gnu.org To: Rami =?utf-8?Q?Ylim=C3=A4ki?= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Aug 31 16:21:58 2016 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bf6Oo-0001r0-I3 for ged-emacs-devel@m.gmane.org; Wed, 31 Aug 2016 16:21:54 +0200 Original-Received: from localhost ([::1]:54393 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6Om-0002fk-8j for ged-emacs-devel@m.gmane.org; Wed, 31 Aug 2016 10:21:52 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48419) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6Oe-0002fe-0C for emacs-devel@gnu.org; Wed, 31 Aug 2016 10:21:46 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bf6OX-00073T-QE for emacs-devel@gnu.org; Wed, 31 Aug 2016 10:21:42 -0400 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:44092) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bf6OX-000735-MP; Wed, 31 Aug 2016 10:21:37 -0400 Original-Received: from 84.94.185.246.cable.012.net.il ([84.94.185.246]:2861 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.82) (envelope-from ) id 1bf6OV-0002kS-Mn; Wed, 31 Aug 2016 10:21:36 -0400 In-reply-to: <1472587906-29711-1-git-send-email-rjy@iki.fi> (message from Rami =?utf-8?Q?Ylim=C3=A4ki?= on Tue, 30 Aug 2016 23:11:46 +0300) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:207026 Archived-At: > From: Rami Ylimäki > 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.