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

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrei" == Andrei Pikas <gdb@mail.api.win> writes:

Andrei> Colors can be specified as "none" for terminal's default color, as a name of
Andrei> one of the eight standard colors of ISO/IEC 6429 "black", "red", "green", etc.,
Andrei> as an RGB hexadecimal tripplet #RRGGBB for 24-bit TrueColor, or as an
Andrei> integer from 0 to 255.  Integers 0 to 7 are the synonyms for the standard
Andrei> colors.  Integers 8-15 are used for the so-called bright colors from the
Andrei> aixterm extended 16-color palette.  Integers 16-255 are the indexes into xterm
Andrei> extended 256-color palette (usually 6x6x6 cube plus gray ramp).  In
Andrei> general, 256-color palette is terminal dependent and sometimes can be
Andrei> changed with OSC 4 sequences, e.g. "\033]4;1;rgb:00/FF/00\033\\".

Thank you for the patch.  I'm sorry about the long delays on this; gdb
has an ongoing review bandwidth problem.  Anyway, I like this a lot and
I would like to see it get in.

I have a fair number of notes here but nothing really major.

Andrei> +    case var_color:
Andrei> +      {
Andrei> +	std::string s = var.get<ui_file_style::color> ().to_string ();
Andrei> +	return value_cstring (s.c_str (), s.size (),
Andrei> +			      builtin_type (gdbarch)->builtin_char);

Other code in this function uses:

	return current_language->value_string (gdbarch, value, len);

Andrei> +  if (*text == '\0')
Andrei> +    {
Andrei> +      /* Convenience to let the user know what the option
Andrei> +	 can accept.  Note there's no common prefix between
Andrei> +	 the strings on purpose, so that complete_on_enum doesn't do
Andrei> +	 a partial match.  */
Andrei> +      tracker.add_completion (make_unique_xstrdup ("NUMBER"));
Andrei> +      tracker.add_completion (make_unique_xstrdup ("#RRGGBB"));
Andrei> +    }

This is a clever idea and I somewhat wish readline had a more robust
completion API to show this kind of info "inline".

Andrei> +ui_file_style::color
Andrei> +parse_cli_var_color (const char **args)
Andrei> +{
Andrei> +  /* Do a "set" command.  ARG is nullptr if no argument, or the
Andrei> +     text of the argument.  */
Andrei> +
Andrei> +  if (args == nullptr || *args == nullptr || **args == '\0')
Andrei> +    {
Andrei> +      std::string msg;
Andrei> +
Andrei> +      for (size_t i = 0; ui_file_style::basic_color_enums[i]; ++i)
Andrei> +	{
Andrei> +	  msg.append ("\"");
Andrei> +	  msg.append (ui_file_style::basic_color_enums[i]);
Andrei> +	  msg.append ("\", ");

I think this ends up appending an extra comma.  Perhaps the
comma-addition should be done at the star of the loop, if "i > 0".

Andrei> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
Andrei> index 05539285c80..3a165ef6d2c 100644
Andrei> --- a/gdb/cli/cli-option.c
Andrei> +++ b/gdb/cli/cli-option.c
Andrei> @@ -45,6 +45,9 @@ union option_value
 
Andrei>    /* For var_string options.  This is malloc-allocated.  */
Andrei>    std::string *string;
Andrei> +
Andrei> +  /* For var_color options.  */
Andrei> +  ui_file_style::color color = ui_file_style::NONE;

I didn't even know inline initializers were possible in a union.
Anyway I think it's probably better to leave this off.

Andrei> +@node Colors In Python
Andrei> +@subsection Python representation of colors

This new node doesn't seem to be referenced from a menu anywhere.  I
think it has to be added to the appropriate spot in the menu in the
"Python API" node.

Andrei> diff --git a/gdb/python/py-color.c b/gdb/python/py-color.c
Andrei> new file mode 100644
Andrei> index 00000000000..1d72cc3e7d1
Andrei> --- /dev/null
Andrei> +++ b/gdb/python/py-color.c
Andrei> @@ -0,0 +1,347 @@
Andrei> +/* Python interface to ui_file_style::color objects.
Andrei> +
Andrei> +   Copyright (C) 2008-2022 Free Software Foundation, Inc.

I suspect this should just be 2024.

Andrei> +/* See py-color.h.  */
Andrei> +bool
Andrei> +gdbpy_is_color (PyObject *obj)
Andrei> +{
Andrei> +  return PyObject_IsInstance (obj, (PyObject *)&colorpy_object_type);

gdb style is a space after the closing paren of a cast.

Andrei> +static PyObject *
Andrei> +colorpy_escape_sequence (PyObject *self, PyObject *is_fg_obj)
Andrei> +{
...
Andrei> +  bool is_fg = PyObject_IsTrue (is_fg_obj);

In theory, this can fail, returning -1.

In practice, the code already calls PyBool_Check, so I think just a
direct comparison against Py_True would be fine.

Andrei> +static int
Andrei> +colorpy_init (PyObject *self, PyObject *args, PyObject *kwds)
...
Andrei> +      if (colorspace_obj)
Andrei> +	{
Andrei> +	  if (PyLong_Check (colorspace_obj))
Andrei> +	    {
Andrei> +	      long colorspace_id = -1;
Andrei> +	      if (! gdb_py_int_as_long (colorspace_obj, &colorspace_id))
Andrei> +		return -1;
Andrei> +	      if (colorspace_id < INT_MIN || colorspace_id > INT_MAX)
Andrei> +		error (_("colorspace %ld is out of range."), colorspace_id);

It seems like the range check here should use the colorspace enum values.

Andrei> +	  Py_ssize_t tuple_size = PyTuple_Size (value_obj);
Andrei> +	  if (tuple_size != 3)
Andrei> +	    error (_("Tuple value with RGB must be of size 3."));

If PyTuple_Size returns -1, this should early-return.

Andrei> +	  if (!str)
Andrei> +	    return -1;

gdb uses comparisons like 'str == nullptr' here.

Andrei> +	  if (colorspace_obj != nullptr
Andrei> +	      && colorspace != obj->color.colorspace ())
Andrei> +	    error (_("colorspace doesn't match to value."));

I would s/to/the in this error message.

Andrei> +  catch (const gdb_exception &except)
Andrei> +    {
Andrei> +      gdbpy_convert_exception (except);
Andrei> +      return -1;
Andrei> +    }

A newish change landed and now this is written

    return gdbpy_handle_exception (-1, except);

Andrei> +/* Deallocate function for a gdb.Color.  */
Andrei> +
Andrei> +static void
Andrei> +colorpy_dealloc (PyObject *)
Andrei> +{
Andrei> +}

I wonder if this is necessary.
If so maybe a comment explaining why would be good.

Andrei> +/* Initialize the 'color' module.  */
Andrei> +static int
Andrei> +gdbpy_initialize_color (void)
Andrei> +{
Andrei> +  colorpy_object_type.tp_new = PyType_GenericNew;
Andrei> +  if (PyType_Ready (&colorpy_object_type) < 0)
Andrei> +    return -1;

A recent change means that this should now call gdbpy_type_ready
instead.  This also removes the need to call gdb_pymodule_addobject, so
this call can be lowered to the end of this function.

Andrei> diff --git a/gdb/python/py-color.h b/gdb/python/py-color.h
...
Andrei> +/* Extracts value from OBJ object of gdb.Color type.  */
Andrei> +extern const ui_file_style::color & gdbpy_get_color (PyObject *obj);

No space after the "&".

Andrei> -  memset (&obj->value, 0, sizeof (obj->value));
Andrei> +  obj->value = {}; /* zeros initialization */

Was this needed?

Andrei> diff --git a/gdb/testsuite/gdb.guile/scm-color.exp b/gdb/testsuite/gdb.guile/scm-color.exp
Andrei> new file mode 100644
Andrei> index 00000000000..e5773650f2f
Andrei> --- /dev/null
Andrei> +++ b/gdb/testsuite/gdb.guile/scm-color.exp
Andrei> @@ -0,0 +1,110 @@
Andrei> +# Copyright (C) 2010-2022 Free Software Foundation, Inc.

Another date to change.  I'd suggest looking at all of them.

Andrei> diff --git a/gdb/testsuite/gdb.python/py-parameter.exp b/gdb/testsuite/gdb.python/py-parameter.exp
Andrei> index de524f49ad6..6e6a6007198 100644
Andrei> --- a/gdb/testsuite/gdb.python/py-parameter.exp
Andrei> +++ b/gdb/testsuite/gdb.python/py-parameter.exp
Andrei> @@ -185,6 +185,58 @@ proc_with_prefix test_enum_parameter { } {
Andrei>  	"Undefined item: \"three\".*" "set invalid enum parameter"
Andrei>  }
 
Andrei> +# Test an color parameter.
Andrei> +proc_with_prefix test_color_parameter { } {
Andrei> +    global env
Andrei> +    save_vars { env(TERM) env(COLORTERM) } {

This should probably use with_ansi_styling_terminal to avoid having
NO_COLOR set?

Andrei> +static void
Andrei> +init_colorsupport_var (void)

You should use "()" instead of "(void)".  The latter is a leftover
C-ism.

Andrei> +{
Andrei> +  const std::vector<color_space>& cs = colorsupport ();

Also move the "&" after the space.

Andrei> +/* See ui-style.h.  */
Andrei> +/* Must correspond to ui_file_style::basic_color.  */
Andrei> +const std::vector<const char *> ui_file_style::basic_color_enums = {
Andrei> +  "none",
Andrei> +  "black",
Andrei> +  "red",
Andrei> +  "green",
Andrei> +  "yellow",
Andrei> +  "blue",
Andrei> +  "magenta",
Andrei> +  "cyan",
Andrei> +  "white",
Andrei> +  nullptr
Andrei> +};

I guess the nullptr is needed since although it is a vector now, it's
still passed to some null-expecting API?  Like maybe completion?

I wonder if a vector can be constexpr.

Andrei>  bool
Andrei>  ui_file_style::color::append_ansi (bool is_fg, std::string *str) const
Andrei>  {
...
Andrei> +  else
Andrei> +    return false;

I wonder when this can happen and if it could somehow be made
impossible.

Andrei> +const std::vector<color_space> &
Andrei> +colorsupport ()
Andrei> +{
Andrei> +  static const std::vector<color_space> value = []

I didn't realize you could omit the '()' from a lambda.

Andrei> +    {
Andrei> +      std::vector<color_space> result = {color_space::MONOCHROME};
Andrei> +
Andrei> +      int colors = tgetnum ((char *)"Co");

Is the cast really needed?

Andrei> +      const char *colorterm = getenv ("COLORTERM");
Andrei> +      if (colorterm && (!strcmp (colorterm, "truecolor")

'colorterm != nullptr'

Andrei> +const char *
Andrei> +color_space_name (color_space c)
Andrei> +{
Andrei> +  switch (c)
Andrei> +    {
Andrei> +    case color_space::MONOCHROME: return "monochrome";
Andrei> +    case color_space::ANSI_8COLOR: return "ansi_8color";
Andrei> +    case color_space::AIXTERM_16COLOR: return "aixterm_16color";
Andrei> +    case color_space::XTERM_256COLOR: return "xterm_256color";
Andrei> +    case color_space::RGB_24BIT: return "rgb_24bit";
Andrei> +    }
Andrei> +  error (_("Color space %d has no name."), static_cast<int> (c));

Probably an assert-not-reached is more appropriate here?

Andrei> +}
Andrei> diff --git a/gdb/ui-style.h b/gdb/ui-style.h
Andrei> index 1b7b5fafb9d..a13618dfce5 100644
Andrei> --- a/gdb/ui-style.h
Andrei> +++ b/gdb/ui-style.h
Andrei> @@ -19,6 +19,26 @@
Andrei>  #ifndef UI_STYLE_H
Andrei>  #define UI_STYLE_H
 
Andrei> +/* One of the color spaces that usually supported by terminals.  */
Andrei> +enum class color_space
Andrei> +{
Andrei> +  MONOCHROME, // one default terminal color
Andrei> +  ANSI_8COLOR, // foreground colors \e[30m ... \e[37m,
Andrei> +	       // background colors \e[40m ... \e[47m
Andrei> +  AIXTERM_16COLOR, // foreground colors \e[30m ... \e[37m, \e[90m ... \e[97m
Andrei> +		   // background colors \e[40m ... \e[47m, \e[100m ... \e107m
Andrei> +  XTERM_256COLOR, // foreground colors \e[38;5;0m ... \e[38;5;255m
Andrei> +		  // background colors \e[48;5;0m ... \e[48;5;255m
Andrei> +  RGB_24BIT // foreground colors \e[38;2;0;0;0m ... \e[38;2;255;255;255m
Andrei> +	    // background colors \e[48;2;0;0;0m ... \e[48;2;255;255;255m

gdb doesn't use "//" comments.  Also normally gdb doesn't use trailing
comments on the line; instead I'd put them before each constant they
describe.

Tom