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

[Stefan Monnier]

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

The way I read it:

      EVENT_INIT (gpm_hold_quit);
      gpm_hold_quit.kind = NO_EVENT;
      [...]
      while (gpm = Gpm_GetEvent (&event), gpm == 1) {
          nread += handle_one_term_event (tty, &event, &gpm_hold_quit);
      }

`gpm_hold_quit->kind` could hold a different value from `NO_EVENT` on the
second iteration (and subsequent ones) of the loop.

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

That's right.

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

Oh, I see, indeed.

So we have the following:

    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        eassert (ie.kind == NO_EVENT);
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
      }
    
    if (ie.kind != NO_EVENT) /* FIXME: We retest the previous if's condition!  
*/
      {
        kbd_buffer_store_event_hold (&ie, hold_quit);
        count++;
      }

where we can move the second `if` into the first (according to the FIXME)

    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        eassert (ie.kind == NO_EVENT);
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
        kbd_buffer_store_event_hold (&ie, hold_quit);
        count++;
      }

and then we know that `hold_quit` starts at NO_EVENT and can only be
changed via this call to `kbd_buffer_store_event_hold`, but this call
will never change it because `ie.kind == GPM_CLICK_EVENT` and
kbd_buffer_store_event_hold would only ever touch it if the kind was
ASCII_KEYSTROKE_EVENT, so we know that `hold_quit->kind == NO_EVENT`
always holds and we can drop it.

The code is now (with added context):

    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        eassert (ie.kind == NO_EVENT);
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
        kbd_buffer_store_event (&ie);
        count++;
      }
    
    if (do_help
        /* FIXME: `hold_quit` is never NULL?!  */
        && !(hold_quit && hold_quit->kind != NO_EVENT))
      { ... }

and the last if's condition can now be simplified:

    bool do_help = false;
    [...]
    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        if (!NILP (help_echo_string)
            || !NILP (previous_help_echo_string))
          do_help = true;

        eassert (ie.kind == NO_EVENT);
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
        kbd_buffer_store_event (&ie);
        count++;
      }
    
    if (do_help)
      { ... }

and now we can again fold the second move into the tail of the first:

    bool do_help = false;
    [...]
    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        if (!NILP (help_echo_string)
            || !NILP (previous_help_echo_string))
          do_help = true;

        eassert (ie.kind == NO_EVENT);
        if (do_help)
          { ... }
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
        kbd_buffer_store_event (&ie);
        count++;
      }
    
and get rid of `do_help` altogether.

    [...]
    if (event->type & (GPM_MOVE | GPM_DRAG))
      {
        [...]
        if (!NILP (help_echo_string)
            || !NILP (previous_help_echo_string))
          { ... }
      }
    else
      {
        [...]
        eassert (ie.kind == GPM_CLICK_EVENT);
        kbd_buffer_store_event (&ie);
        count++;
      }
    
So, I installed the patch below.


        Stefan


