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

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

[Arsen Arsenović]

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.

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

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

[Eli Zaretskii]

> From: Arsen Arsenović <arsen@aarsen.me>
> Cc: Arsen Arsenović <arsen@aarsen.me>,
> 	Eli Zaretskii <eliz@gnu.org>
> Date: Sat,  4 Nov 2023 16:29:50 +0100
> 
>  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(-)

Thanks, the documentation parts are OK.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>

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

[Andrew Burgess]

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

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

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 2880 bytes --]

Hi Andrew,

Andrew Burgess <aburgess@redhat.com> writes:

> 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!

Hmm, I never thought about reading the body separate of the summary line
(just the inverse, reading the summary without the body present).  What
do you think of the following:

--8<---------------cut here---------------start------------->8---
The new 'vsplit' layout is analogous to the old 'split' TUI layout, but
instead of splitting the screen into three horizontal sections, it
splits the screen into two, and then the top of the split into two,
where the top left pane is source and the top right pane is assembly,
and the bottom pane is the command pane, with a status line between the
top two and the bottom panes.  This allows GDB to better utilize screen
real-estate on wide-screen displays that are commonly used nowadays.

A crude drawing of this layout looks like:

   +---------------+---------------+
   |               |               |
   |      Src      |      Asm      |
   |               |               |
   +---------------+---------------+
   | Status Line                   |
   +-------------------------------+
   |                               |
   |      Command Pane             |
   |                               |
   +-------------------------------+
--8<---------------cut here---------------end--------------->8---

> 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?

The stack is horizontal, but the 'main' split on the screen is vertical
(hence, 'vertically split' => 'vsplit').

> 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?

Yes, I have it already.  This layout was even already in the testsuite!
I shared the story of how this patch came to be in v1:
https://inbox.sourceware.org/20231104014343.3199584-1-arsen@aarsen.me/

I was also initially double-thinking sending this patch in, but I
figured that if multiple people use it, and if it's similar in spirit to
existing layouts, it's probably not /too/ redundant.

Naturally, if you disagree, that's alright - this is just a suggestion.

Thanks, have a lovely day :-)
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

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

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

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

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

FWIW I tend to support adding a few more layouts, including this one.
I don't know what criteria to use to accept or reject them, though.

One thing in particular is that if the 'locals' patch goes in, it would
be good to have a built-in layout that shows it.

Tom