Re: mouse-face and help echo support for xterm mouse

[Jared Finder]

On 2020-11-04 7:56 am, Stefan Monnier wrote:
> diff --git a/src/term.c b/src/term.c
> index ff1aabfed2..a03a246415 100644
> --- a/src/term.c
> +++ b/src/term.c
> @@ -2562,7 +2562,8 @@
>    ie.kind = NO_EVENT;
>    ie.arg = Qnil;
>
> -  if (event->type & (GPM_MOVE | GPM_DRAG)) {
> +  if (event->type & (GPM_MOVE | GPM_DRAG))
> +    {

Old code style is OLD.  Apparently this formatting is from when the 
function was first added in 2007. :)

> @@ -2577,29 +2578,45 @@
>      || !NILP (previous_help_echo_string))
>        do_help = 1;
>
> -    goto done;
> +      eassert (ie.kind == NO_EVENT);

This is the assert for the mouse movement path.  Please remember this 
value.

>    }
> -  else {
> +  else
> +    {
>      f->mouse_moved = 0;
> +      eassert (ie.kind == NO_EVENT);
>      term_mouse_click (&ie, event, f);
> +      eassert (ie.kind == GPM_CLICK_EVENT);
>      if (tty_handle_tab_bar_click (f, event->x, event->y,
>                                   (ie.modifiers & down_modifier) != 0, 
> &ie))
>        {
> +          eassert (ie.kind == GPM_CLICK_EVENT
> +                   || ie.kind == TAB_BAR_EVENT);
>         /* tty_handle_tab_bar_click stores 2 events in the event
>            queue, so we are done here.  */
> +          /* FIXME: Actually, `tty_handle_tab_bar_click` returns true
> +             without storing any events, when
> +             (ie.modifiers & down_modifier) != 0  */
>         count += 2;
>         return count;
>        }
> +      eassert (ie.kind == GPM_CLICK_EVENT);

This assert is for the mouse click path, please remember this value.

>    }
>
> - done:
> -  if (ie.kind != NO_EVENT)
> +  if (ie.kind != NO_EVENT) /* FIXME: We retest the previous if's 
> condition!  */
>      {
> +      /* FIXME: `hold_quit` could already hold a previous event,
> +         so it risks crushing that previous event.
> +         What am I missing?  */
>        kbd_buffer_store_event_hold (&ie, hold_quit);

This is the meat of the refactor.

Given the current code hold_quit will never hold a previous event. 
tty_read_avail_input initializes hold_quit right before calling 
handle_one_term_event and and sets kind to NO_EVENT.  This is the first 
and only time in this function hold_quit is passed to any other 
function.

Because it is the first time passed, we can safely deduce that 
hold_quit->kind will still be NO_EVENT *unless* a previous loop 
iteration changed it.

Because it is the only time passed, we can safely deduce that 
hold_quit->kind can only changed in a previous loop iteration by this 
function storing a quit event.  However, this can not happen.  hold_quit 
would get set by kbd_buffer_store_buffered_event which 
kbd_buffer_store_event_hold calls.  To get set, the following must all 
be true:

1. An event of kind ASCII_KEYSTROKE_EVENT must be processed.
2. And that event's character code must be quit_char.
3. And that event must be on the current frame's KBOARD (not quite sure 
what this means).

However, 1 will never be true as the only events that ever reach this 
point are of kind GPM_CLICK_EVENT, as deduced from the above asserts.

>        count++;
>      }
>
>    if (do_help
> +      /* FIXME: `hold_quit` is never NULL?!  */

Agreed.

> +      /* FIXME: Why do we test `hold_quit->kind != NO_EVENT` here?
> +         This either comes from `ie` (via kbd_buffer_store_event_hold 
> above)
> +         or from an earlier call to us (tty_read_avail_input calls us 
> in a
> +         loop with the same `hold_quit` struct).  */

And from above reasoning, we know that hold_quit is always non-NULL and 
hold_quit->kind is always NO_EVENT.

  -- MJF