Re: [PATCH] [gdb/tui] Add set tui hide-last-column on/off/auto

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

On 5/10/23 15:48, Andrew Burgess wrote:
> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> 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=<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 ();
>> +}
>>   \f
>>   
>>   /* 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
>