Re: [PATCH v8] 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,  5 Oct 2024 22:27:44 +0300
> 
>  gdb/Makefile.in                           |   2 +
>  gdb/NEWS                                  |  31 ++
>  gdb/cli/cli-cmds.c                        |   6 +
>  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                       |  46 ++-
>  gdb/doc/guile.texi                        | 104 ++++++
>  gdb/doc/python.texi                       |  98 +++++
>  gdb/guile/guile-internal.h                |   9 +
>  gdb/guile/guile.c                         |   1 +
>  gdb/guile/scm-color.c                     | 427 ++++++++++++++++++++++
>  gdb/guile/scm-param.c                     |  33 +-
>  gdb/python/py-color.c                     | 336 +++++++++++++++++
>  gdb/python/py-color.h                     |  35 ++
>  gdb/python/py-param.c                     |  40 +-
>  gdb/python/python.c                       |   7 +
>  gdb/testsuite/gdb.base/default.exp        |   8 +-
>  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           |  24 +-
>  gdb/testsuite/lib/gdb.exp                 |   3 +-
>  gdb/top.c                                 |  14 +
>  gdb/ui-style.c                            | 353 ++++++++++++++----
>  gdb/ui-style.h                            | 157 +++++++-
>  gdb/unittests/style-selftests.c           |   6 +-
>  34 files changed, 2436 insertions(+), 171 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, some comments for the documentation parts below.

> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -49,11 +49,42 @@
>    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.  Each color space name is one
> +  of monochrome, ansi_8color, aixterm_16color, xterm_256color or rgb_24bit.
> +  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
> +
>  * Python API
>  
>    ** Added gdb.record.clear.  Clears the trace data of the current recording.
>       This forces re-decoding of the trace for successive commands.
>  
> +  ** New class gdb.Color for dealing with colors.
> +
> +  ** New constant gdb.PARAM_COLOR represents color type of a
> +     gdb.Parameter.value.  Parameter's value is gdb.Color instance.
> +
> +* Guile API
> +
> +  ** New type <gdb:color> for dealing with colors.
> +
> +  ** New constant PARAM_COLOR represents color type of a value
> +     of a <gdb:parameter> object.  Parameter's value is <gdb::color> instance.
> +
>  * Debugger Adapter Protocol changes

The NEWS part is okay.

> +@item $_colorsupport
> +@vindex $_colorsupport@r{, convenience variable}
> +@cindex color space
> +Comma-separated list of @dfn{color space} names supported by terminal.  Names

Please always put all the index commands _before_ @item, not after it.
This is so following the index entry in an Info reader places you at
the line corresponding to @item and not on the line after that.

>  @table @code
>  @item set style filename background @var{color}
> -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.
> +
> +Valid color names are @samp{black}, @samp{red}, @samp{green},
> +@samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan}, and
> +@samp{white}.

To make sure the reader understands that these 8 names are what is
mentioned earlier as "the eight standard colors", I suggest to start
this paragraph with "Valid names of the eight standard colors are..."

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

As followup to my previous questions: can these bright colors also be
used on the Windows terminal?

Also, shouldn't we document in the manual the names of those bright
colors?

> 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\\". A hexadecimal 24-bit TrueColor is specified in
                             ^^ ^^
What is that double backslash there? are those two literal
backslashes?  Also, please leave two spaces between sentences.

