Re: [PATCH v7] Add an option with a color type.

[Eli Zaretskii <eliz@gnu.org>]

> From: Andrei Pikas <gdb@mail.api.win>
> Cc: gdb-patches@sourceware.org,
> 	Andrei Pikas <gdb@mail.api.win>
> Date: Sat, 14 Sep 2024 22:04:52 +0300
> 
> Colors can be specified as "none" for terminal's default color, as a name of
> one of the eight standard colors of ISO/IEC 6429 "black", "red", "green", etc.,
> as an RGB hexadecimal tripplet #RRGGBB for 24-bit TrueColor, or as an
> integer from 0 to 255.  Integers 0 to 7 are the synonyms for the standard
> colors.  Integers 8-15 are used for the so-called bright colors from the
> aixterm extended 16-color palette.  Integers 16-255 are the indexes into xterm
> extended 256-color palette (usually 6x6x6 cube plus gray ramp).  In
> general, 256-color palette is terminal dependent and sometimes can be
> changed with OSC 4 sequences, e.g. "\033]4;1;rgb:00/FF/00\033\\".
> 
> It is the responsibility of the user to verify that the terminal supports
> the specified colors.
> 
> PATCH v5 changes: documentation fixed.
> PATCH v6 changes: documentation fixed.
> PATCH v7 changes: rebase onto master and fixes after review.
> ---
>  gdb/Makefile.in                           |   2 +
>  gdb/NEWS                                  |  29 ++
>  gdb/cli/cli-cmds.c                        |   7 +
>  gdb/cli/cli-decode.c                      | 174 +++++++++
>  gdb/cli/cli-decode.h                      |  21 +
>  gdb/cli/cli-option.c                      |  44 +++
>  gdb/cli/cli-option.h                      |  21 +
>  gdb/cli/cli-setshow.c                     |  21 +
>  gdb/cli/cli-style.c                       |  49 +--
>  gdb/cli/cli-style.h                       |   4 +-
>  gdb/command.h                             |  26 +-
>  gdb/doc/gdb.texinfo                       |  38 +-
>  gdb/doc/guile.texi                        | 104 +++++
>  gdb/doc/python.texi                       |  97 +++++
>  gdb/guile/guile-internal.h                |   9 +
>  gdb/guile/guile.c                         |   1 +
>  gdb/guile/scm-color.c                     | 443 ++++++++++++++++++++++
>  gdb/guile/scm-param.c                     |  33 +-
>  gdb/python/py-color.c                     | 347 +++++++++++++++++
>  gdb/python/py-color.h                     |  35 ++
>  gdb/python/py-param.c                     |  38 +-
>  gdb/python/python.c                       |   7 +
>  gdb/testsuite/gdb.base/style.exp          | 197 ++++++++++
>  gdb/testsuite/gdb.guile/scm-color.exp     | 110 ++++++
>  gdb/testsuite/gdb.guile/scm-parameter.exp |  47 +++
>  gdb/testsuite/gdb.python/py-color.exp     | 100 +++++
>  gdb/testsuite/gdb.python/py-parameter.exp |  53 +++
>  gdb/testsuite/lib/gdb-utils.exp           |  22 +-
>  gdb/testsuite/lib/gdb.exp                 |   3 +-
>  gdb/top.c                                 |  14 +
>  gdb/ui-style.c                            | 331 ++++++++++++----
>  gdb/ui-style.h                            | 142 ++++++-
>  gdb/unittests/style-selftests.c           |   6 +-
>  33 files changed, 2414 insertions(+), 161 deletions(-)
>  create mode 100644 gdb/guile/scm-color.c
>  create mode 100644 gdb/python/py-color.c
>  create mode 100644 gdb/python/py-color.h
>  create mode 100644 gdb/testsuite/gdb.guile/scm-color.exp
>  create mode 100644 gdb/testsuite/gdb.python/py-color.exp

Thanks.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -37,11 +37,40 @@
>    history has been reached.  It also specifies that the forward execution can
>    continue, and the recording will also continue.
>  
> +* "set style" commands now supports numeric format for basic colors
> +  from 0 to 255 and #RRGGBB format for TrueColor.
> +
> +* New built-in convenience variable $_colorsupport provides comma-separated
> +  list of color space names supported by terminal.  It is handy for
> +  conditionally using styling colors based on terminal features.  For example:
> +
> +  (gdb) if $_regex ($_colorsupport, ".*(^|,)rgb_24bit($|,).*")
> +   >set style filename background #FACADE
> +   >else
> +   >if $_regex ($_colorsupport, ".*(^|,)xterm_256color($|,).*")
> +    >set style filename background 224
> +    >else
> +    >set style filename background red
> +    >end
> +   >end
> +

I'd prefer to have a plain-English explanation of "color space" here,
instead of trying to explain it via script which uses the variable.

> --- a/gdb/cli/cli-style.c
> +++ b/gdb/cli/cli-style.c
> @@ -42,20 +42,6 @@ bool source_styling = true;
>  
>  bool disassembler_styling = true;
>  
> -/* Name of colors; must correspond to ui_file_style::basic_color.  */
> -static const char * const cli_colors[] = {
> -  "none",
> -  "black",
> -  "red",
> -  "green",
> -  "yellow",
> -  "blue",
> -  "magenta",
> -  "cyan",
> -  "white",
> -  nullptr
> -};

I don't understand the effect of this and related changes, and the log
message says nothing about this.  Could you please explain why you
remove these names and their uses, and what will be used in their
stead?  And why?

Regardless, were these changes tested in the MinGW port of GDB?  It
emulates Posix terminal handling of colors via SGR escape sequences,
and I wonder whether these changes might somehow break styling support
in the MinGW port.

> +@item $_colorsupport
> +@vindex $_colorsupport@r{, convenience variable}
> +Comma-separated list of color space names supported by terminal.  Names could
> +be any of @samp{monochrome}, @samp{ansi_8color}, @samp{aixterm_16color},
> +@samp{xterm_256color}, @samp{rgb_24bit}.  E.g., for plain linux terminal the
> +value could be @samp{monochrome,ansi_8color} and for terminal with truecolor
> +support it could be
> +@samp{monochrome,ansi_8color,aixterm_16color,xterm_256color,rgb_24bit}.

What does this return for the MS-Windows terminal? aixterm_16color?
IOW, will any 16-color terminal return aixterm_16color?  I think this
should be documented, and perhaps we should remove the "aix" part from
the name (since it is not necessarily specific to AIX).

Also, please add a "@cindex color space" entry before the above text,
as I believe this is where we describe this term.  For the same
reason, I think the first use of "color spaces" should have the @dfn
markup.

> -Set the background to @var{color}.  Valid colors are @samp{none}
> -(meaning the terminal's default color), @samp{black}, @samp{red},
> -@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan},
> -and@samp{white}.
> +Set the background to @var{color}.  @var{color} can be @samp{none}
> +(meaning the terminal's default color), a name of one of the eight standard
> +colors of ISO/IEC 6429, index from 0 to 255 into terminal's color
> +palette or a hexadecimal RGB triplet in @samp{#RRGGBB} format for
> +24-bit TrueColor.

Should the RGB specs always use 2 hex characters per component,
or can one use RRRGGGBBB etc. as well?  This should be documented.