[RFC,3/8] Add output styles to gdb

  This adds some output styling to the CLI.

A style is currently a foreground color, a background color, and an
intensity (dim or bold).  (This list could be expanded depending on
terminal capabilities.)

A style can be applied while printing.  This patch changes cli-out.c
to apply styles just to certain fields, recognized by name.  In
particular, function names and file names are stylized.

This seemed like a reasonable approach, because the names are fixed
due to their use in MI.  That is, the CLI is just as exposed to future
changes as MI is.

Users can control the style via a number of new set/show commands.  In
the interest of not typing many nearly-identical documentation
strings, I automated this.  On the down side, this is not very
i18n-friendly.

I've chose some default colors to use.  I think it would be good to
enable this by default, so that when users start the new gdb, they
will see the new feature.

Stylizing is done if TERM is set and is not "dumb".  This could be
improved when the TUI is available by using the curses has_colors
call.  That is, the lowest layer could call this without committing to
using curses everywhere; see my other patch for TUI colorizing.

I considered adding a new "set_style" method to ui_file.  However,
because the implementation had to interact with the pager code, I
didn't take this approach.  But, one idea might be to put the isatty
check there and then have it defer to the lower layers.
---
 gdb/Makefile.in     |   2 +
 gdb/cli-out.c       |  19 ++++-
 gdb/cli/cli-style.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/cli/cli-style.h |  67 +++++++++++++++++
 gdb/ui-file.h       |  42 +++++++++++
 gdb/utils.c         |  69 +++++++++++++++++-
 gdb/utils.h         |   4 +
 7 files changed, 408 insertions(+), 2 deletions(-)
 create mode 100644 gdb/cli/cli-style.c
 create mode 100644 gdb/cli/cli-style.h
  

On 2018-09-06 5:12 p.m., Tom Tromey wrote:
> This adds some output styling to the CLI.
> 
> A style is currently a foreground color, a background color, and an
> intensity (dim or bold).  (This list could be expanded depending on
> terminal capabilities.)
> 
> A style can be applied while printing.  This patch changes cli-out.c
> to apply styles just to certain fields, recognized by name.  In
> particular, function names and file names are stylized.
> 
> This seemed like a reasonable approach, because the names are fixed
> due to their use in MI.  That is, the CLI is just as exposed to future
> changes as MI is.
> 
> Users can control the style via a number of new set/show commands.  In
> the interest of not typing many nearly-identical documentation
> strings, I automated this.  On the down side, this is not very
> i18n-friendly.
> 
> I've chose some default colors to use.  I think it would be good to
> enable this by default, so that when users start the new gdb, they
> will see the new feature.
> 
> Stylizing is done if TERM is set and is not "dumb".  This could be
> improved when the TUI is available by using the curses has_colors
> call.  That is, the lowest layer could call this without committing to
> using curses everywhere; see my other patch for TUI colorizing.
> 
> I considered adding a new "set_style" method to ui_file.  However,
> because the implementation had to interact with the pager code, I
> didn't take this approach.  But, one idea might be to put the isatty
> check there and then have it defer to the lower layers.

Hi Tom,

Overall this looks great.  I'm not too worried about internationalization.
I think in this case for example:

  _("The \"%s\" display intensity is: %s\n"), name, value

If "name" corresponds to a field name or sub-command name (like "filename"),
it's probably better to leave it in english.  If it refers to the concept
of a filename, then we would want to translate it.

In the latter case, I guess we could do it in two steps, and also pass the value
through gettext:

  std::string tmpl = string_printf ("The %s display intensity is: %%s\n", name);
  printf (_(tmpl.c_str ()), _(value));

So tmpl would contain "The filename display intensity is: %s\n", which could be
looked up by gettext to something that has the proper translation for "filename".
Then, the color name would be translated too.

Or maybe I don't understand how gettext works.


I'm just not sure about choosing styles using the field name.  For a filename, you could
have a bunch of different field names, file filename, original_filename, absolute_filename,
etc.  So it can quickly become a bit crowded here.

Could we pass an additional enum parameter to do_field_string to indicate the type of
element this field represents?  If this parameter has a default value of "NOTHING",
then we can add then incrementally.  Or it can even be an hybrid approach, where we
try to match field names, which works 95% of the time, and then have this enum parameter
to override the auto-detection if needed.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Could we pass an additional enum parameter to do_field_string to
Simon> indicate the type of element this field represents?  If this
Simon> parameter has a default value of "NOTHING", then we can add then
Simon> incrementally.

Yes, either this or, as you mentioned in another note, passing the style
directly, could be done.  I suppose I tend to lean more toward the
semantic approach than the style approach, because maybe having some
kind of "type" attached to a field could be useful in other situations.

Be sure to read this as well:

https://sourceware.org/ml/gdb-patches/2018-10/msg00107.html

It presents another alternative -- one I rejected but maybe it can be
rehabilitated.

These questions about the overall approach and the API are the main
things to resolve up front, since they have the highest cost to change.

