Patchwork Truncate long TUI window titles

login
register
mail settings
Submitter Tom Tromey
Date Aug. 30, 2019, 9:28 p.m.
Message ID <20190830212834.22131-1-tom@tromey.com>
Download mbox | patch
Permalink /patch/34364/
State New
Headers show

Comments

Tom Tromey - Aug. 30, 2019, 9:28 p.m.
If a TUI window has a long title, it can overflow the title line.
This changes the TUI to use just the tail part of the title in this
case.

gdb/ChangeLog
2019-08-30  Tom Tromey  <tom@tromey.com>

	* tui/tui-wingeneral.c (box_win): Truncate long window titles.

gdb/testsuite/ChangeLog
2019-08-30  Tom Tromey  <tom@tromey.com>

	* gdb.tui/resize.exp: Remove setup_xfail.
	* gdb.tui/regs.exp: Remove setup_xfail.
	* gdb.tui/basic.exp: Remove setup_xfail.
---
 gdb/ChangeLog                    |  4 ++++
 gdb/testsuite/ChangeLog          |  6 ++++++
 gdb/testsuite/gdb.tui/basic.exp  |  6 ------
 gdb/testsuite/gdb.tui/regs.exp   |  3 ---
 gdb/testsuite/gdb.tui/resize.exp |  2 --
 gdb/tui/tui-wingeneral.c         | 16 +++++++++++++++-
 6 files changed, 25 insertions(+), 12 deletions(-)
Andrew Burgess - Sept. 3, 2019, 2:15 p.m.
* Tom Tromey <tom@tromey.com> [2019-08-30 15:28:34 -0600]:

> If a TUI window has a long title, it can overflow the title line.
> This changes the TUI to use just the tail part of the title in this
> case.
> 
> gdb/ChangeLog
> 2019-08-30  Tom Tromey  <tom@tromey.com>
> 
> 	* tui/tui-wingeneral.c (box_win): Truncate long window titles.
> 
> gdb/testsuite/ChangeLog
> 2019-08-30  Tom Tromey  <tom@tromey.com>
> 
> 	* gdb.tui/resize.exp: Remove setup_xfail.
> 	* gdb.tui/regs.exp: Remove setup_xfail.
> 	* gdb.tui/basic.exp: Remove setup_xfail.

Nice!  I had just one small question...

