[gdb/tui] Make tui compact-source more compact

  I noticed with tui compact-source on, that when reducing the height of the
src window we go from:
...
|___09_  i++;                          |
|___10_  i++;                          |
+--------------------------------------+
...
to:
...
|___09_  i++;                          |
+--------------------------------------+
...
In other words, the digits used to print 9 remains 2, even though only 1 is
needed.

This is due to this line in tui_source_window::set_contents:
...
      int max_line_nr = std::max (lines_in_file, last_line_nr_in_window);
...
which takes both the last line in the window and the total lines in the source
file into account.

Make "tui compact-source" more compact by just using:
...
      int max_line_nr = last_line_nr_in_window;
...
such that we have:
...
|___9_  i++;                           |
+--------------------------------------+
...

Tested on x86_64-linux.
---
 gdb/testsuite/gdb.tui/compact-source-2.exp | 68 ++++++++++++++++++++++
 gdb/tui/tui-source.c                       |  3 +-
 2 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/compact-source-2.exp


base-commit: 2093c2af3c58da1fe63807dfea07d56afc6e7a8a
  

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

> I noticed with tui compact-source on, that when reducing the height of the
> src window we go from:
> ...
> |___09_  i++;                          |
> |___10_  i++;                          |
> +--------------------------------------+
> ...
> to:
> ...
> |___09_  i++;                          |
> +--------------------------------------+
> ...
> In other words, the digits used to print 9 remains 2, even though only 1 is
> needed.

The benefit of using two digits here is that as we scroll down, and
lines 10+ appear on the screen, the source code doesn't move 1 character
to the right.  This will happen again when line 100 appears.

The docs (as you quoted in commit 2093c2af3c58) say:

  The default display uses more space for line numbers and starts the
  source text at the next tab stop; the compact display uses only as
  much space as is needed for the line numbers in the current file, and
  only a single space to separate the line numbers from the source.

So the '09' is the expected behaviour for a file with a double digit
number of lines.

I think I prefer the original behaviour -- your change certainly makes
sense when looking at a pretty small src window, at a pretty small file,
but this feels like an edge case.

That said, I don't feel strongly enough to block the change if you
really want to see it merged -- but you'll need to update the docs.

>
> This is due to this line in tui_source_window::set_contents:
> ...
>       int max_line_nr = std::max (lines_in_file, last_line_nr_in_window);

That line is not correct I think.  Better would be something like:

  int max_line_nr = lines_in_file + nlines - 1;

this accounts for that at the limit we can scroll such that only the
last line of the source file is displayed, then the rest of the source
window can also have line numbers assigned.  A source file with 9 lines
will (currently) still see a source shift as the (empty) line 10 appears.

> ...
> which takes both the last line in the window and the total lines in the source
> file into account.
>
> Make "tui compact-source" more compact by just using:
> ...
>       int max_line_nr = last_line_nr_in_window;
> ...
> such that we have:
> ...
> |___9_  i++;                           |
> +--------------------------------------+
> ...
>
> Tested on x86_64-linux.
> ---
>  gdb/testsuite/gdb.tui/compact-source-2.exp | 68 ++++++++++++++++++++++
>  gdb/tui/tui-source.c                       |  3 +-
>  2 files changed, 69 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.tui/compact-source-2.exp
>
> diff --git a/gdb/testsuite/gdb.tui/compact-source-2.exp b/gdb/testsuite/gdb.tui/compact-source-2.exp
> new file mode 100644
> index 00000000000..416c76e1d26
> --- /dev/null
> +++ b/gdb/testsuite/gdb.tui/compact-source-2.exp
> @@ -0,0 +1,68 @@
> +# Copyright 2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Check that "set tui compact-source on" has the intended effect.

This comment will be true once the docs have changed.