diff --git a/src/keyboard.c b/src/keyboard.c
index 2e0143379a..49a0a8bd23 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7005,12 +7005,8 @@ tty_read_avail_input (struct terminal *terminal,
   if (gpm_tty == tty)
   {
       Gpm_Event event;
-      struct input_event gpm_hold_quit;
       int gpm, fd = gpm_fd;
 
-      EVENT_INIT (gpm_hold_quit);
-      gpm_hold_quit.kind = NO_EVENT;
-
       /* gpm==1 if event received.
          gpm==0 if the GPM daemon has closed the connection, in which case
                 Gpm_GetEvent closes gpm_fd and clears it to -1, which is why
@@ -7018,13 +7014,11 @@ tty_read_avail_input (struct terminal *terminal,
                select masks.
          gpm==-1 if a protocol error or EWOULDBLOCK; the latter is normal.  */
       while (gpm = Gpm_GetEvent (&event), gpm == 1) {
-         nread += handle_one_term_event (tty, &event, &gpm_hold_quit);
+         nread += handle_one_term_event (tty, &event);
       }
       if (gpm == 0)
        /* Presumably the GPM daemon has closed the connection.  */
        close_gpm (fd);
-      if (gpm_hold_quit.kind != NO_EVENT)
-         kbd_buffer_store_event (&gpm_hold_quit);
       if (nread)
          return nread;
   }
diff --git a/src/term.c b/src/term.c
index ff1aabfed2..3a13da165e 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2550,67 +2550,63 @@ term_mouse_click (struct input_event *result, Gpm_Event 
*event,
 }
 
 int
-handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event,
-                      struct input_event *hold_quit)
+handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event)
 {
   struct frame *f = XFRAME (tty->top_frame);
   struct input_event ie;
-  bool do_help = 0;
   int count = 0;
 
   EVENT_INIT (ie);
   ie.kind = NO_EVENT;
   ie.arg = Qnil;
 
-  if (event->type & (GPM_MOVE | GPM_DRAG)) {
-    previous_help_echo_string = help_echo_string;
-    help_echo_string = Qnil;
+  if (event->type & (GPM_MOVE | GPM_DRAG))
+    {
+      previous_help_echo_string = help_echo_string;
+      help_echo_string = Qnil;
 
-    Gpm_DrawPointer (event->x, event->y, fileno (tty->output));
+      Gpm_DrawPointer (event->x, event->y, fileno (tty->output));
 
-    if (!term_mouse_movement (f, event))
-      help_echo_string = previous_help_echo_string;
+      if (!term_mouse_movement (f, event))
+        help_echo_string = previous_help_echo_string;
 
-    /* If the contents of the global variable help_echo_string
-       has changed, generate a HELP_EVENT.  */
-    if (!NILP (help_echo_string)
-       || !NILP (previous_help_echo_string))
-      do_help = 1;
+      /* If the contents of the global variable help_echo_string
+         has changed, generate a HELP_EVENT.  */
+      if (!NILP (help_echo_string)
+         || !NILP (previous_help_echo_string))
+       {
+         Lisp_Object frame;
 
-    goto done;
-  }
-  else {
-    f->mouse_moved = 0;
-    term_mouse_click (&ie, event, f);
-    if (tty_handle_tab_bar_click (f, event->x, event->y,
-                                 (ie.modifiers & down_modifier) != 0, &ie))
-      {
-       /* tty_handle_tab_bar_click stores 2 events in the event
-          queue, so we are done here.  */
-       count += 2;
-       return count;
-      }
-  }
+         if (f)
+           XSETFRAME (frame, f);
+         else
+           frame = Qnil;
 
- done:
-  if (ie.kind != NO_EVENT)
-    {
-      kbd_buffer_store_event_hold (&ie, hold_quit);
-      count++;
+         gen_help_event (help_echo_string, frame, help_echo_window,
+                         help_echo_object, help_echo_pos);
+         count++;
+       }
     }
-
-  if (do_help
-      && !(hold_quit && hold_quit->kind != NO_EVENT))
+  else
     {
-      Lisp_Object frame;
-
-      if (f)
-       XSETFRAME (frame, f);
-      else
-       frame = Qnil;
-
-      gen_help_event (help_echo_string, frame, help_echo_window,
-                     help_echo_object, help_echo_pos);
+      f->mouse_moved = 0;
+      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); */
+      kbd_buffer_store_event (&ie);
       count++;
     }
 
diff --git a/src/termhooks.h b/src/termhooks.h
index d18b750c3a..6ab06ceff9 100644
--- a/src/termhooks.h
+++ b/src/termhooks.h
@@ -370,7 +370,7 @@ #define EVENT_INIT(event) memset (&(event), 0, sizeof 
(struct input_event))
 
 #ifdef HAVE_GPM
 #include <gpm.h>
-extern int handle_one_term_event (struct tty_display_info *, Gpm_Event *, 
struct input_event *);
+extern int handle_one_term_event (struct tty_display_info *, Gpm_Event *);
 #ifndef HAVE_WINDOW_SYSTEM
 extern void term_mouse_moveto (int, int);
 #endif