From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 24A15385E003 for ; Wed, 10 May 2023 15:09:54 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 24A15385E003 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id DC3BB219BB; Wed, 10 May 2023 15:09:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1683731392; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dvCKxqIsGjnAzKnxkrxdQTNle4gAYT4Fe0YFu1m6YTw=; b=t/b4qGbFlgwBJIJCKhZg7JxCLf3IKm3i/cPmxPVz0WypiFrYke+qhlrX+QaBxLU7SZCLNo G3cJUKYieo1LRtIcqA5wFweoa10rFXIk3Gd/ODra/G9WRSjaCCtFMsoiaLNV+4mpx+KmSD J0IS/Mkfgen5rnEtU2yXvZMw5uZSJhA= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1683731392; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=dvCKxqIsGjnAzKnxkrxdQTNle4gAYT4Fe0YFu1m6YTw=; b=/RAYeaRUVPBdzlK66U6MSqemqBOzFm6RPgA/30UZ/lwIETw6+Gwmtr2J16oFqfDscOGB98 idhRtPUUuzQl6mDQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 7B3E713519; Wed, 10 May 2023 15:09:52 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id 79maGcCzW2QMSAAAMHmgww (envelope-from ); Wed, 10 May 2023 15:09:52 +0000 Message-ID: Date: Wed, 10 May 2023 17:09:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCH] [gdb/tui] Make tui compact-source more compact To: Andrew Burgess , gdb-patches@sourceware.org Cc: Tom Tromey References: <20230510062015.20974-1-tdevries@suse.de> <87jzxg7e4b.fsf@redhat.com> Content-Language: en-US From: Tom de Vries In-Reply-To: <87jzxg7e4b.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 5/10/23 11:03, Andrew Burgess wrote: > Tom de Vries via Gdb-patches 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 . >> + >> +# 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 >