[PATCH] [gdb/tui] Fix fingerprint for cmd-only layout

[PATCH] [gdb/tui] Fix fingerprint for cmd-only layout

[Tom de Vries]

I added a cmd-only layout:
...
(gdb) tui new-layout cmd cmd 1
...
and set it:
...
(gdb) layout cmd
...
which gave me the expect result: only the cmd window in the screen.

However, after going back to layout src:
...
(gdb) layout src
...
I got a source window with only one line in it, and the cmd window taking most
of the screen.

I traced this back to tui_set_layout, where for both the old and the new
layout the fingerprint of the cmd window in the layout is taken.  If the
fingerprint is the same, an effort will be done to preserve the command
window size.

The fingerprint is "VC" for both the old (cmd) and new (src) layouts, which
explains the behaviour.

I think this is essentially a bug in the finger print calculation, and it
should be "C" for the cmd layout.

Fix this by not adding a V or H in the fingerprint if the list size is one.

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.tui/new-layout.exp | 9 +++++++++
 gdb/tui/tui-layout.c                 | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.tui/new-layout.exp b/gdb/testsuite/gdb.tui/new-layout.exp
index eaebac88ce1..3085cc36232 100644
--- a/gdb/testsuite/gdb.tui/new-layout.exp
+++ b/gdb/testsuite/gdb.tui/new-layout.exp
@@ -142,3 +142,12 @@ foreach_with_prefix layout $layouts {
 			   "$gdb_prompt\\s+"]
     }
 }
+
+Term::command "layout src"
+Term::command "winheight cmd 8"
+Term::check_box "before cmd_only: src box in src layout" 0 0 80 15
+
+Term::command "layout cmd_only"
+
+Term::command "layout src"
+Term::check_box "after cmd_only: src box in src layout" 0 0 80 15
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index 50c568fb7d7..159445dc520 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -1101,7 +1101,7 @@ tui_layout_split::layout_fingerprint () const
   for (auto &item : m_splits)
     {
       std::string fp = item.layout->layout_fingerprint ();
-      if (!fp.empty ())
+      if (!fp.empty () && m_splits.size () != 1)
 	return std::string (m_vertical ? "V" : "H") + fp;
     }
 

base-commit: 85f4cf41a852b5983ca436615a019315f6dc7301
-- 
2.35.3

Re: [PATCH] [gdb/tui] Fix fingerprint for cmd-only layout

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> The fingerprint is "VC" for both the old (cmd) and new (src) layouts, which
Tom> explains the behaviour.

Tom> I think this is essentially a bug in the finger print calculation, and it
Tom> should be "C" for the cmd layout.

Tom> Fix this by not adding a V or H in the fingerprint if the list size is one.

I think this makes sense, but the comment in tui-layout.h needs to be
updated, since it says:

     When called on a complete, top-level layout, the fingerprint will be a
     non-empty string made of 'V' and 'H' characters, followed by a single
     'C' character.  Each 'V' and 'H' represents a vertical or horizontal

... but now the prefix could be empty in this corner case.

Tom

Re: [PATCH] [gdb/tui] Fix fingerprint for cmd-only layout

[Tom de Vries]

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

On 5/30/23 18:16, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> The fingerprint is "VC" for both the old (cmd) and new (src) layouts, which
> Tom> explains the behaviour.
> 
> Tom> I think this is essentially a bug in the finger print calculation, and it
> Tom> should be "C" for the cmd layout.
> 
> Tom> Fix this by not adding a V or H in the fingerprint if the list size is one.
> 
> I think this makes sense, but the comment in tui-layout.h needs to be
> updated, since it says:
> 
>       When called on a complete, top-level layout, the fingerprint will be a
>       non-empty string made of 'V' and 'H' characters, followed by a single
>       'C' character.  Each 'V' and 'H' represents a vertical or horizontal
> 
> ... but now the prefix could be empty in this corner case.

Thanks for the review.