Tom
  

On 2018-10-06 3:06 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Could we pass an additional enum parameter to do_field_string to
> Simon> indicate the type of element this field represents?  If this
> Simon> parameter has a default value of "NOTHING", then we can add then
> Simon> incrementally.
> 
> Yes, either this or, as you mentioned in another note, passing the style
> directly, could be done.  I suppose I tend to lean more toward the
> semantic approach than the style approach, because maybe having some
> kind of "type" attached to a field could be useful in other situations.
> 
> Be sure to read this as well:
> 
> https://sourceware.org/ml/gdb-patches/2018-10/msg00107.html
> 
> It presents another alternative -- one I rejected but maybe it can be
> rehabilitated.

I guess you refer to Pedro's suggestion of having custom format strings.  I am not
against that, I am just afraid that because it's a lot more efforts than something
like you did, it would postpone the results indefinitely (unless somebody is super
motivated to do it right now).

Also, I think these custom format strings (LLVM uses its own style too, as mentioned)
need to bring significantly more value (in terms of code simplicity) than the other
alternatives, because they are harder to learn, modify and debug than simple function
calls.
> These questions about the overall approach and the API are the main
> things to resolve up front, since they have the highest cost to change.

Indeed, but at least it's not external API, so we have the option to change.

Simon
  

>> These questions about the overall approach and the API are the main
>> things to resolve up front, since they have the highest cost to change.

Simon> Indeed, but at least it's not external API, so we have the option to change.

The main thing I am concerned about is that if we pick the approach I've
implemented, then changing our minds to a format-based approach will be
harder.  I think that's the case because the user-facing "set"s would be
hard to migrate to a format-based approach.

And, FWIW, I think the format-based approach isn't super hard to
implement.  The main problems there are design problems: whether to
remap the names of tables and fields; how to handle the trouble cases I
found in my initial investigation; and of course whether there are more
difficult cases lurking.

Overall I think I'm in favor of the approach I've posted, but I think
it's a good to examine this idea critically.

Tom
  

On 2018-10-07 20:23, Tom Tromey wrote:
> Simon> Indeed, but at least it's not external API, so we have the
> option to change.
> 
> The main thing I am concerned about is that if we pick the approach 
> I've
> implemented, then changing our minds to a format-based approach will be
> harder.  I think that's the case because the user-facing "set"s would 
> be
> hard to migrate to a format-based approach.
> 
> And, FWIW, I think the format-based approach isn't super hard to
> implement.  The main problems there are design problems: whether to
> remap the names of tables and fields; how to handle the trouble cases I
> found in my initial investigation; and of course whether there are more
> difficult cases lurking.

Ok, so I'd like to understand better the idea of the format-based 
approach, maybe I don't quite get what you mean.  It would be easier to 
comment on something concrete (even without code, just a relatively 
well-defined description of the format).

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Ok, so I'd like to understand better the idea of the format-based
Simon> approach, maybe I don't quite get what you mean.  It would be easier
Simon> to comment on something concrete (even without code, just a relatively
Simon> well-defined description of the format).

The minimum, I think, is to have something that encompasses the
formatting, but not the titling, emitted by table headers.  So instead
of:

  uiout->table_header (10, ui_left, "refcount", "Refcount");

the example I gave said

  "{refcount<10}"

where the idea is that "refcount" is the field name, "<" says to
left-justify, and "10" is the field size.  I pictured using ">" for
right justification, and something like "|" for center and ":" or "="
for "none".

Anything outside of {} would be plain text that is just emitted
directly.

Exactly how to spell the formatting can be debated, like should there
be a lead-in character ("set extended-prompt" uses backslash, C uses %).

So, something like maintenance_info_bfds currently reads:

  ui_out_emit_table table_emitter (uiout, 3, -1, "bfds");
  uiout->table_header (10, ui_left, "refcount", "Refcount");
  uiout->table_header (18, ui_left, "addr", "Address");
  uiout->table_header (40, ui_left, "filename", "Filename");

... but in the format approach the first 2 arguments of each call could
be removed.

Then when printing the body:

  ui_out_emit_tuple tuple_emitter (uiout, NULL);
  uiout->field_int ("refcount", gdata->refc);
  uiout->field_string ("addr", host_address_to_string (abfd));
  uiout->field_string ("filename", bfd_get_filename (abfd));
  uiout->text ("\n");

Here the ->text call could be removed and the text put into the format
string.


I didn't do an exhaustive survey of lists or tuples in the way that I
looked at tables.  There are probably difficult cases lurking in there,
just as there were for tables.  And, the difficult table cases remain
unsolved -- I haven't really given them much thought.

Maybe another tricky case is handling indentation of elided stack
frames.


Something I like about the approach I implemented is that it's
relatively easy to explain and use.

The upside of the foramtting approach is that it gives more power to
users.  For example maybe there are things that are currently hidden
behind mi-like-p that could be exposed; or at least users could arrange
to drop fields they are not interested in.