> ---
>  gdb/ChangeLog                    |  4 ++++
>  gdb/testsuite/ChangeLog          |  6 ++++++
>  gdb/testsuite/gdb.tui/basic.exp  |  6 ------
>  gdb/testsuite/gdb.tui/regs.exp   |  3 ---
>  gdb/testsuite/gdb.tui/resize.exp |  2 --
>  gdb/tui/tui-wingeneral.c         | 16 +++++++++++++++-
>  6 files changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
> index f60011c386a..716c52f68a6 100644
> --- a/gdb/testsuite/gdb.tui/basic.exp
> +++ b/gdb/testsuite/gdb.tui/basic.exp
> @@ -35,9 +35,6 @@ gdb_assert {![string match "No Source Available" $text]} \
>  Term::command "list main"
>  Term::check_contents "list main" "21 *return 0"
>  
> -# This check fails because the file name in the title overwrites the
> -# box.
> -setup_xfail *-*-*
>  Term::check_box "source box" 0 0 80 15
>  
>  Term::command "layout asm"
> @@ -48,8 +45,5 @@ Term::check_box "asm box" 0 0 80 15
>  Term::command "layout split"
>  Term::check_contents "split layout contents" "21 *return 0.*$hex <main>"
>  
> -# This check fails because the file name in the title overwrites the
> -# box.
> -setup_xfail *-*-*
>  Term::check_box "source box in split layout" 0 0 80 8
>  Term::check_box "asm box in split layout" 0 7 80 8
> diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
> index f17b1db8d40..1af943dd152 100644
> --- a/gdb/testsuite/gdb.tui/regs.exp
> +++ b/gdb/testsuite/gdb.tui/regs.exp
> @@ -38,9 +38,6 @@ Term::check_contents "source at startup" ">|21 *return 0"
>  
>  Term::command "layout regs"
>  Term::check_box "register box" 0 0 80 8
> -# This check fails because the file name in the title overwrites the
> -# box.
> -setup_xfail *-*-*
>  Term::check_box "source box in regs layout" 0 7 80 8
>  
>  set text [Term::get_line 1]
> diff --git a/gdb/testsuite/gdb.tui/resize.exp b/gdb/testsuite/gdb.tui/resize.exp
> index 8484e03c717..3b885e1198e 100644
> --- a/gdb/testsuite/gdb.tui/resize.exp
> +++ b/gdb/testsuite/gdb.tui/resize.exp
> @@ -37,6 +37,4 @@ if {![Term::enter_tui]} {
>  Term::check_contents "source at startup" ">|21 *return 0"
>  
>  Term::resize 40 90
> -# Resizing seems to be somewhat broken.
> -setup_xfail *-*-*
>  Term::check_box "source box after resize" 0 0 90 23
> diff --git a/gdb/tui/tui-wingeneral.c b/gdb/tui/tui-wingeneral.c
> index f900eab0133..0291bf1a335 100644
> --- a/gdb/tui/tui-wingeneral.c
> +++ b/gdb/tui/tui-wingeneral.c
> @@ -74,7 +74,21 @@ box_win (struct tui_win_info *win_info,
>    box (win, tui_border_vline, tui_border_hline);
>  #endif
>    if (!win_info->title.empty ())
> -    mvwaddstr (win, 0, 3, win_info->title.c_str ());
> +    {
> +      /* 2 chars for the corners, plus 3 more for the indent that's
> +	 used below.  */
> +      int max_len = win_info->width - 2 - 3;

I didn't really understand this comment.  Currently long titles will
be laid out like this:  +--...long title-+

You claim 2 for the corners, and 3 for the indent, but this seems an
odd way of describing it, why not just say 3 for the left edge and 2
for the right edge?

Also, is there a reason why you went with a 3/2 split for the top
border rather than having a balanced 3/3 or 2/2?

Thanks,
Andrew

> +
> +      if (win_info->title.size () <= max_len)
> +	mvwaddstr (win, 0, 3, win_info->title.c_str ());
> +      else
> +	{
> +	  std::string truncated
> +	    = "..." + win_info->title.substr (win_info->title.size ()
> +					      - max_len + 3);
> +	  mvwaddstr (win, 0, 3, truncated.c_str ());
> +	}
> +    }
>    wattroff (win, attrs);
>  }
>  
> -- 
> 2.17.2
>
Tom Tromey - Sept. 3, 2019, 11:33 p.m.
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

>> +      /* 2 chars for the corners, plus 3 more for the indent that's
>> +	 used below.  */
>> +      int max_len = win_info->width - 2 - 3;

Andrew> I didn't really understand this comment.  Currently long titles will
Andrew> be laid out like this:  +--...long title-+

Andrew> You claim 2 for the corners, and 3 for the indent, but this seems an
Andrew> odd way of describing it, why not just say 3 for the left edge and 2
Andrew> for the right edge?

No reason, I will change it.

Andrew> Also, is there a reason why you went with a 3/2 split for the top
Andrew> border rather than having a balanced 3/3 or 2/2?

No particularly good reason.  I didn't change the left hand side to
preserve the historical decision.  On the right I thought it would be
good to use as much space as possible, but still leave one "-" to make
it still resemble a line.  3 would work just as well -- I just didn't
give it a whole lot of consideration.

One thing I did consider is trying harder to truncate the file name at a
directory boundary.  This can always be done if desirable, though.

Tom
Andrew Burgess - Sept. 4, 2019, 11:07 a.m.
* Tom Tromey <tom@tromey.com> [2019-09-03 17:33:33 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> >> +      /* 2 chars for the corners, plus 3 more for the indent that's
> >> +	 used below.  */
> >> +      int max_len = win_info->width - 2 - 3;
> 
> Andrew> I didn't really understand this comment.  Currently long titles will
> Andrew> be laid out like this:  +--...long title-+
> 
> Andrew> You claim 2 for the corners, and 3 for the indent, but this seems an
> Andrew> odd way of describing it, why not just say 3 for the left edge and 2
> Andrew> for the right edge?
> 
> No reason, I will change it.

Thanks, I just got confused as I assumed "indent below" was the
"...".  I get confused easily!

> 
> Andrew> Also, is there a reason why you went with a 3/2 split for the top
> Andrew> border rather than having a balanced 3/3 or 2/2?
> 
> No particularly good reason.  I didn't change the left hand side to
> preserve the historical decision.  On the right I thought it would be
> good to use as much space as possible, but still leave one "-" to make
> it still resemble a line.  3 would work just as well -- I just didn't
> give it a whole lot of consideration.

I don't really mind, it just looked a bit weird and I thought there
might be an off-by-one bug in the code. I'd personally be happy with a
2/2 split.

In fact, given that no (sane) person is going to be pattern matching
against the TUI output we could be a little more dynamic.  What if we
kept the 3 on the left for short titles, but moved to 2 of the left
once titles get long enough?

But meh, the patch works for me, and is a million times better than
before, so I'm happy with whatever you choose to push.

Thanks,
Andrew

> 
> One thing I did consider is trying harder to truncate the file name at a
> directory boundary.  This can always be done if desirable, though.
> 
> Tom
Tom Tromey - Sept. 8, 2019, 7:55 p.m.
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> I don't really mind, it just looked a bit weird and I thought there
Andrew> might be an off-by-one bug in the code. I'd personally be happy with a
Andrew> 2/2 split.

I changed it to 2/2, and I updated the comment.  I'll push soon.

Tom

Patch

diff --git a/gdb/testsuite/gdb.tui/basic.exp b/gdb/testsuite/gdb.tui/basic.exp
index f60011c386a..716c52f68a6 100644
--- a/gdb/testsuite/gdb.tui/basic.exp
+++ b/gdb/testsuite/gdb.tui/basic.exp
@@ -35,9 +35,6 @@  gdb_assert {![string match "No Source Available" $text]} \
 Term::command "list main"
 Term::check_contents "list main" "21 *return 0"
 
-# This check fails because the file name in the title overwrites the
-# box.
-setup_xfail *-*-*
 Term::check_box "source box" 0 0 80 15
 
 Term::command "layout asm"
@@ -48,8 +45,5 @@  Term::check_box "asm box" 0 0 80 15
 Term::command "layout split"
 Term::check_contents "split layout contents" "21 *return 0.*$hex <main>"
 
-# This check fails because the file name in the title overwrites the
-# box.
-setup_xfail *-*-*
 Term::check_box "source box in split layout" 0 0 80 8
 Term::check_box "asm box in split layout" 0 7 80 8
diff --git a/gdb/testsuite/gdb.tui/regs.exp b/gdb/testsuite/gdb.tui/regs.exp
index f17b1db8d40..1af943dd152 100644
--- a/gdb/testsuite/gdb.tui/regs.exp
+++ b/gdb/testsuite/gdb.tui/regs.exp
@@ -38,9 +38,6 @@  Term::check_contents "source at startup" ">|21 *return 0"
 
 Term::command "layout regs"
 Term::check_box "register box" 0 0 80 8
-# This check fails because the file name in the title overwrites the
-# box.
-setup_xfail *-*-*
 Term::check_box "source box in regs layout" 0 7 80 8
 
 set text [Term::get_line 1]
diff --git a/gdb/testsuite/gdb.tui/resize.exp b/gdb/testsuite/gdb.tui/resize.exp
index 8484e03c717..3b885e1198e 100644
--- a/gdb/testsuite/gdb.tui/resize.exp
+++ b/gdb/testsuite/gdb.tui/resize.exp
@@ -37,6 +37,4 @@  if {![Term::enter_tui]} {
 Term::check_contents "source at startup" ">|21 *return 0"
 
 Term::resize 40 90
-# Resizing seems to be somewhat broken.
-setup_xfail *-*-*
 Term::check_box "source box after resize" 0 0 90 23
diff --git a/gdb/tui/tui-wingeneral.c b/gdb/tui/tui-wingeneral.c
index f900eab0133..0291bf1a335 100644
--- a/gdb/tui/tui-wingeneral.c
+++ b/gdb/tui/tui-wingeneral.c
@@ -74,7 +74,21 @@  box_win (struct tui_win_info *win_info,
   box (win, tui_border_vline, tui_border_hline);
 #endif
   if (!win_info->title.empty ())
-    mvwaddstr (win, 0, 3, win_info->title.c_str ());
+    {
+      /* 2 chars for the corners, plus 3 more for the indent that's
+	 used below.  */
+      int max_len = win_info->width - 2 - 3;
+
+      if (win_info->title.size () <= max_len)
+	mvwaddstr (win, 0, 3, win_info->title.c_str ());
+      else
+	{
+	  std::string truncated
+	    = "..." + win_info->title.substr (win_info->title.size ()
+					      - max_len + 3);
+	  mvwaddstr (win, 0, 3, truncated.c_str ());
+	}
+    }
   wattroff (win, attrs);
 }