Package: emacs;
Reported by: Ewan <ewan <at> etown.dev>
Date: Sun, 24 Aug 2025 06:07:02 UTC
Severity: normal
Merged with 79297
Message #8 received at 79298 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Ewan <ewan <at> etown.dev> Cc: 79298 <at> debbugs.gnu.org Subject: Re: bug#79298: patch: full color in windows terminal Date: Sun, 24 Aug 2025 10:20:08 +0300
merge 79298 79297
thanks
> Date: Sun, 24 Aug 2025 02:27:35 +0000
> From: Ewan via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> * configure.ac: Compiler flag for setting w32 console color space.
> * lisp/term/w32console.el: Define and apply different color spaces.
> * src/term.c: Set tty max colors based on compiler flag.
> * src/w32console.c: Write face colors to console via VT sequences.
Thanks. Such a large contribution will need you to sign a
copyright-assignment agreement with the FSF. If you agree to do it, I
will send you the form to fill and the instructions to go with it.
> --- a/configure.ac
> +++ b/configure.ac
> @@ -299,6 +299,21 @@ AC_ARG_WITH([all],
> [with_features=$withval],
> [with_features=yes])
>
> +AC_ARG_WITH([w32-vt-color],
> +[AS_HELP_STRING([--with-w32-vt-color=ARG],
> +[use VT sequences for w32 console color (ARG one of: no, 16, 156, 24bit)])],
> +[ case "${withval}" in
> + no ) val=no ;;
> + 16 ) val=16 ;;
> + 256 ) val=256 ;;
> + 24bit ) val=24bit ;;
> + * )
> + AC_MSG_ERROR(['--with-vt-color=$withval' is invalid;
> + the value should be 'no', '16', '256', or '24bit']) ;;
> + esac
> + with_w32_vt_color=$val
> +])
Why do we need this configure-time switch? why not compile this
feature into Emacs unconditionally?
> +(defun w32con-define-256-colors ()
> + "Defines 256 color space"
The first line of a doc string should be a single complete sentence,
and must end in a period (here and elsewhere in the patch).
> + ;; Define the color space
> + (tty-color-clear)
> + (let ((ncolors (display-color-cells)))
> + (cond ((= ncolors 16777216) (w32con-define-24bit-colors))
> + ((= ncolors 265 (w32con-define-256-colors)))
> + (t (w32con-define-base-colors))))
> (clear-face-cache)
> ;; Figure out what are the colors of the console window, and set up
> ;; the background-mode correspondingly.
> diff --git a/src/term.c b/src/term.c
> index a1e3f6312c6..e762024bdd5 100644
> --- a/src/term.c
> +++ b/src/term.c
> @@ -4686,7 +4686,17 @@ use the Bourne shell command 'TERM=...; export TERM' (C-shell:\n\
> don't think we're losing anything by turning it off. */
> tty->line_ins_del_ok = 0;
>
> - tty->TN_max_colors = 16; /* Must be non-zero for tty-display-color-p. */
> + /* Support full range of colors in new windows terminal app */
> + #if defined(USE_W32CONVTCOLOR_16)
> + tty->TN_max_colors = 16;
> + #elif defined(USE_W32CONVTCOLOR_256)
> + tty->TN_max_colors = 256;
> + #elif defined(USE_W32CONVTCOLOR_24BIT)
> + tty->TN_max_colors = 16777216;
> + #else
> + /* Must be non-zero for tty-display-color-p. */
> + tty->TN_max_colors = 16;
> + #endif
I think it's sub-optimal to have this conditioned on the build-time
conditions. I think users should be able to change the number of
supported colors when starting Emacs (using the --color command-line
option) and/or from within a running Emacs session by changing the
value of a variable. Could you please rework the patch to allow that?
> +#if defined(USE_W32CONVTCOLOR_16) || defined(USE_W32CONVTCOLOR_256) || defined(USE_W32CONVTCOLOR_24BIT)
> + if (!WriteConsole (cur_screen, conversion_buffer,
> + coding->produced, &r, NULL))
> + {
> + printf ("Failed writing console characters: %lu\n",
> + GetLastError ());
> + fflush (stdout);
> + }
We cannot use such printf's in a production session. I guess this is
debugging code that should be removed?
> +#if defined(USE_W32CONVTCOLOR_16) || defined(USE_W32CONVTCOLOR_256) || defined(USE_W32CONVTCOLOR_24BIT)
> + DWORD mode;
> + GetConsoleMode (cur_screen, &mode);
> + mode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
> + SetConsoleMode (cur_screen, mode);
> +#endif
AFAIK, the ENABLE_VIRTUAL_TERMINAL_PROCESSING is not supported on
Windows 9X (which we still strive to support). So either we should
make sure it is ignored on those old systems, or we should disable
these enhanced color modes on those systems.
> + snprintf(p, 10, "[%lum", fg + 30);
Our style is to leave 1 space between the function's name and the
following opening parenthesis.
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + if (fg >= 8 && fg < 16)
> + {
> + char p[10];
> + snprintf(p, 10, "[%lum", fg - 8 + 90);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + if (fg >= 16 && fg < 256)
> + {
> + char p[20];
> + snprintf(p, 20, "[38;5;%lum", fg);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + if (bg >= 0 && bg < 8)
> + {
> + char p[10];
> + snprintf(p, 10, "[%lum", bg + 40);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + if (bg >= 8 && bg < 16)
> + {
> + char p[10];
> + snprintf(p, 10, "[%lum", bg - 8 + 100);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + if (bg>= 16 && bg < 256)
> + {
> + char p[20];
> + snprintf(p, 20, "[48;5;%lum", bg);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> + }
> + }
> + else if (tty->TN_max_colors == 16777216)
> +
> +
> + {
> + char p[30];
> + snprintf(p,30, "[38;2;%lu;%lu;%lum", fg/65536, (fg/256)&255, fg&255);
> + WriteConsole (cur_screen, p, strlen(p), &r, NULL);
> +
> + char q[30];
> + snprintf(q, 30, "[48;2;%lu;%lu;%lum", bg/65536, (bg/256)&255, bg&255);
> + WriteConsole (cur_screen, q, strlen(q), &r, NULL);
> + }
> + }
Can we make this code more compact, like by using fewer snprintf calls
(since they are all so much alike), and just a single WriteConsole
call for each of foreground and background colors?
Finally, these changes will need a NEWS entry and suitable
additions/changes to the user manual.
Thanks again for working on this.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.