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

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

On 7/14/23 14:18, Tom de Vries wrote:
> 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.
> 

Pushed.

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
>