Re: [PATCH] [gdb/tui] Make tui compact-source more compact

[Tom de Vries <tdevries@suse.de>]

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
>