Updated the comment and committed as attached.

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-tui-Fix-fingerprint-for-cmd-only-layout.patch --]
[-- Type: text/x-patch, Size: 3591 bytes --]

From 026a6ec31bbedbc1ff06b10e8cd021964f4eeca1 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Sun, 28 May 2023 16:36:41 +0200
Subject: [PATCH] [gdb/tui] Fix fingerprint for cmd-only layout

I added a cmd-only layout:
...
(gdb) tui new-layout cmd cmd 1
...
and set it:
...
(gdb) layout cmd
...
which gave me the expect result: only the cmd window in the screen.

However, after going back to layout src:
...
(gdb) layout src
...
I got a source window with only one line in it, and the cmd window taking most
of the screen.

I traced this back to tui_set_layout, where for both the old and the new
layout the fingerprint of the cmd window in the layout is taken.  If the
fingerprint is the same, an effort will be done to preserve the command
window size.

The fingerprint is "VC" for both the old (cmd) and new (src) layouts, which
explains the behaviour.

I think this is essentially a bug in the finger print calculation, and it
should be "C" for the cmd layout.

Fix this by not adding a V or H in the fingerprint if the list size is one.

Tested on x86_64-linux.

Reviewed-By: Tom Tromey <tom@tromey.com>
---
 gdb/testsuite/gdb.tui/new-layout.exp | 9 +++++++++
 gdb/tui/tui-layout.c                 | 2 +-
 gdb/tui/tui-layout.h                 | 5 +++--
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.tui/new-layout.exp b/gdb/testsuite/gdb.tui/new-layout.exp
index eaebac88ce1..3085cc36232 100644
--- a/gdb/testsuite/gdb.tui/new-layout.exp
+++ b/gdb/testsuite/gdb.tui/new-layout.exp
@@ -142,3 +142,12 @@ foreach_with_prefix layout $layouts {
 			   "$gdb_prompt\\s+"]
     }
 }
+
+Term::command "layout src"
+Term::command "winheight cmd 8"
+Term::check_box "before cmd_only: src box in src layout" 0 0 80 15
+
+Term::command "layout cmd_only"
+
+Term::command "layout src"
+Term::check_box "after cmd_only: src box in src layout" 0 0 80 15
diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c
index 50c568fb7d7..159445dc520 100644
--- a/gdb/tui/tui-layout.c
+++ b/gdb/tui/tui-layout.c
@@ -1101,7 +1101,7 @@ tui_layout_split::layout_fingerprint () const
   for (auto &item : m_splits)
     {
       std::string fp = item.layout->layout_fingerprint ();
-      if (!fp.empty ())
+      if (!fp.empty () && m_splits.size () != 1)
 	return std::string (m_vertical ? "V" : "H") + fp;
     }
 
diff --git a/gdb/tui/tui-layout.h b/gdb/tui/tui-layout.h
index ff354fb7c2f..a6d34851bef 100644
--- a/gdb/tui/tui-layout.h
+++ b/gdb/tui/tui-layout.h
@@ -111,7 +111,8 @@ class tui_layout_base
      non-empty string made of 'V' and 'H' characters, followed by a single
      'C' character.  Each 'V' and 'H' represents a vertical or horizontal
      layout that must be passed through in order to find the cmd
-     window.
+     window.  A vertical or horizontal layout of just one window does not add
+     a 'V' or 'H' character.
 
      Of course, layouts are built recursively, so, when called on a partial
      layout, if this object represents a single window, then either the
@@ -119,7 +120,7 @@ class tui_layout_base
      containing a single 'C' is returned.
 
      For object representing layouts, if the layout contains the cmd
-     window then we will get back a valid fingerprint string (contains 'V'
+     window then we will get back a valid fingerprint string (may contain 'V'
      and 'H', ends with 'C'), or, if this layout doesn't contain the cmd
      window, an empty string is returned.  */
   virtual std::string layout_fingerprint () const = 0;

base-commit: 2fee907cfd3dd9d8a8c0df7aa8ec29e7d2d1575e
-- 
2.35.3