> +
> +require allow_tui_tests
> +
> +tuiterm_env
> +
> +standard_testfile
> +
> +# Let's generate the source file.  We want a short file, with more than 10
> +# lines, but with the first line in main below 10.
> +set src_list \
> +    [list \
> +	 "int" \
> +	 "main (void)" \
> +	 "{" \
> +	 "  int i = 1;" \
> +	 "  i++;" \
> +	 "  i++;" \
> +	 "  i++;" \
> +	 "  i++;" \
> +	 "  i++;" \
> +	 "  return 0;" \
> +	 "}"]
> +set line_four [lindex $src_list 3]
> +set src_txt [join $src_list "\n"]
> +set srcfile [standard_output_file $srcfile]
> +set fd [open $srcfile w]
> +puts $fd $src_txt
> +close $fd
> +
> +if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
> +    return -1
> +}
> +
> +Term::clean_restart 17 80 $binfile
> +
> +gdb_test_no_output "maint set tui-left-margin-verbose on"
> +gdb_test_no_output "set tui compact-source on"
> +
> +if {![Term::enter_tui]} {
> +    unsupported "TUI not supported"
> +    return
> +}
> +
> +set re_border "\\|"
> +Term::check_contents "compact source format" \
> +    "${re_border}___04_$line_four *$re_border"
> +
> +with_test_prefix window-resize=1 {

Minor nit: that prefix doesn't seem to reflect what actually changes
(height -= 1 vs height == 1).

Thanks,
Andrew


> +    Term::command "wh src -1"
> +    Term::check_contents "compact source" \
> +	"${re_border}___4_$line_four *$re_border"
> +}
> diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
> index 1233e945cab..bef3d3163dd 100644
> --- a/gdb/tui/tui-source.c
> +++ b/gdb/tui/tui-source.c
> @@ -79,9 +79,8 @@ tui_source_window::set_contents (struct gdbarch *arch,
>      {
>        /* Solaris 11+gcc 5.5 has ambiguous overloads of log10, so we
>  	 cast to double to get the right one.  */
> -      int lines_in_file = offsets->size ();
>        int last_line_nr_in_window = line_no + nlines - 1;
> -      int max_line_nr = std::max (lines_in_file, last_line_nr_in_window);
> +      int max_line_nr = last_line_nr_in_window;
>        int digits_needed = 1 + (int)log10 ((double) max_line_nr);
>        int trailing_space = 1;
>        m_digits = digits_needed + trailing_space;
>
> base-commit: 2093c2af3c58da1fe63807dfea07d56afc6e7a8a
> -- 
> 2.35.3
  

On 5/10/23 11:03, Andrew Burgess wrote:
> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> I noticed with tui compact-source on, that when reducing the height of the
>> src window we go from:
>> ...
>> |___09_  i++;                          |
>> |___10_  i++;                          |
>> +--------------------------------------+
>> ...
>> to:
>> ...
>> |___09_  i++;                          |
>> +--------------------------------------+
>> ...
>> In other words, the digits used to print 9 remains 2, even though only 1 is
>> needed.
> 
> The benefit of using two digits here is that as we scroll down, and
> lines 10+ appear on the screen, the source code doesn't move 1 character
> to the right.  This will happen again when line 100 appears.
> 
> The docs (as you quoted in commit 2093c2af3c58) say:
> 
>    The default display uses more space for line numbers and starts the
>    source text at the next tab stop; the compact display uses only as
>    much space as is needed for the line numbers in the current file, and
>    only a single space to separate the line numbers from the source.
> 
> So the '09' is the expected behaviour for a file with a double digit
> number of lines.
> 

Thanks for pointing that out, somehow I read over that.

> I think I prefer the original behaviour -- your change certainly makes
> sense when looking at a pretty small src window, at a pretty small file,
> but this feels like an edge case.
> 
> That said, I don't feel strongly enough to block the change if you
> really want to see it merged -- but you'll need to update the docs.
> 

Ack, but now that you've pointed out how this is supposed to work, I'm 
fine with keeping the documented behaviour.  At it happens, it's not 
current behaviour, and I've submitted a patch to fix that, using your 
suggested change below here ( 
https://sourceware.org/pipermail/gdb-patches/2023-May/199470.html ).

>>
>> This is due to this line in tui_source_window::set_contents:
>> ...
>>        int max_line_nr = std::max (lines_in_file, last_line_nr_in_window);
> 
> That line is not correct I think.  Better would be something like:
> 
>    int max_line_nr = lines_in_file + nlines - 1;
> 
> this accounts for that at the limit we can scroll such that only the
> last line of the source file is displayed, then the rest of the source
> window can also have line numbers assigned.  A source file with 9 lines
> will (currently) still see a source shift as the (empty) line 10 appears.
> 
>> ...
>> which takes both the last line in the window and the total lines in the source
>> file into account.
>>
>> Make "tui compact-source" more compact by just using:
>> ...
>>        int max_line_nr = last_line_nr_in_window;
>> ...
>> such that we have:
>> ...
>> |___9_  i++;                           |
>> +--------------------------------------+
>> ...
>>
>> Tested on x86_64-linux.
>> ---
>>   gdb/testsuite/gdb.tui/compact-source-2.exp | 68 ++++++++++++++++++++++
>>   gdb/tui/tui-source.c                       |  3 +-
>>   2 files changed, 69 insertions(+), 2 deletions(-)
>>   create mode 100644 gdb/testsuite/gdb.tui/compact-source-2.exp
>>
>> diff --git a/gdb/testsuite/gdb.tui/compact-source-2.exp b/gdb/testsuite/gdb.tui/compact-source-2.exp
>> new file mode 100644
>> index 00000000000..416c76e1d26
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.tui/compact-source-2.exp
>> @@ -0,0 +1,68 @@
>> +# Copyright 2023 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Check that "set tui compact-source on" has the intended effect.
> 
> This comment will be true once the docs have changed.
> 
>> +
>> +require allow_tui_tests
>> +
>> +tuiterm_env
>> +
>> +standard_testfile
>> +
>> +# Let's generate the source file.  We want a short file, with more than 10
>> +# lines, but with the first line in main below 10.
>> +set src_list \
>> +    [list \
>> +	 "int" \
>> +	 "main (void)" \
>> +	 "{" \
>> +	 "  int i = 1;" \
>> +	 "  i++;" \
>> +	 "  i++;" \
>> +	 "  i++;" \
>> +	 "  i++;" \
>> +	 "  i++;" \
>> +	 "  return 0;" \
>> +	 "}"]
>> +set line_four [lindex $src_list 3]
>> +set src_txt [join $src_list "\n"]
>> +set srcfile [standard_output_file $srcfile]
>> +set fd [open $srcfile w]
>> +puts $fd $src_txt
>> +close $fd
>> +
>> +if {[build_executable "failed to prepare" ${testfile} ${srcfile}] == -1} {
>> +    return -1
>> +}
>> +
>> +Term::clean_restart 17 80 $binfile
>> +
>> +gdb_test_no_output "maint set tui-left-margin-verbose on"
>> +gdb_test_no_output "set tui compact-source on"
>> +
>> +if {![Term::enter_tui]} {
>> +    unsupported "TUI not supported"
>> +    return
>> +}
>> +
>> +set re_border "\\|"
>> +Term::check_contents "compact source format" \
>> +    "${re_border}___04_$line_four *$re_border"
>> +
>> +with_test_prefix window-resize=1 {
> 
> Minor nit: that prefix doesn't seem to reflect what actually changes
> (height -= 1 vs height == 1).
> 

The idea there was that 1 was supposed to be a sequence number rather 
than a quantity, but anyway, I'm dropping this patch, and I've used this 
suggestion in the update of gdb.tui/compact-source.exp in the patch 
mentioned above.

Thanks,
- Tom

> Thanks,
> Andrew
> 
> 
>> +    Term::command "wh src -1"
>> +    Term::check_contents "compact source" \
>> +	"${re_border}___4_$line_four *$re_border"
>> +}
>> diff --git a/gdb/tui/tui-source.c b/gdb/tui/tui-source.c
>> index 1233e945cab..bef3d3163dd 100644
>> --- a/gdb/tui/tui-source.c
>> +++ b/gdb/tui/tui-source.c
>> @@ -79,9 +79,8 @@ tui_source_window::set_contents (struct gdbarch *arch,
>>       {
>>         /* Solaris 11+gcc 5.5 has ambiguous overloads of log10, so we
>>   	 cast to double to get the right one.  */
>> -      int lines_in_file = offsets->size ();
>>         int last_line_nr_in_window = line_no + nlines - 1;
>> -      int max_line_nr = std::max (lines_in_file, last_line_nr_in_window);
>> +      int max_line_nr = last_line_nr_in_window;
>>         int digits_needed = 1 + (int)log10 ((double) max_line_nr);
>>         int trailing_space = 1;
>>         m_digits = digits_needed + trailing_space;
>>
>> base-commit: 2093c2af3c58da1fe63807dfea07d56afc6e7a8a
>> -- 
>> 2.35.3
>