[RFA,0/4] Improve "show style", use style in "help" and "apropos".

Message ID 02aed9b6-1ce0-b48f-ed01-ef0fc0fa20b5@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves June 18, 2019, 1:09 p.m. UTC
  On 5/31/19 2:18 PM, Philippe Waroquiers wrote:
> Improve "show style", have "help" and "apropos" styling their output.
> 
> This patch series improves the "show style" output to let the
> user visualize all the styles with one command.
> 
> "help" and "apropos" commands are now styling their output.
> 
> The "help" testing is also better factorized.

I tried this new feature today, and I admit that I felt a bit lost
with the output.  I think the reason why is that I have pagination
disabled in my .gdbinit, usually preferring to scroll up/down
than to press enter to move pagination to the next page.  So
while scrolling up, it isn't immediately obvious to me where
each of the command's docs are split.

(
 another use for "with":
  (gdb) with pagination off -- apropos -v breakpoint
)

I tried the patchlet below, to add a "-------" separator line
between commands, and to me, the guidelines help.
It also helps when styling is disabled, as with:

  (gdb) | with pagination off -- apropos -v breakpoint | less

WDYT?

From ce57941ee6e1240f1b7c16374d6f6c18ea12053e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Jun 2019 13:25:15 +0100
Subject: [PATCH] apropos

---
 gdb/cli/cli-decode.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
  

Comments

Tom Tromey June 18, 2019, 4:03 p.m. UTC | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I tried the patchlet below, to add a "-------" separator line
Pedro> between commands, and to me, the guidelines help.
Pedro> It also helps when styling is disabled, as with:

This seems reasonable to me.

Pedro> +      rl_get_screen_size (nullptr, &width);

I was going to ask why not chars_per_line, but I see that can be
UINT_MAX.

Tom
  
Philippe Waroquiers June 18, 2019, 8:11 p.m. UTC | #2
On Tue, 2019-06-18 at 14:09 +0100, Pedro Alves wrote:
> I tried the patchlet below, to add a "-------" separator line
> between commands, and to me, the guidelines help.
> It also helps when styling is disabled, as with:
> 
>   (gdb) | with pagination off -- apropos -v breakpoint | less
> 
> WDYT?
Effectively, 'pagination off' and event worse 'style enabled off'
make it significantly more difficult to see where apropos -v 
starts the doc of a new command.

So, adding a separator line of ----------------- helps for such
cases.

I am just wondering if we then still need an empty line
before the ------ separator line.

I have a slight preference for the more compact one separator line
of ------------ (but of course, I will not have any nausea if we
keep the 2 lines separator).

Philippe
  
Tom Tromey June 18, 2019, 8:46 p.m. UTC | #3
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

>> (gdb) | with pagination off -- apropos -v breakpoint | less

Philippe> Effectively, 'pagination off' and event worse 'style enabled off'
Philippe> make it significantly more difficult to see where apropos -v 
Philippe> starts the doc of a new command.

Maybe this command should still use styling?  I wonder.

Tom
  
Philippe Waroquiers June 18, 2019, 8:52 p.m. UTC | #4
On Tue, 2019-06-18 at 14:46 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> > > (gdb) | with pagination off -- apropos -v breakpoint | less
> 
> Philippe> Effectively, 'pagination off' and event worse 'style enabled off'
> Philippe> make it significantly more difficult to see where apropos -v 
> Philippe> starts the doc of a new command.
> 
> Maybe this command should still use styling?  I wonder.

Wondering ...

At shell level, coloring seems to be disabled when piped.
E.g.  
  $ ls -l
  .... files with some colors
  $ ls -l | cat
  .... same output, but no colors
  $
  
Philippe Waroquiers June 18, 2019, 8:57 p.m. UTC | #5
On Tue, 2019-06-18 at 22:52 +0200, Philippe Waroquiers wrote:
> On Tue, 2019-06-18 at 14:46 -0600, Tom Tromey wrote:
> > > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> > > > 
> > > > (gdb) | with pagination off -- apropos -v breakpoint | less
> > 
> > Philippe> Effectively, 'pagination off' and event worse 'style enabled off'
> > Philippe> make it significantly more difficult to see where apropos -v 
> > Philippe> starts the doc of a new command.
> > 
> > Maybe this command should still use styling?  I wonder.
> 
> Wondering ...
> 
> At shell level, coloring seems to be disabled when piped.
> E.g.  
>   $ ls -l
>   .... files with some colors
>   $ ls -l | cat
>   .... same output, but no colors
>   $

ls has an option to control coloring.
   ls -l --color=always|cat
will still be colored.

So, we might add an option -style [on|off] to the GDB pipe_command,
if we believe it should output styling sequence.

Philippe
  