> +@item PARAM_COLOR
> +The value is either a string or an unsigned integer.  Integer from 0 to 255
> +means index into terminal's color palette.  String can be a hex RGB triplet in
> +@samp{#RRGGBB} format or one of the following color names:
> +@samp{none} (meaning the terminal's default color), @samp{black}, @samp{red},
> +@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan},
> +or @samp{white}.
>  @end vtable

This text repeats what you say below in "Colors in Guile".  It is
better to replace it with a cross-reference to that mode, saying here
just that the value can be either an integer or a string.

Btw, since this is describing a programmatic interface, should we also
say what happens if the value is an integer or a string that the
terminal does not support?

> +@deffn {Scheme Procedure} make-color @w{@r{[}value} @
> +    @w{@r{[}#:color-space color-space@r{]}@r{]}}
> +
> +@var{value} is an integer index of a color in palette, tuple with color
> +components or a string.                                ^^^^^^^^^^^^^^^^
   ^^^^^^^^^^
This is the first and the only time you mention tuple of components in
the Guile section.  If tuples are indeed allowed, they should be
mentioned in the other places and also documented in more detail.

> +format or one of the following color names:
> +@samp{none} (meaning the terminal's default color), @samp{black}, @samp{red},
> +@samp{green}, @samp{yellow}, @samp{blue}, @samp{magenta}, @samp{cyan},
> +or @samp{white}.
> +
> +@var{color-space} should be one of the @samp{COLORSPACE_} constants.  This
> +argument tells @value{GDBN} which color space @var{value} belongs.

COLOR-SPACE is an optional argument, right?  If so, this should
document what happens if it is omitted.

More generally, I'm a bit confused by the notion of "palette" or
"color space" (which are the same, right?).  If I have a color whose
index is N < 8, doesn't that refer to the same color regardless of the
palette?  And if I specify a color with #RRGGBB whose value exactly
corresponds to one of the 8 standard colors, don't I get the same
color regardless of the palette?  If so, it sounds like the palette
only makes a difference when the color is specified by a value (index
or RGB string) that does NOT belong to the palette, right?  And if so,
why do we at all need this notion of the palette in GDB?

> +@deffn {Scheme Procedure} color-none? color
> +Return @code{#t} if @var{color} is terminal's default.

Is it a good idea to use 'color-none?' as the name? why not
color-default?  Also, AFAIU a terminal has two default colors: one for
the foreground, the other for the background.  How does this procedure
know which of these two is the caller asking about?

> +@deffn {Scheme Procedure} color-indexed? color
> +Return @code{#t} if @var{color} is indexed, i.e. belongs to some palette.

Please use "i.e.@:" or "i.e.,".  Otherwise, TeX could decide that the
period ends the sentence.

> +Otherwise return @code{#f}.
> +@end deffn
> +
> +@deffn {Scheme Procedure} color-direct? color
> +Return @code{#t} if @var{color} is direct in the sense of ISO/IEC 8613-6.
> +Otherwise return @code{#f}.
> +@end deffn

I'm a bit confused about these predicates: don't at least some,
perhaps even all, of the colors satisfy both indexed? and direct?
predicates?  If so, when will these return #f?  And why have these
predicates in the first place? when will they be useful?

> +@deffn {Scheme Procedure} color-string color
> +Return the textual representation of a @code{<gdb:color>} object.
> +@end deffn

This should probably explain what is the returned string.  I'm
guessing it's the name for the 8 standard colors (or maybe for the 16
standard colors of a 16-color terminal), and #RRGGBB for the rest, but
the manual should tell that explicitly.

> +@deffn {Scheme Procedure} color-index color
> +Return index of the color of a @code{<gdb:color>} object in a palette.
> +@end deffn

What does this return for TrueColor colors?

> +@deffn {Scheme Procedure} color-components color
> +Return components of the direct @code{<gdb:color>} object.
> +@end deffn

Please document the form of the return value, i.e. how the components
are returned.

> +@deffn {Scheme Procedure} color-escape-sequence color is_foreground
> +Return string to change terminal's color to this.
          ^^^^^^
"the string"

> +When color is initialized, its color space must be specified.

What do you mean by "color is initialized"?  This is the first time a
color's initialization is mentioned, and it must be explained.

> +@vtable @code
> +@item COLORSPACE_MONOCHROME
> +Palette with only terminal's default color.

Two colors, right?  One for foreground, the other for background.  Or
does this refer only to the foreground color?

> +@item COLORSPACE_AIXTERM_16COLOR
> +Palette with 16 colors.  First eight are standard colors of ISO/IEC 6429
> +"black", "red", "green", etc.  Next eight are their bright version.
                                                              ^^^^^^^
"versions".

> +@defvar Color.is_none
> +This atribute is boolean.  If its value is @code{True} then color is terminal's
> +default.
> +@end defvar
> +
> +@defvar Color.is_indexed
> +This atribute is boolean.  If its value is @code{True} then color is indexed,
> +i.e. belongs to some palette.
> +@end defvar
> +
> +@defvar Color.is_direct
> +This atribute is boolean.  If its value is @code{True} then this object
> +describes direct colour in the sense of ISO/IEC 8613-6.
> +@end defvar

Same questions/comments here as for the equivalent Guile APIs.

> +
> +@defvar Color.index
> +This attribute exist if @code{is_indexed} is @code{True}.  Its integer value is
> +index of a color in a palette.
> +@end defvar
> +
> +@defvar Color.components
> +This attribute exist if @code{is_direct} is @code{True}.  Its value is tuple
> +with integer components of a color.
> +@end defvar

The corresponding Guile APIs didn't include the conditions under which
the values are significant.  Why not?

> +When color is initialized, its color space must be specified.  The

Does "initialized" refer to Color.__init__?  If so, I think this
should be said explicitly, perhaps where that function is described.

Finally, I see from the code that we sometimes approximate the
requested color (which is reasonable), but I see nothing about that in
the documentation.  I think we should say something about that in the
manual.