Re: [PATCH v3] gdb/tui: add a vsplit layout

[Andrew Burgess <aburgess@redhat.com>]

Arsen Arsenović <arsen@aarsen.me> writes:

> The usual 'split' layout features a vertical stack that inadequately
> makes use of widely-used widescreen displays.  This layout displays the
> same information but in a horizontal stack in a way that's quite handy
> for debugging on wide screens.

An ideal commit message body should be readable without having to read
the title.  I notice you don't actually mention 'vsplit' in the body at
all!

Also, the name seems a little odd (to me), you even say:

  > The usual 'split' layout features a vertical stack...

And

  > This [new 'vsplit'] layout displays the same information but in a
  > horizontal stack...

So I'd have thought 'hsplit' would be a more natural name?

Also, I have to ask, as you've not mentioned it in the commit message.
Did you know that you can add this line to your ~/.gdbinit file?

  tui new-layout vsplit { -horizontal src 1 asm 1 } 1 status 1 cmd 1

And now you have the vsplit layout you want.  This feature was added in
part (I believe) to remove the need for every possible layout to be
added into GDB core.  Did you consider this as an option?

Thanks,
Andrew





>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> ---
> Afternoon,
>
> v2:
> https://inbox.sourceware.org/20231104130153.3649198-1-arsen@aarsen.me/
>
> Changes since v2:
> - Added a NEWS item.
>
> TIA, have a lovely day.
>
>  gdb/NEWS                             |  6 ++++++
>  gdb/doc/gdb.texinfo                  |  4 ++++
>  gdb/testsuite/gdb.tui/completion.exp |  2 +-
>  gdb/testsuite/gdb.tui/tui-layout.exp |  5 ++++-
>  gdb/tui/tui-layout.c                 | 11 +++++++++++
>  5 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 95663433f1c..af1480aaf1a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -17,6 +17,12 @@
>    ** New read/write attribute gdb.Value.bytes that contains a bytes
>       object holding the contents of this value.
>  
> +* GDB now includes a TUI layout called "vsplit", which places a source box, and
> +  to the right of it a disassembly box, and below both the status line and
> +  command box.  This layout is similar to the existing "split" layout, except
> +  with the split made vertically, so that it accommodates wide-screen displays
> +  better.  It can be enabled by running "tui layout vsplit".
> +
>  *** Changes in GDB 14
>  
>  * GDB now supports the AArch64 Scalable Matrix Extension 2 (SME2), which
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index db1a82ec838..209af3a4b0e 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -30604,6 +30604,10 @@ Display the assembly and command windows.
>  @item split
>  Display the source, assembly, and command windows.
>  
> +@item vsplit
> +Display the source and assembly side by side, and the command window
> +below them.
> +
>  @item regs
>  When in @code{src} layout display the register, source, and command
>  windows.  When in @code{asm} or @code{split} layout display the
> diff --git a/gdb/testsuite/gdb.tui/completion.exp b/gdb/testsuite/gdb.tui/completion.exp
> index 9cf8dc2ee25..4110045d117 100644
> --- a/gdb/testsuite/gdb.tui/completion.exp
> +++ b/gdb/testsuite/gdb.tui/completion.exp
> @@ -50,7 +50,7 @@ proc test_tab_completion {input_line expected_re} {
>  
>  if { [readline_is_used] } {
>      with_test_prefix "completion of layout names" {
> -	test_tab_completion "layout" "asm *next *prev *regs *split *src *"
> +	test_tab_completion "layout" "asm *next *prev *regs *split *src *vsplit *"
>      }
>  
>  
> diff --git a/gdb/testsuite/gdb.tui/tui-layout.exp b/gdb/testsuite/gdb.tui/tui-layout.exp
> index 90f27c5eac1..ea99d8f6ef9 100644
> --- a/gdb/testsuite/gdb.tui/tui-layout.exp
> +++ b/gdb/testsuite/gdb.tui/tui-layout.exp
> @@ -83,13 +83,16 @@ proc test_layout_or_focus {layout_name terminal execution} {
>  	} elseif {$layout_name == "split"} {
>  	    Term::check_box "src box" 0 0 80 8
>  	    Term::check_box "asm box" 0 7 80 8
> +	} elseif {$layout_name == "vsplit"} {
> +	    Term::check_box "src box" 0 0 40 15
> +	    Term::check_box "asm box" 39 0 41 15
>  	}
>      }
>  }
>  
>  foreach_with_prefix terminal {ansi dumb} {
>      foreach_with_prefix execution {false true} {
> -	foreach_with_prefix layout {"asm" "reg" "src" "split"} {
> +	foreach_with_prefix layout {"asm" "reg" "src" "split" "vsplit"} {
>  	    test_layout_or_focus $layout $terminal $execution
>  	}
>      }
> diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
> index 159445dc520..824ed282622 100644
> --- a/gdb/tui/tui-layout.c
> +++ b/gdb/tui/tui-layout.c
> @@ -1184,6 +1184,17 @@ initialize_layouts ()
>    layout->add_window (CMD_NAME, 1);
>    add_layout_command ("split", layout);
>  
> +  layout = new tui_layout_split ();
> +  {
> +    auto vsplit_top_half  = std::make_unique<tui_layout_split> (false);
> +    vsplit_top_half->add_window (SRC_NAME, 1);
> +    vsplit_top_half->add_window (DISASSEM_NAME, 1);
> +    layout->add_split (std::move (vsplit_top_half), 1);
> +    layout->add_window (STATUS_NAME, 0);
> +    layout->add_window (CMD_NAME, 1);
> +    add_layout_command ("vsplit", layout);
> +  }
> +
>    layout = new tui_layout_split ();
>    layout->add_window (DATA_NAME, 1);
>    layout->add_window (SRC_NAME, 1);
> -- 
> 2.42.1