Re: [PATCH v2] Make the TUI command window support the mouse

[Hannes Domani via Gdb-patches <gdb-patches@sourceware.org>]

 Am Sonntag, 13. Juni 2021, 04:46:15 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> On 2021-06-12 7:08 p.m., Pedro Alves wrote:
> > On 2021-06-12 1:32 p.m., Hannes Domani wrote:
>
> >> On the other hand, if keypad was enabled, couldn't we just forward readline
> >> the escape sequences for the arrow keys instead?
> >
> > Yeah, to be honest I think that is likely a better approach and worth it of a
> > try -- my only concern is whether the escape sequences are standard enough
> > across terminals?  Maybe it's all covered by ANSI and it's all we need to care
> > about?  I thought of the other approach because that let's us not care about
> > specific sequences, other than the mouse sequence, which seemed pretty much
> > standard from looking around.  Also, it was run to write.  :-)
> >
>
> Alright, I gave that approach a go.  Below's a patch implementing that.  It
> works quite nicely here.  I tried it on konsole, xterm and rxvt.  Among those,
> rxvt uses different escape sequences for ctrl-up/ctrl-down/ctrl-left/ctrl-right,
> but it doesn't really matter -- I found that readline binds actions to
> different variants of escape sequences, so if we pick sequences readline always binds,
> it should always work.  See readline.c:bind_arrow_keys_internal.
> Despite that, for the standard ncurses keys below KEY_MAX, it's easier
> to use the corresponding lowercase key_foo variable, which I believe is
> filled in from termcap so should also always work, as readline also
> binds the key sequences termcap returns.
>
> Overall, I'm pleased with this approach.  See commit log for more, particularly
> the extra improvement we get.
>
> From: Pedro Alves <pedro@palves.net>
> Date: 2021-06-05 19:11:09 +0100
>
> Make the TUI command window support the mouse
>
> Currently, when the focus is on the command window, we disable the
> keypad.  This means that when the command window has the focus, keys
> such as up/down/home/end etc. are not processed by ncurses, and their
> escape sequences go straight to readline.
>
> A side effect of disabling keypad mode is that wgetch no longer
> processes mouse escape sequences, with the end result being the mouse
> doesn't work, and worse, the raw mouse escape sequences are printed on
> the terminal.
>
> This commit makes the TUI command window support the mouse as well, by
> always enabling the keypad, and then to avoid losing support for
> up/down browsing the command history, home/end/left/right moving the
> cursor position, etc., we forward those keys as raw escape sequences
> to readline.
>
> Note that the patch makes it so that CTLC-L is already passed to
> readline even if the command window does not have the focus.  It was
> simpler to implement that way, and it just seems correct to me.  I
> don't know of a reason we shouldn't do that.
>
> The patch improves the TUI behavior in a related way.  Now we can pass
> keys to readline irrespective of which window has focus.  First, we
> try to dispatch the key to a window, via tui_displatch_ctrl_char.  If
> the key is dispatched, then we don't pass it to readline.  E.g.,
> pressing "up" when you have the source window in focus results in
> scrolling the source window, and nothing else.  If however, you press
> ctrl-del instead, that results in killing the next word in the command
> window, no matter which window has focus.  Before, it would only work
> if you had the command window in focus.  Similarly,
> ctrl-left/ctrl-right to move between words, etc.
>
> Similarly, the previous spot where we handled mouse events was
> incorrect.  It was never reached if the window with focus can't
> scroll, which is the case for the command window.  Mouse scrolling
> affects the window under the mouse cursor, not the window in focus.
> We now always try to dispatch mouse events.

This is great, thank you for doing this.

I just tried it, and I had just one problem, see below.


> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <pedro@palves.net>
>
>     * tui/tui-io.c (tui_dispatch_mouse_event): New, factored out from
>     ...
>     (tui_dispatch_ctrl_char): ... this.  Move CTRL-L handling to
>     tui_getc_1.
>     (cur_seq, start_sequence): New.
>     (tui_getc_1): Pass key escape sequences for ncurses control keys
>     to readline.  Handle mouse and ctrl-l here.
>     (tui_resize_all): Disable/reenable the keypad if the command
>     window has the focus too.
>     * tui/tui-win.c (tui_set_focus_command): Don't change keypad
>     setting.
>     * tui/tui.c (tui_rl_other_window): Don't change keypad setting.
>
> Change-Id: Ie0a7d849943cfb47f4a6589e1c73341563740fa9
>
> ---
>
> gdb/tui/tui-io.c  |  195 ++++++++++++++++++++++++++++++++++++++++++-----------
> gdb/tui/tui-win.c |  12 +--
> gdb/tui/tui.c    |    5 -
> 3 files changed, 160 insertions(+), 52 deletions(-)
>
> diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
> index 7df0e2f1bd3..4da6f2bab11 100644
> --- a/gdb/tui/tui-io.c
> +++ b/gdb/tui/tui-io.c
> @@ -946,6 +946,38 @@ tui_initialize_io (void)
> #endif
> }
>
> +/* Dispatch the correct tui function based upon the mouse event.  */
> +
> +static void
> +tui_dispatch_mouse_event ()
> +{
> +  MEVENT mev;
> +  if (getmouse (&mev) != OK)
> +    return;
> +
> +  for (tui_win_info *wi : all_tui_windows ())
> +    if (mev.x > wi->x && mev.x < wi->x + wi->width - 1
> +    && mev.y > wi->y && mev.y < wi->y + wi->height - 1)
> +      {
> +    if ((mev.bstate & BUTTON1_CLICKED) != 0
> +        || (mev.bstate & BUTTON2_CLICKED) != 0
> +        || (mev.bstate & BUTTON3_CLICKED) != 0)
> +      {
> +        int button = (mev.bstate & BUTTON1_CLICKED) != 0 ? 1
> +          :        ((mev.bstate & BUTTON2_CLICKED) != 0 ? 2
> +            : 3);
> +        wi->click (mev.x - wi->x - 1, mev.y - wi->y - 1, button);
> +      }
> +#ifdef BUTTON5_PRESSED
> +    else if ((mev.bstate & BUTTON4_PRESSED) != 0)
> +      wi->backward_scroll (3);
> +    else if ((mev.bstate & BUTTON5_PRESSED) != 0)
> +      wi->forward_scroll (3);
> +#endif
> +    break;
> +      }
> +}
> +
> /* Dispatch the correct tui function based upon the control
>     character.  */
> static unsigned int
> @@ -953,10 +985,6 @@ tui_dispatch_ctrl_char (unsigned int ch)
> {
>   struct tui_win_info *win_info = tui_win_with_focus ();
>
> -  /* Handle the CTRL-L refresh for each window.  */
> -  if (ch == '\f')
> -    tui_refresh_all_win ();
> -
>   /* If no window has the focus, or if the focus window can't scroll,
>       just pass the character through.  */
>   if (win_info == NULL || !win_info->can_scroll ())
> @@ -984,39 +1012,6 @@ tui_dispatch_ctrl_char (unsigned int ch)
>     case KEY_LEFT:
>       win_info->right_scroll (1);
>       break;
> -#ifdef NCURSES_MOUSE_VERSION
> -    case KEY_MOUSE:
> -    {
> -      MEVENT mev;
> -      if (getmouse (&mev) != OK)
> -        break;
> -
> -      for (tui_win_info *wi : all_tui_windows ())
> -        if (mev.x > wi->x && mev.x < wi->x + wi->width - 1
> -        && mev.y > wi->y && mev.y < wi->y + wi->height - 1)
> -          {
> -        if ((mev.bstate & BUTTON1_CLICKED) != 0
> -            || (mev.bstate & BUTTON2_CLICKED) != 0
> -            || (mev.bstate & BUTTON3_CLICKED) != 0)
> -          {
> -            int button = (mev.bstate & BUTTON1_CLICKED) != 0 ? 1
> -              :        ((mev.bstate & BUTTON2_CLICKED) != 0 ? 2
>
> -                : 3);
>
> -            wi->click (mev.x - wi->x - 1, mev.y - wi->y - 1, button);
> -          }
> -#ifdef BUTTON5_PRESSED
> -        else if ((mev.bstate & BUTTON4_PRESSED) != 0)
> -          wi->backward_scroll (3);
> -        else if ((mev.bstate & BUTTON5_PRESSED) != 0)
> -          wi->forward_scroll (3);
> -#endif
> -        break;
> -          }
> -    }
> -      break;
> -#endif
> -    case '\f':
> -      break;
>     default:
>       /* We didn't recognize the character as a control character, so pass it
>     through.  */
> @@ -1067,6 +1062,24 @@ tui_inject_newline_into_command_window ()
>     }
> }
>
> +/* If we're passing an escape sequence to readline, this points to a
> +  string holding the remaining characters of the sequence to pass.
> +  We advance the pointer one character at a time until '\0' is
> +  reached.  */
> +static const char *cur_seq = nullptr;
> +
> +/* Set CUR_SEQ to point at the current sequence to pass to readline,
> +  setup to call the input handler again so we complete the sequence
> +  shortly, and return the first character to start the sequence.  */
> +
> +static int
> +start_sequence (const char *seq)
> +{
> +  call_stdin_event_handler_again_p = 1;
> +  cur_seq = seq + 1;
> +  return seq[0];
> +}
> +
> /* Main worker for tui_getc.  Get a character from the command window.
>     This is called from the readline package, but wrapped in a
>     try/catch by tui_getc.  */
> @@ -1084,11 +1097,115 @@ tui_getc_1 (FILE *fp)
>   tui_readline_output (0, 0);
> #endif
>
> -  ch = gdb_wgetch (w);
> +  /* We enable keypad mode so that ncurses's wgetch processes mouse
> +    escape sequences.  In keypad mode, wgetch also processes the
> +    escape sequences for keys such as up/down etc. and returns KEY_UP
> +    / KEY_DOWN etc.  When we have the focus on the command window
> +    though, we want to pass the raw up/down etc. escape codes to
> +    readline so readline understands them.  */
> +  if (cur_seq != nullptr)
> +    {
> +      ch = *cur_seq++;
> +
> +      /* If we've reached the end of the string, we're done with the
> +    sequence.  Otherwise, setup to get back here again for
> +    another character.  */
> +      if (*cur_seq == '\0')
> +    cur_seq = nullptr;
> +      else
> +    call_stdin_event_handler_again_p = 1;
> +      return ch;
> +    }
> +  else
> +    ch = gdb_wgetch (w);
>
>   /* Handle prev/next/up/down here.  */
>   ch = tui_dispatch_ctrl_char (ch);
> -
> +
> +#ifdef NCURSES_MOUSE_VERSION
> +  if (ch == KEY_MOUSE)
> +    {
> +      tui_dispatch_mouse_event ();
> +      return 0;
> +    }
> +#endif
> +
> +  /* Translate ncurses keys back to escape sequences so that readline
> +    can understand them.  We do this irrespective of what is the
> +    focus window.  If e.g., we're focused on a non-command window,
> +    then the up/down keys will already have been filtered by
> +    tui_dispatch_ctrl_char.  Keys that haven't been intercepted will
> +    be passed down to readline.  */
> +  if (current_ui->command_editing)
> +    {
> +      /* For the standard keys, we can find them efficiently in the
> +    key_xxx macros, defined by ncurses.  We could also hardcode
> +    sequences readline understands, and/or use rl_get_termcap.
> +    See readline/readline.c:bind_arrow_keys_internal for
> +    hardcoded sequences.  */
> +      switch (ch)
> +    {
> +    case KEY_NPAGE: /* page down */
> +      return start_sequence (key_npage);
> +    case KEY_PPAGE: /* page up */
> +      return start_sequence (key_ppage);
> +    case KEY_DOWN:
> +      return start_sequence (key_down);
> +    case KEY_UP:
> +      return start_sequence (key_up);
> +    case KEY_RIGHT:
> +      return start_sequence (key_right);
> +    case KEY_LEFT:
> +      return start_sequence (key_left);
> +    case KEY_HOME:
> +      return start_sequence (key_home);
> +    case KEY_END:
> +      return start_sequence (key_end);
> +    case KEY_DC: /* del */
> +      return start_sequence (key_dc);
> +    case KEY_IC: /* ins */
> +      return start_sequence (key_ic);
> +    }

PDcurses doesn't have these key_npage/key_ppage/key_down/... variables,
so I had to use the hardcoded sequences as you described.
This is working for the arrow keys and home/end, and for del I used '\004'
instead, but I didn't see any for page up/down and ins.

I'm not sure if it's even a problem for page up/down, they seem to not be used
by readline, but I couldn't make ins work.

And rl_get_termcap returned for everything except "le" NULL, but I assume
that's just how it is on Windows.


> +
> +      /* Keycodes above KEY_MAX are not garanteed to be stable.
> +    Compare keyname instead.  */
> +      if (ch >= KEY_MAX)
> +    {
> +      auto name = gdb::string_view (keyname (ch));
> +
> +      /* ctrl-arrow keys */
> +      if (name == "kLFT5") /* ctrl-left */
> +        return start_sequence ("\033[1;5D");
> +      else if (name == "kRIT5") /* ctrl-right */
> +        return start_sequence ("\033[1;5C");
> +      else if (name == "kUP5") /* ctrl-up */
> +        return start_sequence ("\033[1;5A");
> +      else if (name == "kDN5") /* ctrl-down */
> +        return start_sequence ("\033[1;5B");
> +      else if (name == "kHOM5") /* ctrl-home */
> +        return start_sequence ("\033[1;5H");
> +      else if (name == "kEND5") /* ctrl-end */
> +        return start_sequence ("\033[1;5F");
> +      else if (name == "kIC5") /* ctrl-ins */
> +        return start_sequence ("\033[2;5~");
> +      else if (name == "kDC5") /* ctrl-del */
> +        return start_sequence ("\033[3;5~");
> +
> +      /* alt-arrow keys */
> +      else if (name == "kLFT3") /* alt-left */
> +        return start_sequence ("\033[1;3D");
> +      else if (name == "kRIT3") /* alt-right */
> +        return start_sequence ("\033[1;3C");
> +    }
> +    }
> +
> +  /* Handle the CTRL-L refresh for each window.  */
> +  if (ch == '\f')
> +    {
> +      tui_refresh_all_win ();
> +      return ch;
> +    }
> +
>   if (ch == KEY_BACKSPACE)
>     return '\b';
>
> diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
> index 4e75a66a00e..a51e7b9f6da 100644
> --- a/gdb/tui/tui-win.c
> +++ b/gdb/tui/tui-win.c
> @@ -498,14 +498,11 @@ tui_resize_all (void)
>   height_diff = screenheight - tui_term_height ();
>   if (height_diff || width_diff)
>     {
> -      struct tui_win_info *win_with_focus = tui_win_with_focus ();
> -
> #ifdef HAVE_RESIZE_TERM
>       resize_term (screenheight, screenwidth);
> #endif     
>       /* Turn keypad off while we resize.  */
> -      if (win_with_focus != TUI_CMD_WIN)
> -    keypad (TUI_CMD_WIN->handle.get (), FALSE);
> +      keypad (TUI_CMD_WIN->handle.get (), FALSE);
>       tui_update_gdb_sizes ();
>       tui_set_term_height_to (screenheight);
>       tui_set_term_width_to (screenwidth);
> @@ -515,10 +512,8 @@ tui_resize_all (void)
>       erase ();
>       clearok (curscr, TRUE);
>       tui_apply_current_layout ();
> -      /* Turn keypad back on, unless focus is in the command
> -    window.  */
> -      if (win_with_focus != TUI_CMD_WIN)
> -    keypad (TUI_CMD_WIN->handle.get (), TRUE);
> +      /* Turn keypad back on.  */
> +      keypad (TUI_CMD_WIN->handle.get (), TRUE);
>     }
> }
>
> @@ -703,7 +698,6 @@ tui_set_focus_command (const char *arg, int from_tty)
>     error (_("Window \"%s\" is not visible"), arg);
>
>   tui_set_win_focus_to (win_info);
> -  keypad (TUI_CMD_WIN->handle.get (), win_info != TUI_CMD_WIN);
>   printf_filtered (_("Focus set to %s window.\n"),
>           tui_win_with_focus ()->name ());
> }
> diff --git a/gdb/tui/tui.c b/gdb/tui/tui.c
> index 529fc62c9ac..5f0c87c05e1 100644
> --- a/gdb/tui/tui.c
> +++ b/gdb/tui/tui.c
> @@ -179,10 +179,7 @@ tui_rl_other_window (int count, int key)
>
>   win_info = tui_next_win (tui_win_with_focus ());
>   if (win_info)
> -    {
> -      tui_set_win_focus_to (win_info);
> -      keypad (TUI_CMD_WIN->handle.get (), win_info != TUI_CMD_WIN);
> -    }
> +    tui_set_win_focus_to (win_info);
>   return 0;
>
> }


Hannes