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

[Andrew Burgess <aburgess@redhat.com>]

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.

But I did have one incredibly minor nit...

> ---
>  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