From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 0CDFE38708B9 for ; Wed, 10 May 2023 15:22:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0CDFE38708B9 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-out2.suse.de (Postfix) with ESMTPS id 3F9FE1F88D; Wed, 10 May 2023 15:21:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1683732119; 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=FCEiG0HneAKN2hBnaAjvk5EB3sYpiMQhatGXxM0SQ2Q=; b=YRrPDvKVA8yB07p43iBf2lY9+IqpQjIrtU7ldjTmsCm+C9jPBXFYfzuoxb9MwZFIbmB36w DoyV6GPczyOtya4l5lYK+5hknn5KqQYdQ31bQjNuzepEecXQe8DXg4qliNgCEHKpeTKMEx 7VqP2ZW2woWlYeL9aP4nYPd+IvrYOmk= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1683732119; 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=FCEiG0HneAKN2hBnaAjvk5EB3sYpiMQhatGXxM0SQ2Q=; b=d2vwazZZ+J8Eyv/v0UzjLe12v0BXvSogDBn997tcSkb1VCvcAwf67uK9vQc1DrPyQrgxPd pyQncfW/scCpDyDA== 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 C56EF13519; Wed, 10 May 2023 15:21:58 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id p5pjLJa2W2TLTgAAMHmgww (envelope-from ); Wed, 10 May 2023 15:21:58 +0000 Message-ID: <55d283dc-1970-bcf5-4c44-d48ce68c5676@suse.de> Date: Wed, 10 May 2023 17:21:57 +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] Add set tui hide-last-column on/off/auto Content-Language: en-US To: Andrew Burgess , gdb-patches@sourceware.org Cc: Tom Tromey References: <20230509132004.22669-1-tdevries@suse.de> <87354470wt.fsf@redhat.com> From: Tom de Vries In-Reply-To: <87354470wt.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_ASCII_DIVIDERS,KAM_DMARC_STATUS,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 15:48, Andrew Burgess wrote: > Tom de Vries via Gdb-patches writes: > >> Consider: >> ... >> $ gdb -q -iex "set tui border-kind ascii" -tui >> ... >> >> This gets us the expected lower border of src window, status line and prompt: >> ... >> +----------------------------------------+ >> No process In: L?? PC: ?? >> (gdb) >> ... >> >> With TERM=ansi, we have instead: >> ... >> +----------------------------------------+ >> No process In: L?? PC: ??( >> gdb) >> ... >> This is PR tui/30388. After doing a ^L we get the expected result. >> >> There is a related problem with wrapping. >> >> Say we have a terminal with COLUMNS == 40, and we type a repetitive numerical >> string to just before wrapping, we have (annotating blinking cursor position >> using "_"): >> ... >> +--------------------------------------+ >> In: L?? PC: ?? >> (gdb) 789012345678901234567890123456789_ >> ... >> >> Now, after typing a 0, we have: >> ... >> +--------------------------------------+ >> In: L?? PC: ?? >> (gdb) 7890123456789012345678901234567890 >> ... >> So, there's no cursor anymore. >> >> After doing ^L we have: >> ... >> +--------------------------------------+ >> In: L?? PC: ?? >> (gdb) 7890123456789012345678901234567890 >> _ >> ... >> and now the cursor is stuck at the end of the next line. >> >> After typing another char, 1, we have: >> ... >> (gdb) 7890123456789012345678901234567890 >> 1 >> ... >> and again the cursor is gone. >> >> After doing ^L again, we have the expected: >> ... >> (gdb) 7890123456789012345678901234567890 >> 1_ >> ... >> >> Both of these problems are in a way self-inflicted, and not the fault of gdb >> or ncurses. >> >> When doing a TERM= export in a terminal emulator, we specify the >> capabilities of the terminal to the application. >> >> That is fine if we limit capabilities, by say overriding a default >> TERM=xterm-256color with TERM=xterm-16color. >> >> But it's different when we define TERM=ansi, because of the implications for >> the xenl flag: >> ... >> $ TERM=xterm tput xenl; echo $? >> 0 >> $ TERM=ansi tput xenl; echo $? >> 1 >> ... >> which stands for "eat newline glitch". >> >> When we run with TERM=ansi, we tell the application that the terminal doesn't >> have the glitch, while it still does, and you can expect to run into trouble. >> >> At this point, we could just disregard this as a user problem, but there is an >> interest in having this TERM=ansi working somewhat. This is because the >> native TERM for our tuiterm terminal emulator is ansi, and it's worthwhile: >> - being able to compare behaviour in the tuiterm and an actual terminal >> emulator like konsole or gnome-terminal (which typically will have the >> glitch), as well as >> - playing around with the TUI and explore new scenarios with TUI running in a >> mode as close as possible to tuiterm to increase the likeliness that the >> scenario can be reproduced in the tuiterm. >> >> So, can we do something about this? An observation that can be made is that >> TERM=ansi doesn't seem to be causing problems in CLI mode. This is because >> readline defines a concept "reliable auto-wrap", and considers that if the >> terminal capabilities include flags am and xenl, there's indeed reliable >> auto-wrap. Consequently, for TERM=ansi it considers there's no reliable >> auto-wrap, and deals with this by hiding the last column of the terminal. >> >> Fix both of the problems mentioned above by doing the same in TUI: if readline >> hides the last column, also hide the last column in TUI. >> >> This is a good solution, but for the tuiterm unnecessary, because it doesn't >> have the glitch, and there's no need for for TUI (or indeed CLI) to work >> around this. We could just update the test-cases, but it would make writing >> and debugging TUI test-cases less intuitive. >> >> So, we expose the fix at user level using a "set tui hide-last-column >> on/off/auto", with auto behaving like this: >> ... >> $ TERM=xterm gdb -q -ex "show tui hide-last-column" >> TUI last column hiding is "auto; currently off". >> $ TERM=ansi gdb -q -ex "show tui hide-last-column" >> TUI last column hiding is "auto; currently on". >> ... >> and use "set tui hide-last-column off" in the testsuite. >> >> [ FWIW, there is currently no way to switch off the hidden column in CLI, >> though I suppose it can be worked around by tricking readline and setting >> width to 81 in an 80 COLUMNS terminal. If that indeed works, we could >> introduce a similar workaround for CLI that does this under the hood, but atm >> this doesn't look useful to me. ] >> >> Tested on x86_64-linux. >> >> PR tui/30388 >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30388 > > I took a look at this patch, and the two bugs gdb/30388 and gdb/30419. > It wasn't clear to me if you were actually proposing this patch for > inclusion based on the comments in gdb/30419 - or if this was more RFC. > Hi Andrew, thanks for the review. I've submitted this intentionally as a patch, with the idea that it's a concrete solution to a hopefully clearly described problem. But I understand there could a discussion akin to what you'd have for an RFC in terms of: is this a problem that we want to fix, is the solution worth the cost of maintenance etc. Also, it could be suggested that auto is not a good default, and off would be better because it does not change current behaviour. Or, that it's better as a maintainance command. Anyway, if you have comments/question/recommendations on those issues, I'm happy to hear about it. > But I did have one incredibly minor nit... Yes, thanks for pointing that out. I'll fix that once we move closer to actually committing this, and settle on a default. Thanks, - Tom > >> --- >> gdb/doc/gdb.texinfo | 9 +++ >> gdb/testsuite/gdb.tui/resize.exp | 23 +++++++- >> gdb/testsuite/gdb.tui/wrap-line.exp | 13 +++++ >> gdb/testsuite/lib/tuiterm.exp | 14 +++++ >> gdb/tui/tui-layout.c | 7 ++- >> gdb/tui/tui-win.c | 88 ++++++++++++++++++++++++++++- >> gdb/tui/tui-win.h | 3 + >> 7 files changed, 151 insertions(+), 6 deletions(-) >> >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 8c4177c1901..f582f484704 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -30316,6 +30316,15 @@ 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. >> >> +@item set tui hide-last-column @r{[}on@r{|}off@r{|}auto@r{]} >> +@kindex set tui hide-last-column >> +Determines whether the TUI hides the last screen column. This is >> +useful when working around problems with incorrect terminal >> +capabilities, for instance when using a terminal emulator that has the >> +@code{xenl} capability while specifying a @env{TERM} setting such as >> +@code{ansi} that implies that it doesn't. If set to @code{auto}, TUI >> +will hide the last column if readline does the same. > > ... you should mention that 'auto' is the default. > > Thanks, > Andrew > >> + >> @kindex set debug tui >> @item set debug tui @r{[}on|off@r{]} >> Turn on or off display of @value{GDBN} internal debug messages relating >> diff --git a/gdb/testsuite/gdb.tui/resize.exp b/gdb/testsuite/gdb.tui/resize.exp >> index 60d5116886b..53ea6f990ba 100644 >> --- a/gdb/testsuite/gdb.tui/resize.exp >> +++ b/gdb/testsuite/gdb.tui/resize.exp >> @@ -35,7 +35,24 @@ if {![Term::enter_tui]} { >> return >> } >> >> -Term::check_contents "source at startup" "\\|.*21 *return 0" >> +with_test_prefix startup { >> + Term::check_contents "source" "\\|.*21 *return 0" >> + Term::check_box "source box" 0 0 80 15 >> +} >> + >> +with_test_prefix resize=1 { >> + Term::resize 40 90 >> + Term::check_box "source box" 0 0 90 26 >> +} >> >> -Term::resize 40 90 >> -Term::check_box "source box after resize" 0 0 90 26 >> +with_test_prefix hide-last-column=on { >> + Term::command "set tui hide-last-column on" >> + Term::check_box "source box" 0 0 89 26 >> + Term::check_contents_not "Hidden last colum empty" "\[^ \]\n" >> + >> + with_test_prefix resize=2 { >> + Term::resize 24 80 >> + Term::check_box "source box" 0 0 79 15 >> + Term::check_contents_not "Hidden last colum empty" "\[^ \]\n" >> + } >> +} >> diff --git a/gdb/testsuite/gdb.tui/wrap-line.exp b/gdb/testsuite/gdb.tui/wrap-line.exp >> index b28170808b8..f3d612fc7d0 100644 >> --- a/gdb/testsuite/gdb.tui/wrap-line.exp >> +++ b/gdb/testsuite/gdb.tui/wrap-line.exp >> @@ -126,6 +126,16 @@ proc test_wrap_cli_tui { auto_detected_width } { >> set wrap_width $::cols >> >> test_wrap $wrap_width >> + >> + with_test_prefix hide-last-column=on { >> + Term::command "set tui hide-last-column on" >> + >> + # Now that we've told curses to hide the last column, check that indeed >> + # we wrap one char earlier. >> + set wrap_width [expr $::cols - 1] >> + >> + test_wrap $wrap_width >> + } >> } >> } >> >> @@ -158,6 +168,9 @@ with_test_prefix width-auto-detected { >> } >> } >> >> + # As in Term::clean_restart. >> + gdb_test_no_output "set tui hide-last-column off" >> + >> # Run tests with auto-detected screen width. >> test_wrap_cli_tui 1 >> } >> diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp >> index 5e4235da942..ddf18ad2c34 100644 >> --- a/gdb/testsuite/lib/tuiterm.exp >> +++ b/gdb/testsuite/lib/tuiterm.exp >> @@ -820,6 +820,12 @@ namespace eval Term { >> } >> >> ::gdb_test_no_output "set pagination off" >> + >> + # We have TERM=ansi, so the default hide-last-column == auto will >> + # select on, which is a good value for an actual terminal emulator >> + # with am && xenl behaviour, but the TUI term doens't have xenl >> + # behaviour, so we don't have to work around it. >> + ::gdb_test_no_output "set tui hide-last-column off" >> } >> } >> >> @@ -1004,6 +1010,14 @@ namespace eval Term { >> gdb_assert {[regexp -- $regexp $contents]} $test_name >> } >> >> + # As check_contents, but checks whether the text contents of the terminal >> + # doesn't match the regular expression. >> + proc check_contents_not {test_name regexp} { >> + dump_screen >> + set contents [get_all_lines] >> + gdb_assert {![regexp -- $regexp $contents]} $test_name >> + } >> + >> # Get the region of the screen described by X, Y, WIDTH, >> # and HEIGHT, and separate the lines using SEP. >> proc get_region { x y width height sep } { >> diff --git a/gdb/tui/tui-layout.c b/gdb/tui/tui-layout.c >> index 50c568fb7d7..107c84431e4 100644 >> --- a/gdb/tui/tui-layout.c >> +++ b/gdb/tui/tui-layout.c >> @@ -77,8 +77,11 @@ tui_apply_current_layout (bool preserve_cmd_win_size_p) >> for (tui_win_info *win_info : tui_windows) >> win_info->make_visible (false); >> >> - applied_layout->apply (0, 0, tui_term_width (), tui_term_height (), >> - preserve_cmd_win_size_p); >> + applied_layout->apply (0, 0, >> + (tui_hide_last_column >> + ? tui_term_width () - 1 >> + : tui_term_width ()), >> + tui_term_height (), preserve_cmd_win_size_p); >> >> /* Keep the list of internal windows up-to-date. */ >> for (int win_type = SRC_WIN; (win_type < MAX_MAJOR_WINDOWS); win_type++) >> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c >> index 7eac03f47a1..68003e6bdd6 100644 >> --- a/gdb/tui/tui-win.c >> +++ b/gdb/tui/tui-win.c >> @@ -346,6 +346,74 @@ tui_set_var_cmd (const char *null_args, >> tui_rehighlight_all (); >> } >> >> +/* Variable corresponding to the "tui hide-last-column" setting. */ >> + >> +static enum auto_boolean tui_hide_last_column_setting = AUTO_BOOLEAN_AUTO; >> + >> +/* See tui-win.h. */ >> + >> +bool tui_hide_last_column; >> + >> +/* Show current value of the "tui hide-last-column" setting. */ >> + >> +static void >> +tui_show_hide_last_column (struct ui_file *file, int from_tty, >> + struct cmd_list_element *c, >> + const char *value) >> +{ >> + if (strcmp (value, "auto") == 0) >> + gdb_printf (file, >> + _("TUI last column hiding is \"%s; currently %s\".\n"), value, >> + tui_hide_last_column ? "on" : "off"); >> + else >> + gdb_printf (file, >> + _("TUI last column hiding is \"%s\".\n"), value); >> +} >> + >> +/* Update tui_hide_last_column according to the current value of the >> + "tui hide-last-column" setting. */ >> + >> +static void >> +update_tui_hide_last_column () >> +{ >> + switch (tui_hide_last_column_setting) >> + { >> + case AUTO_BOOLEAN_AUTO: >> + tui_hide_last_column = readline_hidden_cols ? true : false; >> + break; >> + case AUTO_BOOLEAN_FALSE: >> + tui_hide_last_column = false; >> + break; >> + case AUTO_BOOLEAN_TRUE: >> + tui_hide_last_column = true; >> + break; >> + default: >> + gdb_assert_not_reached (""); >> + } >> +} >> + >> +/* Set "tui hide-last-column". */ >> + >> +static void >> +tui_set_hide_last_column (const char *null_args, >> + int from_tty, struct cmd_list_element *c) >> +{ >> + bool prev_tui_hide_last_column = tui_hide_last_column; >> + update_tui_hide_last_column (); >> + >> + if (!tui_active) >> + return; >> + >> + if (!prev_tui_hide_last_column && tui_hide_last_column) >> + { >> + /* We're going to ignore the last column, clear it first. */ >> + clear (); >> + refresh (); >> + } >> + >> + tui_apply_current_layout (false); >> + tui_refresh_all_win (); >> +} >> >> >> /* True if TUI resizes should print a message. This is used by the >> @@ -454,7 +522,7 @@ tui_update_gdb_sizes (void) >> >> if (tui_active) >> { >> - width = TUI_CMD_WIN->width; >> + width = TUI_CMD_WIN->width + (tui_hide_last_column ? 1 : 0); >> height = TUI_CMD_WIN->height; >> } >> else >> @@ -587,6 +655,12 @@ tui_async_resize_screen (gdb_client_data arg) >> } >> else >> { >> + if (tui_hide_last_column) >> + { >> + /* Make sure the hidden last column is cleared. */ >> + clear (); >> + refresh (); >> + } >> tui_set_win_resized_to (false); >> tui_resize_all (); >> tui_refresh_all_win (); >> @@ -1304,6 +1378,18 @@ When enabled, the left margin will use '_' and '0' instead of spaces."), >> &maintenance_set_cmdlist, >> &maintenance_show_cmdlist); >> >> + add_setshow_auto_boolean_cmd ("hide-last-column", class_tui, >> + &tui_hide_last_column_setting, _("\ >> +Set whether TUI should hide the last column."), _("\ >> +Show whether TUI should hide the last column."), _("\ >> +When enabled, TUI will hide the last column."), >> + tui_set_hide_last_column, >> + tui_show_hide_last_column, >> + &tui_setlist, &tui_showlist); >> + /* If tui_hide_last_column_setting == auto by default, determine the actual >> + on/off value. */ >> + update_tui_hide_last_column (); >> + >> tui_border_style.changed.attach (tui_rehighlight_all, "tui-win"); >> tui_active_border_style.changed.attach (tui_rehighlight_all, "tui-win"); >> } >> diff --git a/gdb/tui/tui-win.h b/gdb/tui/tui-win.h >> index 3d35f1dfb7f..c10199eb37b 100644 >> --- a/gdb/tui/tui-win.h >> +++ b/gdb/tui/tui-win.h >> @@ -58,4 +58,7 @@ extern bool style_tui_current_position; >> /* Whether to replace the spaces in the left margin with '_' and '0'. */ >> extern bool tui_left_margin_verbose; >> >> +/* Whether the tui should hide the last screen column. */ >> +extern bool tui_hide_last_column; >> + >> #endif /* TUI_TUI_WIN_H */ >> >> base-commit: d9cc4b060dd23724e1acf974aed3d1b72c8459e5 >> -- >> 2.35.3 >