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

[Jared Finder via "Emacs development discussions." <emacs-devel@gnu.org>]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

On 2020-11-01 7:56 am, Jared Finder wrote:
> On 2020-11-01 5:39 am, Stefan Monnier wrote:
>> Hi Jared,
>> 
>> I really like this new feature but have just one comment/question?
>> 
>>> +  previous_help_echo_string = help_echo_string;
>>> +  help_echo_string = Qnil;
>>> +
>>> +  note_mouse_highlight(XFRAME(frame), XFIXNUM (mouse_x), XFIXNUM 
>>> (mouse_y));
>>> +
>>> +  if (!NILP (help_echo_string)
>>> +      || !NILP (previous_help_echo_string))
>>> +    {
>>> +      gen_help_event (help_echo_string, frame, help_echo_window,
>>> +                      help_echo_object, help_echo_pos);
>>> +    }
>> 
>> I see this exact same code in other C files.
>> Could we move it to a file where we can share it instead of having
>> N copies?
> 
> I completely agree, not just for this code but also for mouse handling
> in general.  I think there should be a shared mouse interface with
> individual C functions for each type of mouse event: mouse move, mouse
> click, etc.  Translating OS-specific events to this shared
> functionality would continue to be OS-specific, but the actual
> handling of these events, such as this logic, would be fully shared.
> For example, this would unify the different codepaths between TTY
> menus for GPM, xterm-mouse, and NT Emacs.
> 
> I would be happy to help with this next.  However I need some help.  I
> can only locally build and test for Linux terminal with xterm-mouse or
> GPM handling the mouse.  Is there someone who can help for other
> platforms?  And should the GUI platforms be included as well?  (I
> suspect yes is the right answer.)

Toward proving that the code could be shared, I refactored the GPM mouse 
logic so that it was clearly apparent how to share it with 
handle-lisp-mouse-motion.  That patch is attached.  I'd like to make 
sure this looks like an appropriate change to make.  If so, I will 
finish up the patch.

Two specific questions:

1. To enable sharing logic, I need to encode a handful of assumptions 
that I believe are true today (example: no need to handle quit-char in 
GPM handling).  Do these assumptions look reasonable?

2. In what file should such a shared function go?  My initial thought is 
a new file "mouse.c" as it would hold shared mouse logic.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Simplifying-the-code-in-handle_one_term_event.patch --]
[-- Type: text/x-diff; name=0001-WIP-Simplifying-the-code-in-handle_one_term_event.patch, Size: 4872 bytes --]

From 186983baf6d316aeef964c5e84485bb8964eb891 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Tue, 3 Nov 2020 22:15:36 -0800
Subject: [PATCH] WIP: Simplifying the code in handle_one_term_event.

Simplifying things so that the mouse movement code looks similar to
handle-lisp-mouse-motion.  Specifically:

1) Removing hold_quit from handle_one_term_event.  GPM only ever
reports click events or move events, so no need to hold quit events
until later.

2) do_help is only ever set for mouse movement events, so directly
inline that branch.

3) f is guaranteed to be non-null, so remove branches that check.

4) ie is only ever set to GPM_CLICK_EVENT, so directly inline that
branch.
---
 src/keyboard.c  |  8 +-------
 src/term.c      | 53 ++++++++++++++++++++-----------------------------
 src/termhooks.h |  2 +-
 3 files changed, 23 insertions(+), 40 deletions(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index 403f583689..598e86e2a5 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -7025,12 +7025,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
@@ -7038,13 +7034,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 b6fa93bb62..f40cae4efd 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2550,18 +2550,11 @@ 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;
@@ -2575,11 +2568,22 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event,
        has changed, generate a HELP_EVENT.  */
     if (!NILP (help_echo_string)
 	|| !NILP (previous_help_echo_string))
-      do_help = 1;
+      {
+        Lisp_Object frame;
 
-    goto done;
+        XSETFRAME (frame, f);
+        gen_help_event (help_echo_string, frame, help_echo_window,
+                        help_echo_object, help_echo_pos);
+        count++;
+      }
   }
   else {
+    struct input_event ie;
+
+    EVENT_INIT (ie);
+    ie.kind = NO_EVENT;
+    ie.arg = Qnil;
+
     f->mouse_moved = 0;
     term_mouse_click (&ie, event, f);
     if (tty_handle_tab_bar_click (f, event->x, event->y,
@@ -2590,29 +2594,14 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event,
        count += 2;
        return count;
       }
-  }
 
- done:
-  if (ie.kind != NO_EVENT)
-    {
-      kbd_buffer_store_event_hold (&ie, hold_quit);
-      count++;
-    }
-
-  if (do_help
-      && !(hold_quit && hold_quit->kind != NO_EVENT))
-    {
-      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);
-      count++;
-    }
+    /* ie can only be of the event type GPM_CLICK_EVENT., it could
+       never be a quit-char keystroke event.  Therefore, there is no
+       need to queue quit events.  */
+    eassert (ie.kind == GPM_CLICK_EVENT);
+    kbd_buffer_store_event_hold (&ie, NULL);
+    count++;
+  }
 
   return 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
-- 
2.20.1