Tom
  

On 2018-10-07 22:49, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Ok, so I'd like to understand better the idea of the 
> format-based
> Simon> approach, maybe I don't quite get what you mean.  It would be 
> easier
> Simon> to comment on something concrete (even without code, just a 
> relatively
> Simon> well-defined description of the format).
> 
> The minimum, I think, is to have something that encompasses the
> formatting, but not the titling, emitted by table headers.  So instead
> of:
> 
>   uiout->table_header (10, ui_left, "refcount", "Refcount");
> 
> the example I gave said
> 
>   "{refcount<10}"
> 
> where the idea is that "refcount" is the field name, "<" says to
> left-justify, and "10" is the field size.  I pictured using ">" for
> right justification, and something like "|" for center and ":" or "="
> for "none".
> 
> Anything outside of {} would be plain text that is just emitted
> directly.

Do we pass in the values as with printf?  Something like

   ui_out->format ("{refcount<10}{addr<18}{filename<40}\n", refcount, 
addr, filename);

How does the underlying implementation of format knows that the refcount 
variable is an int, and that addr and filename are char*?  We go by 
field name too?  Or should the format specifier contain the data type?

> Exactly how to spell the formatting can be debated, like should there
> be a lead-in character ("set extended-prompt" uses backslash, C uses 
> %).
> 
> So, something like maintenance_info_bfds currently reads:
> 
>   ui_out_emit_table table_emitter (uiout, 3, -1, "bfds");
>   uiout->table_header (10, ui_left, "refcount", "Refcount");
>   uiout->table_header (18, ui_left, "addr", "Address");
>   uiout->table_header (40, ui_left, "filename", "Filename");
> 
> ... but in the format approach the first 2 arguments of each call could
> be removed.
> 
> Then when printing the body:
> 
>   ui_out_emit_tuple tuple_emitter (uiout, NULL);
>   uiout->field_int ("refcount", gdata->refc);
>   uiout->field_string ("addr", host_address_to_string (abfd));
>   uiout->field_string ("filename", bfd_get_filename (abfd));
>   uiout->text ("\n");
> 
> Here the ->text call could be removed and the text put into the format
> string.
> 
> 
> I didn't do an exhaustive survey of lists or tuples in the way that I
> looked at tables.  There are probably difficult cases lurking in there,
> just as there were for tables.  And, the difficult table cases remain
> unsolved -- I haven't really given them much thought.
> 
> Maybe another tricky case is handling indentation of elided stack
> frames.
> 
> 
> Something I like about the approach I implemented is that it's
> relatively easy to explain and use.
> 
> The upside of the foramtting approach is that it gives more power to
> users.  For example maybe there are things that are currently hidden
> behind mi-like-p that could be exposed; or at least users could arrange
> to drop fields they are not interested in.

I tend to think that introducing a format-based system like this is 
relatively orthogonal to the coloring/styling issue.  I see it as 
syntactic sugar over the field_* methods.  Something would parse that 
format string and do the appropriate field_* calls for you under the 
hood.  If we color based on field names, as the current patch does, then 
it wouldn't make any difference.  If we provide a way for the caller to 
override the element type (on which the color/style choice is based), 
then we need to consider that in the format string syntax.  If field 
"foo" is a filename, we can have something like

     "{foo<40:filename}"

So my opinion is that introducing something like this isn't in the way 
of introducing coloring with the current system.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> "{refcount<10}"

Simon> Do we pass in the values as with printf?  Something like
Simon> ui_out-> format ("{refcount<10}{addr<18}{filename<40}\n", refcount,
Simon> addr, filename);

Nope, the idea is to continue to use the existing ui-out methods.  So
continuing the gdb_bfd example, this code in print_one_bfd:

  uiout->field_int ("refcount", gdata->refc);
  uiout->field_string ("addr", host_address_to_string (abfd));
  uiout->field_string ("filename", bfd_get_filename (abfd));

... would not change at all.

The idea is that the format would be registered as a set/show command
somewhere.  For tables, cli-out could look up the appropriate format
string using the table name; not sure how tuples or lists would work.

Simon> How does the underlying implementation of format knows that the
Simon> refcount variable is an int, and that addr and filename are char*?  We
Simon> go by field name too?  Or should the format specifier contain the data
Simon> type?

The above answers, but basically it would work like today -- the type is
implicit in the ui-out calls, though really at the bottom layer it is
all just strings.

Simon> I tend to think that introducing a format-based system like this is
Simon> relatively orthogonal to the coloring/styling issue.

Part of the overall idea was to let users put styling directly in the
format strings -- sorry, maybe I didn't really spell this out.  So
another example would be

  "{fg=red}{field<40}{fg=default}"

... or something along those lines.  This would conflict a bit with the
idea of styling elements the way that the current patch does it, since
then you'd have to decide what takes precedence.

Though, your note makes me think that this potential conflict really
isn't so bad and we could push forward with the current approach and
deal with more customizable formatting later.

Tom