GNU bug report logs - #79298
patch: full color in windows terminal

Previous Next

Full log

View this message in rfc822 format

From: Ewan <ewan <at> etown.dev>
To: Eli Zaretskii <eliz <at> gnu.org>, "79298 <at> debbugs.gnu.org" <79298 <at> debbugs.gnu.org>
Subject: bug#79298: patch: full color in windows terminal
Date: Mon, 25 Aug 2025 06:09:37 +0000

> merge 79298 79297

> Such a large contribution will need you to sign a
> copyright-assignment agreement with the FSF.


I assumed as much, and have sent off my details in the form request to assign <at> gnu.org.
If it's easier for you to just send me the form, that works too!

> > --- 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)])],
> > ...
> 
> Why do we need this configure-time switch? why not compile this
> feature into Emacs unconditionally?


This work was inherited from someone else, and I had wondered the same...
I had some worries about supporting Windows Console Host (still 16-color),
but on further review, they've implemented a mapping from 24bit RGB.
Happy to remove the compiler flags and default to 24bit!

> 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?


Will do (just need to figure out where the --color arg gets processed).

> > +#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?


I suspect so, and the existing ones got duplicated in the changes I inherited.
I'll remove them in the revised patch.

> > +#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).


Will it be sufficient to disable the enhanced color modes on start up,
but still allow W9 users to activate them (at their own peril)?

> > + 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);
> > + }
> > + ...
> 
> 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?


More compact, definitely, and we should actually be able to get away with:
- one WriteConsole call for turning the face on
- one WriteConsole call for writing the text
- one WriteConsole call for turning the face off

> Finally, these changes will need a NEWS entry and suitable
> additions/changes to the user manual.


I'll add a NEWS entry.
I'm not sure what exactly editing the manual entails, but happy to do so.


I'll make these changes in the next few days and send off a new patch.

Best,

- Ewan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.