Re: [PATCH] Consolidate the custom TUI query hook with the default query hook

[Patrick Palka <patrick@parcs.ath.cx>]

On Fri, Jan 9, 2015 at 8:01 AM, Patrick Palka <patrick@parcs.ath.cx> 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...
>
> -- >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;
>

Oops, this last hunk in tui-io.c is redundant.  The c_line >= 0
predicate can stay (and probably should, to be safe).  I intended to
leave this hunk out.