Consolidate the custom TUI query hook with the default query hook

  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(-)
  

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.
  

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;
>
  

On Fri, Jan 9, 2015 at 10:27 AM, Pedro Alves <palves@redhat.com> wrote:
> 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).

And tab completion now "works" in queries too :)

Attached is what I committed.

>
> Thanks,
> Pedro Alves