Pedro Alves June 18, 2019, 10:40 p.m. UTC | #6
On 6/18/19 9:57 PM, Philippe Waroquiers wrote:
>> Wondering ...
>>
>> At shell level, coloring seems to be disabled when piped.
>> E.g.  
>>   $ ls -l
>>   .... files with some colors
>>   $ ls -l | cat
>>   .... same output, but no colors
>>   $
> ls has an option to control coloring.
>    ls -l --color=always|cat
> will still be colored.
> 
> So, we might add an option -style [on|off] to the GDB pipe_command,
> if we believe it should output styling sequence.

Agreed.

In other words, this would let us pipe with style.  :-D

In addition, "set style enabled" could be made an auto-boolean
setting, with default "auto", enable styling iff outputting to
a terminal.  Then, "on" would force styling even if outputting
to a pipe.  This would let you force styling with:

 (gdb) with style enabled on -- | apropos -v breakpoint | less

Thanks,
Pedro Alves
  
Philippe Waroquiers June 19, 2019, 12:25 a.m. UTC | #7
On Tue, 2019-06-18 at 23:40 +0100, Pedro Alves wrote:
> On 6/18/19 9:57 PM, Philippe Waroquiers wrote:
> > > Wondering ...
> > > 
> > > At shell level, coloring seems to be disabled when piped.
> > > E.g.  
> > >   $ ls -l
> > >   .... files with some colors
> > >   $ ls -l | cat
> > >   .... same output, but no colors
> > >   $
> > 
> > ls has an option to control coloring.
> >    ls -l --color=always|cat
> > will still be colored.
> > 
> > So, we might add an option -style [on|off] to the GDB pipe_command,
> > if we believe it should output styling sequence.
> 
> Agreed.
> 
> In other words, this would let us pipe with style.  :-D
> 
> In addition, "set style enabled" could be made an auto-boolean
> setting, with default "auto", enable styling iff outputting to
> a terminal.  Then, "on" would force styling even if outputting
> to a pipe.  This would let you force styling with:
> 
>  (gdb) with style enabled on -- | apropos -v breakpoint | less

When we have this 'set style enabled auto|on|off',
wondering if the user will often have to override the behavior
in the pipe command:
The global setting will be enough for most of the users,
and the 'with' command should be good enough for the
unfrequent need to override styling for the pipe command.


Note that I am envisaging to do a small addition
to the 'define' command so as to make it slightly
easier to write a 'define wrapper shortcut' around
a 'with command'.

Something like:
define Lc
  with language c -- $arg@
end
document Lc
  Usage: Lc [COMMAND [ARGS]...]
  Shortcut for:
     with language c -- [COMMAND [ARGS]...]
end

This new '$arg@' will allow a user (like me) that
prefers to type as little as possible to define
very small shortcuts for often used 'with commands',
without expanding all arguments inside the 'define'
command with a bunch of 'if's depending on $argc.

Note that interactively, the 'with' command
combines properly with the 'if' command.

But the 'define' parser seems to not understand that
a 'with' line can start an 'if' that needs a
corresponding 'end'.

In other words, interactively, the below works
(gdb) with language c -- if $xxx == 0
 >echo zero\n
 >else
 >echo not zero\n
 >end
zero
(gdb) 

but inside a 'define', the parser does not understand
the above:

(gdb) define nnnn
Type commands for definition of "nnnn".
End with a line saying just "end".
>with language c -- if $xxx == 0
>echo zero\n
>else
(gdb) show user nnnn
User command "nnnn":
(gdb) 

Maybe worth adding a 'mini with command' detector/parser
in the 'define' parser, so as to allow user defined
commands to be less dependent of the current language
when evaluating expressions.

Alternatively, we should allow alias to accept 'arguments':
Instead of:
   (gdb) alias Lc = with language c --
   Invalid command to alias to: with language c --
   (gdb)
GDB should do:
   (gdb) alias Lc = with language c --
   => Defined Lc as an alias of 'with' command, using prefix args 'language c --'
   (gdb)
and then
   (gdb) Lc somecommand somearg someotherarg
should be the equivalent of:
   (gdb) with language c -- somecommand somearg someotherarg

No idea if the above is easy to do.

I have in a corner a patch that allows to add default arguments to
a command or an alias, which gives more or less the above, but using
2 successive actions:
  (gdb) alias bt2 = bt
  (gdb) help add-args 
  Specify additional arguments to preprend to user provided command arguments.
  Usage:  add-args COMMAND [= ADDITIONAL_ARGS...]
  Allows to specify or clear the additional arguments automatically
  prepended to the user provided arguments when COMMAND is run.
  Note that COMMAND can be an alias.  Aliases and their aliased commands
  do not share their additional arguments, so you can specify different
  additional arguments for a command and for each of its aliases.
  Without the [= ADDITIONAL_ARGS...], clears COMMAND additional arguments.
  (gdb) add-args bt2 = 2
  (gdb) bt2
  #0  0x00007ffff78fe603 in select () at ../sysdeps/unix/syscall-template.S:84
  #1  0x0000555555554f5e in sleeper_or_burner (v=0x7fffffffdf30) at sleepers.c:86
  (More stack frames follow...)
  (gdb) 

