Use less horizontal space in source window

  The source window currently uses a field width of 6 for line numbers,
and it further aligns to the next tab stop.

This seemed a bit wasteful of horizontal space to me.  This patch
changes the TUI to compute the maximum field width needed for the
current source file, and to only add a single space after the line
number.  Line numbers are now right justified, as well, which I think
also looks better visually when scrolling.

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

	* tui/tui-source.c (tui_set_source_content): Compute width of line
	numbers.
	* tui/tui-source.c (copy_source_line): Add "ndigits" parameter.
	Emit fewer spaces between line number and text.
---
 gdb/ChangeLog        |  7 +++++++
 gdb/tui/tui-source.c | 29 ++++++++++++++++++-----------
 2 files changed, 25 insertions(+), 11 deletions(-)
  

* Tom Tromey <tom@tromey.com> [2019-08-15 20:36:31 -0600]:

> The source window currently uses a field width of 6 for line numbers,
> and it further aligns to the next tab stop.
> 
> This seemed a bit wasteful of horizontal space to me.  This patch
> changes the TUI to compute the maximum field width needed for the
> current source file, and to only add a single space after the line
> number.  Line numbers are now right justified, as well, which I think
> also looks better visually when scrolling.
> 
> gdb/ChangeLog
> 2019-08-15  Tom Tromey  <tom@tromey.com>
> 
> 	* tui/tui-source.c (tui_set_source_content): Compute width of line
> 	numbers.
> 	* tui/tui-source.c (copy_source_line): Add "ndigits" parameter.
> 	Emit fewer spaces between line number and text.
> ---
>  gdb/ChangeLog        |  7 +++++++
>  gdb/tui/tui-source.c | 29 ++++++++++++++++++-----------
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
> index 3d88f66d549..e43b8381fe5 100644
> --- a/gdb/tui/tui-source.c
> +++ b/gdb/tui/tui-source.c
> @@ -37,22 +37,22 @@
>  #include "tui/tui-source.h"
>  #include "gdb_curses.h"
>  
> +#include <math.h>
> +
>  /* A helper function for tui_set_source_content that extracts some
> -   source text from PTR.  LINE_NO is the line number; FIRST_COL is the
> -   first column to extract, and LINE_WIDTH is the number of characters
> -   to display.  Returns a string holding the desired text.  */
> +   source text from PTR.  LINE_NO is the line number; NDIGITS the
> +   number of digits to use for printing; FIRST_COL is the first column
> +   to extract, and LINE_WIDTH is the number of characters to display.
> +   Returns a string holding the desired text.  */
>  
>  static std::string
> -copy_source_line (const char **ptr, int line_no, int first_col,
> -		  int line_width)
> +copy_source_line (const char **ptr, int line_no, int ndigits,
> +		  int first_col, int line_width)
>  {
>    const char *lineptr = *ptr;
>  
>    /* Init the line with the line number.  */
> -  std::string result = string_printf ("%-6d", line_no);
> -  int len = result.size ();
> -  len = len - ((len / tui_tab_width) * tui_tab_width);
> -  result.append (len, ' ');
> +  std::string result = string_printf ("%*d ", ndigits, line_no);
>  
>    int column = 0;
>    char c;
> @@ -141,8 +141,10 @@ tui_set_source_content (tui_source_window_base *win_info,
>        nlines = (line_no + (win_info->height - 2)) - line_no;
>  
>        std::string srclines;
> +      const std::vector<off_t> *offsets;
>        if (!g_source_cache.get_source_lines (s, line_no, line_no + nlines,
> -					    &srclines))
> +					    &srclines)
> +	  || !g_source_cache.get_line_charpos (s, &offsets))
>  	{
>  	  if (!noerror)
>  	    {
> @@ -172,6 +174,11 @@ tui_set_source_content (tui_source_window_base *win_info,
>  	  win_info->start_line_or_addr.loa = LOA_LINE;
>  	  cur_line_no = win_info->start_line_or_addr.u.line_no = line_no;
>  
> +	  double l = log10 (offsets->size ());
> +	  int digits = (int) l;
> +	  if (l > digits)
> +	    ++digits;
> +

I think this calculation of digits is incorrect, for a file with
exactly 10 lines, log10(10) == 1, so digits == 1.  A better version is
I think:

	double l = log10 (offsets->size ());
	int digits = ((int) l) + 1;

Otherwise this looks good.

Thanks,
Andrew

>  	  const char *iter = srclines.c_str ();
>  	  win_info->content.resize (nlines);
>  	  while (cur_line < nlines)
> @@ -181,7 +188,7 @@ tui_set_source_content (tui_source_window_base *win_info,
>  
>  	      std::string text;
>  	      if (*iter != '\0')
> -		text = copy_source_line (&iter, cur_line_no,
> +		text = copy_source_line (&iter, cur_line_no, digits,
>  					 win_info->horizontal_offset,
>  					 line_width);
>  
> -- 
> 2.17.2
>
  

On 8/16/19 3:36 AM, Tom Tromey wrote:
> The source window currently uses a field width of 6 for line numbers,
> and it further aligns to the next tab stop.
> 
> This seemed a bit wasteful of horizontal space to me.  This patch
> changes the TUI to compute the maximum field width needed for the
> current source file, and to only add a single space after the line
> number.  Line numbers are now right justified, as well, which I think
> also looks better visually when scrolling.

I tried this out a bit, and IMHO the experience is worse than before, given
the left/right shifting when stepping as you move between source files.

E.g., try debugging gdb and running to main, and stepping into gdb_main.
And then all the way into captured_main_1.  I much prefer that the
source window remains in the same place as I step through all
these things.  

I'd probably be fine with reducing the width if we still had a reasonable
minimum that is enough to fit reasonably-sized source files.  I think that
if we change this, we should also have more than one space between the line
number and the source though.  One single space as in your patch makes it harder
to distinguish between what is a line and the source IMO.  More so with
styling disabled.

I have to say that I find it a bit odd to be optimizing the horizontal space,
since the sources I debug (gdb!) mostly wrap around 80 cols, and my terminal
is usually much larger than that, so I always have tons of empty space
on the right side of the source window.

Thanks,
Pedro Alves
  

Hello,

On Fri, 16 Aug 2019 at 16:55, Pedro Alves <palves@redhat.com> wrote:
>
> On 8/16/19 3:36 AM, Tom Tromey wrote:
> > The source window currently uses a field width of 6 for line numbers,
> > and it further aligns to the next tab stop.
> >
> > This seemed a bit wasteful of horizontal space to me.  This patch
> > changes the TUI to compute the maximum field width needed for the
> > current source file, and to only add a single space after the line
> > number.  Line numbers are now right justified, as well, which I think
> > also looks better visually when scrolling.
>
> I tried this out a bit, and IMHO the experience is worse than before, given
> the left/right shifting when stepping as you move between source files.
>
> E.g., try debugging gdb and running to main, and stepping into gdb_main.
> And then all the way into captured_main_1.  I much prefer that the
> source window remains in the same place as I step through all
> these things.
>
> I'd probably be fine with reducing the width if we still had a reasonable
> minimum that is enough to fit reasonably-sized source files.  I think that
> if we change this, we should also have more than one space between the line
> number and the source though.  One single space as in your patch makes it harder
> to distinguish between what is a line and the source IMO.  More so with
> styling disabled.

What if line numbering were inverted in colors (or made different in
another way) or maybe separated by a vertical line from the source? My
experience is with much wider sources, and I'd appreciate some
optimization of horizontal space usage in GDB TUI.

To me this change is definitely an improvement, so I'd like to have it
in some form, not dropped completely.

Thanks,
Ruslan

>
> I have to say that I find it a bit odd to be optimizing the horizontal space,
> since the sources I debug (gdb!) mostly wrap around 80 cols, and my terminal
> is usually much larger than that, so I always have tons of empty space
> on the right side of the source window.
>
> Thanks,
> Pedro Alves
  

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I tried this out a bit, and IMHO the experience is worse than before, given
Pedro> the left/right shifting when stepping as you move between source files.

Thanks for giving it a try.

BTW, normally I remember to push branches to github, so it's often easy
to try them by fetching and checking out the appropriate branch.  At
least, I find this simpler than applying patches from email.

Pedro> I'd probably be fine with reducing the width if we still had a reasonable
Pedro> minimum that is enough to fit reasonably-sized source files.  I think that
Pedro> if we change this, we should also have more than one space between the line
Pedro> number and the source though.  One single space as in your patch makes it harder
Pedro> to distinguish between what is a line and the source IMO.  More so with
Pedro> styling disabled.

Good points.  And about the styling -- I try never to have it off any
more, so I didn't notice that.

Pedro> I have to say that I find it a bit odd to be optimizing the horizontal space,
Pedro> since the sources I debug (gdb!) mostly wrap around 80 cols, and my terminal
Pedro> is usually much larger than that, so I always have tons of empty space
Pedro> on the right side of the source window.

I often (but not always) have an 80 column terminal, so when I have
debugged gdb, it's still made a bit of difference sometimes.

Tom
  

>>>>> "Ruslan" == Ruslan Kabatsayev <b7.10110111@gmail.com> writes:

Ruslan> What if line numbering were inverted in colors (or made different in
Ruslan> another way) or maybe separated by a vertical line from the source? My
Ruslan> experience is with much wider sources, and I'd appreciate some
Ruslan> optimization of horizontal space usage in GDB TUI.

We could certainly do anything like that.  Maybe a curses-style vertical
line would be good?

Ruslan> To me this change is definitely an improvement, so I'd like to have it
Ruslan> in some form, not dropped completely.

One option would be to make this selectable via "set tui something".

Tom