This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Consolidate the custom TUI query hook with the default query hook
- From: Pedro Alves <palves at redhat dot com>
- To: Patrick Palka <patrick at parcs dot ath dot cx>, gdb-patches at sourceware dot org
- Date: Fri, 09 Jan 2015 15:27:06 +0000
- Subject: Re: [PATCH] Consolidate the custom TUI query hook with the default query hook
- Authentication-results: sourceware.org; auth=none
- References: <1420808488-30044-1-git-send-email-patrick at parcs dot ath dot cx>
On 01/09/2015 01:01 PM, Patrick Palka wrote:
> Hi Pedro,
>
> I noticed some latent "rendering" issues when invoking GDB's quit prompt
> within TUI, e.g. the quit prompt would repeat itself every time you
> pressed a key. Here's an updated version of the patch that fixes the
> two redisplay issues I found. TUI is a mess...
Thanks a lot for cleaning up the mess. :-)
It's great that a lot of the little annoying issues that
plague the TUI are getting fixed.
This is OK.
Another nice side effect is that now all the readline keys,
like ctrl-w works in queries (even in the CLI).
Thanks,
Pedro Alves
>
> -- >8 --
>
> This patch primarily rewrites defaulted_query() to use
> gdb_readline_wrapper() to prompt the user for input, like
> prompt_for_continue() does. The motivation for this rewrite is to be
> able to reuse the default query hook in TUI, obviating the need for a
> custom TUI query hook.
>
> However, having TUI use the default query mechanism exposed a couple of
> latent bugs in tui_redisplay_readline() related to the handling of
> multi-line prompts, in particular GDB's multi-line quit prompt.
>
> The first issue is an off-by-one error in the height calculation of the
> prompt. The check in question should be col <= prev_col, not
> c < prev_col, to properly account for the case when a prompt contains
> multiple consecutive newlines. Failing to do so makes TUI have the
> wrong idea of the vertical height of the prompt. This patch fixes the
> column check.
>
> The second issue is that cur_line does not get updated to reflect the
> cursor position if the user's prompt cursor is at the end of the prompt
> (i.e. if rl_point == rl_end). cur_line only gets updated if rl_point
> lies between 0..rl_end-1 because that is the bounds of the for loop
> responsible for updating cur_line. This patch changes the loop's bounds
> to 0..rl_end so that cur_line always gets updated.
>
> With these two bug fixes out of the way, the default query mechanism
> works well in TUI even with multi-line prompts like GDB's quit prompt.
>
> gdb/ChangeLog:
>
> * utils.c (defaulted_query): Rewrite to use gdb_readline_wrapper
> to prompt for input.
> * tui/tui-hooks.c (tui_query_hook): Remove.
> (tui_install_hooks): Don't set deprecated_query_hook.
> * tui/tui-io.c (tui_redisplay_readline): Fix off-by-one error in
> height calculation. Always update the command window's cur_line.
> ---
> gdb/tui/tui-hooks.c | 64 -----------------------------------------------------
> gdb/tui/tui-io.c | 19 ++++++++--------
> gdb/utils.c | 61 ++++++++++++--------------------------------------
> 3 files changed, 24 insertions(+), 120 deletions(-)
>
> diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
> index 6ba6285..e53f526 100644
> --- a/gdb/tui/tui-hooks.c
> +++ b/gdb/tui/tui-hooks.c
> @@ -63,68 +63,6 @@ tui_new_objfile_hook (struct objfile* objfile)
> tui_display_main ();
> }
>
> -static int ATTRIBUTE_PRINTF (1, 0)
> -tui_query_hook (const char *msg, va_list argp)
> -{
> - int retval;
> - int ans2;
> - int answer;
> - char *question;
> - struct cleanup *old_chain;
> -
> - /* Format the question outside of the loop, to avoid reusing
> - ARGP. */
> - question = xstrvprintf (msg, argp);
> - old_chain = make_cleanup (xfree, question);
> -
> - echo ();
> - while (1)
> - {
> - wrap_here (""); /* Flush any buffered output. */
> - gdb_flush (gdb_stdout);
> -
> - fputs_filtered (question, gdb_stdout);
> - printf_filtered (_("(y or n) "));
> -
> - wrap_here ("");
> - gdb_flush (gdb_stdout);
> -
> - answer = tui_getc (stdin);
> - clearerr (stdin); /* in case of C-d */
> - if (answer == EOF) /* C-d */
> - {
> - retval = 1;
> - break;
> - }
> - /* Eat rest of input line, to EOF or newline. */
> - if (answer != '\n')
> - do
> - {
> - ans2 = tui_getc (stdin);
> - clearerr (stdin);
> - }
> - while (ans2 != EOF && ans2 != '\n' && ans2 != '\r');
> -
> - if (answer >= 'a')
> - answer -= 040;
> - if (answer == 'Y')
> - {
> - retval = 1;
> - break;
> - }
> - if (answer == 'N')
> - {
> - retval = 0;
> - break;
> - }
> - printf_filtered (_("Please answer y or n.\n"));
> - }
> - noecho ();
> -
> - do_cleanups (old_chain);
> - return retval;
> -}
> -
> /* Prevent recursion of deprecated_register_changed_hook(). */
> static int tui_refreshing_registers = 0;
>
> @@ -263,8 +201,6 @@ tui_install_hooks (void)
> deprecated_print_frame_info_listing_hook
> = tui_print_frame_info_listing_hook;
>
> - deprecated_query_hook = tui_query_hook;
> -
> /* Install the event hooks. */
> tui_bp_created_observer
> = observer_attach_breakpoint_created (tui_event_create_breakpoint);
> diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
> index 233e7a6..7b1ec43 100644
> --- a/gdb/tui/tui-io.c
> +++ b/gdb/tui/tui-io.c
> @@ -234,20 +234,23 @@ tui_redisplay_readline (void)
> {
> waddch (w, prompt[in]);
> getyx (w, line, col);
> - if (col < prev_col)
> + if (col <= prev_col)
> height++;
> prev_col = col;
> }
> - for (in = 0; in < rl_end; in++)
> + for (in = 0; in <= rl_end; in++)
> {
> unsigned char c;
>
> - c = (unsigned char) rl_line_buffer[in];
> if (in == rl_point)
> {
> getyx (w, c_line, c_pos);
> }
>
> + if (in == rl_end)
> + break;
> +
> + c = (unsigned char) rl_line_buffer[in];
> if (CTRL_CHAR (c) || c == RUBOUT)
> {
> waddch (w, '^');
> @@ -270,12 +273,10 @@ tui_redisplay_readline (void)
> wclrtobot (w);
> getyx (w, TUI_CMD_WIN->detail.command_info.start_line,
> TUI_CMD_WIN->detail.command_info.curch);
> - if (c_line >= 0)
> - {
> - wmove (w, c_line, c_pos);
> - TUI_CMD_WIN->detail.command_info.cur_line = c_line;
> - TUI_CMD_WIN->detail.command_info.curch = c_pos;
> - }
> +
> + wmove (w, c_line, c_pos);
> + TUI_CMD_WIN->detail.command_info.cur_line = c_line;
> + TUI_CMD_WIN->detail.command_info.curch = c_pos;
> TUI_CMD_WIN->detail.command_info.start_line -= height - 1;
>
> wrefresh (w);
> diff --git a/gdb/utils.c b/gdb/utils.c
> index 72b1e2a..a9a3082 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1198,12 +1198,11 @@ compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
> static int ATTRIBUTE_PRINTF (1, 0)
> defaulted_query (const char *ctlstr, const char defchar, va_list args)
> {
> - int answer;
> int ans2;
> int retval;
> int def_value;
> char def_answer, not_def_answer;
> - char *y_string, *n_string, *question;
> + char *y_string, *n_string, *question, *prompt;
> /* Used to add duration we waited for user to respond to
> prompt_for_continue_wait_time. */
> struct timeval prompt_started, prompt_ended, prompt_delta;
> @@ -1263,62 +1262,31 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
>
> /* Format the question outside of the loop, to avoid reusing args. */
> question = xstrvprintf (ctlstr, args);
> + prompt = xstrprintf (_("%s%s(%s or %s) %s"),
> + annotation_level > 1 ? "\n\032\032pre-query\n" : "",
> + question, y_string, n_string,
> + annotation_level > 1 ? "\n\032\032query\n" : "");
> + xfree (question);
>
> /* Used for calculating time spend waiting for user. */
> gettimeofday (&prompt_started, NULL);
>
> while (1)
> {
> - wrap_here (""); /* Flush any buffered output. */
> - gdb_flush (gdb_stdout);
> -
> - if (annotation_level > 1)
> - printf_filtered (("\n\032\032pre-query\n"));
> -
> - fputs_filtered (question, gdb_stdout);
> - printf_filtered (_("(%s or %s) "), y_string, n_string);
> -
> - if (annotation_level > 1)
> - printf_filtered (("\n\032\032query\n"));
> + char *response, answer;
>
> - wrap_here ("");
> gdb_flush (gdb_stdout);
> + response = gdb_readline_wrapper (prompt);
>
> - answer = fgetc (stdin);
> -
> - /* We expect fgetc to block until a character is read. But
> - this may not be the case if the terminal was opened with
> - the NONBLOCK flag. In that case, if there is nothing to
> - read on stdin, fgetc returns EOF, but also sets the error
> - condition flag on stdin and errno to EAGAIN. With a true
> - EOF, stdin's error condition flag is not set.
> -
> - A situation where this behavior was observed is a pseudo
> - terminal on AIX. */
> - while (answer == EOF && ferror (stdin) && errno == EAGAIN)
> - {
> - /* Not a real EOF. Wait a little while and try again until
> - we read something. */
> - clearerr (stdin);
> - gdb_usleep (10000);
> - answer = fgetc (stdin);
> - }
> -
> - clearerr (stdin); /* in case of C-d */
> - if (answer == EOF) /* C-d */
> + if (response == NULL) /* C-d */
> {
> printf_filtered ("EOF [assumed %c]\n", def_answer);
> retval = def_value;
> break;
> }
> - /* Eat rest of input line, to EOF or newline. */
> - if (answer != '\n')
> - do
> - {
> - ans2 = fgetc (stdin);
> - clearerr (stdin);
> - }
> - while (ans2 != EOF && ans2 != '\n' && ans2 != '\r');
> +
> + answer = response[0];
> + xfree (response);
>
> if (answer >= 'a')
> answer -= 040;
> @@ -1333,8 +1301,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
> specify the required input or have it default by entering
> nothing. */
> if (answer == def_answer
> - || (defchar != '\0' &&
> - (answer == '\n' || answer == '\r' || answer == EOF)))
> + || (defchar != '\0' && answer == '\0'))
> {
> retval = def_value;
> break;
> @@ -1350,7 +1317,7 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
> timeval_add (&prompt_for_continue_wait_time,
> &prompt_for_continue_wait_time, &prompt_delta);
>
> - xfree (question);
> + xfree (prompt);
> if (annotation_level > 1)
> printf_filtered (("\n\032\032post-query\n"));
> return retval;
>