(add-args allows to implement Lc and similar, but also allows
a user to specify 'default' arguments for any command (or alias).
So, maybe I should finalize an RFA for this add-args idea in preference
to the 'alias argument idea' and/or to the '$arg@' idea.

Also maybe 'alias xxx = somecommand someargs'
can just be a fast way to type:
     alias xxx = somecommand
     add-args xxx = someargs

Too many ideas, too little time ... :(

Philippe

NB: the above discussion can be summarized as
   'the return of the slash command abbreviations for those that liked them' :)
  
Tom Tromey June 19, 2019, 7:26 p.m. UTC | #8
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> In other words, this would let us pipe with style.  :-D

Ouch.

Pedro> In addition, "set style enabled" could be made an auto-boolean
Pedro> setting, with default "auto", enable styling iff outputting to
Pedro> a terminal.  Then, "on" would force styling even if outputting
Pedro> to a pipe.  This would let you force styling with:

Pedro>  (gdb) with style enabled on -- | apropos -v breakpoint | less

I like this idea.

Tom
  
Tom Tromey June 19, 2019, 7:43 p.m. UTC | #9
>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> Note that I am envisaging to do a small addition
Philippe> to the 'define' command so as to make it slightly
Philippe> easier to write a 'define wrapper shortcut' around
Philippe> a 'with command'.

Philippe> Something like:
Philippe> define Lc
Philippe>   with language c -- $arg@
Philippe> end

Philippe> But the 'define' parser seems to not understand that
Philippe> a 'with' line can start an 'if' that needs a
Philippe> corresponding 'end'.

I'm surprised "with if" is intended to work, but if it is, then that's
just a bug.

If it needs special handling for "if" it will probably also need it for
the other multi-line commands.

Philippe> Alternatively, we should allow alias to accept 'arguments':
Philippe> Instead of:
Philippe>    (gdb) alias Lc = with language c --
Philippe>    Invalid command to alias to: with language c --

I think that would be a reasonable addition.  I was surprised this
wasn't done initially.

Philippe> I have in a corner a patch that allows to add default arguments to
Philippe> a command or an alias, which gives more or less the above, but using
Philippe> 2 successive actions:
Philippe>   (gdb) alias bt2 = bt
Philippe>   (gdb) help add-args 
Philippe>   Specify additional arguments to preprend to user provided command arguments.
Philippe>   Usage:  add-args COMMAND [= ADDITIONAL_ARGS...]
Philippe>   Allows to specify or clear the additional arguments automatically
Philippe>   prepended to the user provided arguments when COMMAND is run.

I am not so sure about this.  It seems pretty obscure.  Maybe a more
gdb-ish way of doing this would be to introduce "set" parameters to
control the defaults of the relevant commands.

Philippe> So, maybe I should finalize an RFA for this add-args idea in preference
Philippe> to the 'alias argument idea' and/or to the '$arg@' idea.

I'm also not sure about the $arg@ idea, or maybe just the spelling of
it.

Tom
  
Pedro Alves June 19, 2019, 10:56 p.m. UTC | #10
On 6/19/19 8:26 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> In other words, this would let us pipe with style.  :-D
> 
> Ouch.

???

/me wonders, surprised, what he might unknowingly might have said, and googles.

and... ouch...  I had no idea.  :-P

Always keep learning.

Pedro Alves
  
Tom Tromey June 20, 2019, 2:21 p.m. UTC | #11
Pedro> In other words, this would let us pipe with style.  :-D

>> Ouch.

Pedro> ???
Pedro> /me wonders, surprised, what he might unknowingly might have said, and googles.
Pedro> and... ouch...  I had no idea.  :-P

Pedro> Always keep learning.

Haha.  It's nothing really, it just fell into "bad pun" territory for me.

Tom
  

Patch

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index a6ddd8cc6d8..ca5751dc2f6 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -25,6 +25,7 @@ 
 #include "cli/cli-decode.h"
 #include "cli/cli-style.h"
 #include "common/gdb_optional.h"
+#include "readline/readline.h"
 
 /* Prototypes for local functions.  */
 
@@ -970,7 +971,18 @@  print_doc_of_command (struct cmd_list_element *c, const char *prefix,
      this documentation from the previous command help, in the likely
      case that apropos finds several commands.  */
   if (verbose)
-    fputs_filtered ("\n", stream);
+    {
+      fputc_filtered ('\n', stream);
+
+      int width;
+      rl_get_screen_size (nullptr, &width);
+      if (width > 80)
+	width = 80;
+
+      for (int i = 0; i < width; i++)
+	fputc_filtered ('-', stream);
+      fputc_filtered ('\n', stream);
+    }
 
   fprintf_styled (stream, title_style.style (),
 		  "%s%s", prefix, c->name);