* [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt
@ 2023-06-22 19:14 Tom de Vries
2023-07-14 12:18 ` Tom de Vries
0 siblings, 1 reply; 3+ messages in thread
From: Tom de Vries @ 2023-06-22 19:14 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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.
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
--
2.35.3
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt
2023-06-22 19:14 [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt Tom de Vries
@ 2023-07-14 12:18 ` Tom de Vries
2023-07-21 7:37 ` Tom de Vries
0 siblings, 1 reply; 3+ messages in thread
From: Tom de Vries @ 2023-07-14 12:18 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt
2023-07-14 12:18 ` Tom de Vries
@ 2023-07-21 7:37 ` Tom de Vries
0 siblings, 0 replies; 3+ messages in thread
From: Tom de Vries @ 2023-07-21 7:37 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-21 7:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-22 19:14 [PATCH v2] [gdb/tui] Fix superfluous newline for long prompt Tom de Vries
2023-07-14 12:18 ` Tom de Vries
2023-07-21 7:37 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).