Re: [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt

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

On 6/22/23 21:14, Tom de Vries via Gdb-patches wrote:
> In test-case gdb.tui/long-prompt.exp, with a prompt of 40 chars, the same size
> as the terminal width, we get a superfluous newline at line 19:
> ...
> 16 (gdb) set prompt 123456789A123456789B123
> 17 456789C123456789>
> 18 123456789A123456789B123456789C123456789>
> 19
> 20 123456789A123456789B123456789C123456789>
> 21 set prompt (gdb)
> 22 (gdb)
> ...
> as well as a superfluous repetition of the prompt at line 20 once we type the
> 's' starting "set prompt".
> 
> I traced the superfluous newline back to readline's readline_internal_setup,
> that does:
> ...
>    /* If we're not echoing, we still want to at least print a prompt, because
>       rl_redisplay will not do it for us.  If the calling application has a
>       custom redisplay function, though, let that function handle it. */
>    if (_rl_echoing_p == 0 && rl_redisplay_function == rl_redisplay)
>      ...
>    else
>      {
>        if (rl_prompt && rl_already_prompted)
> 	rl_on_new_line_with_prompt ();
>        else
> 	rl_on_new_line ();
>        (*rl_redisplay_function) ();
> ...
> and then we hit the case that calls rl_on_new_line_with_prompt, which does:
> ...
>    /* If the prompt length is a multiple of real_screenwidth, we don't know
>       whether the cursor is at the end of the last line, or already at the
>       beginning of the next line. Output a newline just to be safe. */
>    if (l > 0 && (l % real_screenwidth) == 0)
>      _rl_output_some_chars ("\n", 1);
> ...
> 
> This doesn't look like a readline bug, because the behaviour matches the
> comment.
> 
> [ And the fact that the output of the newline doesn't happen in the scope of
> tui_redisplay_readline means it doesn't get the prompt wrap detection
> treatment, causing start_line to be incorrect, which causes the superfluous
> repetition of the prompt. ]
> 
> I looked at ways to work around this, and managed by switching off
> rl_already_prompted, which we set to 1 in tui_rl_startup_hook:
> ...
> /* Readline hook to redisplay ourself the gdb prompt.
>     In the SingleKey mode, the prompt is not printed so that
>     the command window is cleaner.  It will be displayed if
>     we temporarily leave the SingleKey mode.  */
> static int
> tui_rl_startup_hook (void)
> {
>    rl_already_prompted = 1;
>    if (tui_current_key_mode != TUI_COMMAND_MODE
>        && !gdb_in_secondary_prompt_p (current_ui))
>      tui_set_key_mode (TUI_SINGLE_KEY_MODE);
>    tui_redisplay_readline ();
>    return 0;
> }
> ...
> 
> Then I started looking at why rl_already_prompted is set to 1.
> 
> The use case for rl_already_prompted seems to be:
> - app (application, the readline user) outputs prompt,
> - app sets rl_already_prompted to 1, and
> - app calls readline, which calls rl_on_new_line_with_prompt, which figures
>    out how long the prompt is, and sets a few readline variables accordingly,
>    which can be used in the following call to rl_redisplay_function.
> 
> AFAICT, TUI does not fit this pattern.  It does not output an initial prompt,
> rather it writes the prompt in every rl_redisplay_function.  It doesn't use
> the variables set by rl_on_new_line_with_prompt, instead it figures stuff out
> by itself.
> 
> Fix this by removing the rl_already_prompted setting.
> 
> Also remove the call to tui_redisplay_readline, it's not necessary, the
> function is called anyway.
> 

I'll commit this end of next week, unless there are comments.

Thanks,
- Tom

> Tested on x86_64-linux, no regressions.
> ---
>   gdb/testsuite/gdb.tui/long-prompt.exp | 18 +-----------------
>   gdb/tui/tui.c                         |  2 --
>   2 files changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.tui/long-prompt.exp b/gdb/testsuite/gdb.tui/long-prompt.exp
> index e4e8b75ede0..a08c08a884a 100644
> --- a/gdb/testsuite/gdb.tui/long-prompt.exp
> +++ b/gdb/testsuite/gdb.tui/long-prompt.exp
> @@ -101,29 +101,13 @@ with_test_prefix "prompt size == width" {
>       #   16 (gdb) set prompt 123456789A123456789B123
>       #   17 456789C123456789>
>       #   18 123456789A123456789B123456789C123456789>
> -    #   19
> -    #   20 123456789A123456789B123456789C123456789>
> -    #   21 set prompt (gdb)
> -    #   22 (gdb)
> -    #
> -    # Note that it would be nice to get instead:
> -    #
> -    #   16 (gdb) set prompt 123456789A123456789B123
> -    #   17 456789C123456789>
> -    #   18 123456789A123456789B123456789C123456789>
>       #   19 set prompt (gdb)
>       #   20 (gdb)
> -    #
> -    # The extra newline is added by rl_on_new_line_with_prompt, which decides
> -    # this as follows:
> -    #  /* If the prompt length is a multiple of real_screenwidth, we don't know
> -    #     whether the cursor is at the end of the last line, or already at the
> -    #     beginning of the next line. Output a newline just to be safe. */
>   
>       gdb_assert { [Term::wait_for "^set prompt $gdb_prompt "] } \
>   	"got prompt back"
>   
> -    gdb_assert { $Term::_cur_row == 22 }
> +    gdb_assert { $Term::_cur_row == 20 }
>   }
>   
>   with_test_prefix "prompt size == width - 1" {
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index 43be8161e4c..941c65c970f 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -271,11 +271,9 @@ tui_rl_next_keymap (int notused1, int notused2)
>   static int
>   tui_rl_startup_hook (void)
>   {
> -  rl_already_prompted = 1;
>     if (tui_current_key_mode != TUI_COMMAND_MODE
>         && !gdb_in_secondary_prompt_p (current_ui))
>       tui_set_key_mode (TUI_SINGLE_KEY_MODE);
> -  tui_redisplay_readline ();
>     return 0;
>   }
>   
> 
> base-commit: d4d5b571954a2bde2a14772d9b18027a9048eb2d