mouse-face and help echo support for xterm mouse

mouse-face and help echo support for xterm mouse

[Jared Finder via Emacs development discussions.]

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

Attached is a patch that makes the xterm-mouse properly support 
mouse-face and help echo text.  It simply adds a new function to call 
from Lisp that does what the existing event handlers 
(handle_one_term_event for GPM, w32_read_socket for Windows, etc.) when 
they notice mouse movement.

I think with this the xterm-mouse is now at parity with other terminal 
mouse support.  Yay!

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Face-changing-text-properties-and-help-echo-now-work.patch --]
[-- Type: text/x-diff; name=0001-Face-changing-text-properties-and-help-echo-now-work.patch, Size: 3296 bytes --]

From b7eb78397bc96c0e1bc2d280bd6660bbaa779790 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 31 Oct 2020 21:25:47 -0800
Subject: [PATCH] Face-changing text properties and help-echo now work with
 xterm-mouse.

* src/term.c (handle-lisp-mouse-motion): New function like
handle_one_term_event but only for mouse motion and with no GPM code.
* lisp/xt-mouse.el (xterm-mouse--handle-mouse-motion): New function that
calls 'handle-lisp-mouse-motion'.
(xterm-mouse-translate-1): Call 'xterm-mouse--handle-mouse-motion'.
---
 lisp/xt-mouse.el |  9 +++++++++
 src/term.c       | 28 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
index f9c08f9a17..37550276f8 100644
--- a/lisp/xt-mouse.el
+++ b/lisp/xt-mouse.el
@@ -77,6 +77,7 @@ xterm-mouse-translate-1
               (copy-sequence event))
 	vec)
        (is-move
+        (xterm-mouse--handle-mouse-motion)
         (if track-mouse vec
           ;; Mouse movement events are currently supposed to be
           ;; suppressed.  Return no event.
@@ -106,8 +107,16 @@ xterm-mouse-translate-1
 	      (if (null track-mouse)
 		  (vector drag)
 		(push drag unread-command-events)
+                (xterm-mouse--handle-mouse-motion)
 		(vector (list 'mouse-movement ev-data))))))))))))
 
+(defun xterm-mouse--handle-mouse-motion ()
+  "Handle mouse motion that was just generated for XTerm mouse."
+  (let ((frame (selected-frame)))
+    (handle-lisp-mouse-motion frame
+                              (terminal-parameter frame 'xterm-mouse-x)
+                              (terminal-parameter frame 'xterm-mouse-y))))
+
 ;; These two variables have been converted to terminal parameters.
 ;;
 ;;(defvar xterm-mouse-x 0
diff --git a/src/term.c b/src/term.c
index ff1aabfed2..710b00e32f 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2617,6 +2617,33 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event,
   return count;
 }
 
+DEFUN ("handle-lisp-mouse-motion", Fhandle_lisp_mouse_motion,
+       Shandle_lisp_mouse_motion, 3, 3, 0,
+       doc: /* Handle mouse motion detected by Lisp code.
+
+This function should be called when Lisp code detects the mouse has
+moved, even if `track-mouse' is nil.  This handles updates that do not
+not rely on input events such as updating display for mouse-face
+proprties or updating the help echo text.  */)
+  (Lisp_Object frame, Lisp_Object mouse_x, Lisp_Object mouse_y)
+{
+  if (NILP (frame))
+    frame = selected_frame;
+
+  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);
+    }
+  return Qnil;
+}
+
 DEFUN ("gpm-mouse-start", Fgpm_mouse_start, Sgpm_mouse_start,
        0, 0, 0,
        doc: /* Open a connection to Gpm.
@@ -4568,6 +4595,7 @@ syms_of_term (void)
   defsubr (&Stty_top_frame);
   defsubr (&Ssuspend_tty);
   defsubr (&Sresume_tty);
+  defsubr (&Shandle_lisp_mouse_motion);
 #ifdef HAVE_GPM
   defsubr (&Sgpm_mouse_start);
   defsubr (&Sgpm_mouse_stop);
-- 
2.20.1

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

[Stefan Monnier]

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?


        Stefan

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

[Jared Finder via Emacs development discussions.]

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

   -- MJF

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

[Jared Finder via Emacs development discussions.]

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

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

[Stefan Monnier]

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

Hmm... this `handle_one_term_event` seems weirdly complex.
I don't understand it enough to judge if your rewrite is OK.

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

I don't know.  I don't even know why `hold_quit` is called that way:
I can't see any reason why it should hold "quit" events more than
anything else.

In the patch below I slightly tweaked the code to simplify the control
flow a tiny bit and to follow our coding convention on placement of
braces, and more importantly I added assertions which I believe always
hold along with comments pointing out things I don't understand.

Could someone help me clarify what's going on?

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

I suspect it would end up being fairly empty, and while I hate the
monster xdisp.c file, I also don't really like tiny files, so I'd prefer
if we could find a better home for it.


        Stefan

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

[Eli Zaretskii]

> Date: Tue, 03 Nov 2020 22:54:32 -0800
> Cc: "Jared Finder via \"Emacs development discussions.\"" <emacs-devel@gnu.org>
> From: Jared Finder via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> 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.

Thanks, see below.

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

Not sure why is this important.  You want to remove the hold_quit
stuff from the code?  I don't see that it complicates the code, so I'd
suggest not to waste your time on worrying about it.  Just leave it
alone.

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

No, new file for a single function is too much.  dispnew.c, I guess.

>        while (gpm = Gpm_GetEvent (&event), gpm == 1) {
> -	  nread += handle_one_term_event (tty, &event, &gpm_hold_quit);
> +	  nread += handle_one_term_event (tty, &event);

As explained above, I'd leave the signature alone, because it is not
guaranteed that every subroutine we call here doesn't need to use it.

> -  if (ie.kind != NO_EVENT)

I also don't think it's a good idea to remove this test.  The
functions you call don't guarantee to always return a meaningful
event, and the assertion you leave instead is too drastic: imagine
that it happens to some innocent user.

> +    kbd_buffer_store_event_hold (&ie, NULL);

If we eventually decide not to use the hold_quit stuff, you can just
call kbd_buffer_store_event here.

Thanks.

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 04 Nov 2020 09:13:07 -0500
> Cc: "Jared Finder via \"Emacs development discussions.\"" <emacs-devel@gnu.org>
> 
> In the patch below I slightly tweaked the code

What patch?

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

[Stefan Monnier]

>> In the patch below I slightly tweaked the code
> What patch?

Hmm... *the* patch, of course.


        Stefan


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))
+    {
     previous_help_echo_string = help_echo_string;
     help_echo_string = Qnil;
 
@@ -2577,29 +2578,45 @@
 	|| !NILP (previous_help_echo_string))
       do_help = 1;
 
-    goto done;
+      eassert (ie.kind == NO_EVENT);
   }
-  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);
   }
 
- 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);
       count++;
     }
 
   if (do_help
+      /* FIXME: `hold_quit` is never NULL?!  */
+      /* 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).  */
       && !(hold_quit && hold_quit->kind != NO_EVENT))
     {
       Lisp_Object frame;

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

[Jared Finder via Emacs development discussions.]

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

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

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Wed, 04 Nov 2020 13:47:29 -0500
> 
> `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.

Are you guys taking into consideration the fact that this is a TTY,
where C-g causes a SIGINT, so it can come in at any time?

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

[Stefan Monnier]

> Are you guys taking into consideration the fact that this is a TTY,
> where C-g causes a SIGINT, so it can come in at any time?

I believe so, yes.


        Stefan

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

[Jared Finder via Emacs development discussions.]

On 2020-11-04 11:05 am, Stefan Monnier wrote:
>> Are you guys taking into consideration the fact that this is a TTY,
>> where C-g causes a SIGINT, so it can come in at any time?
> 
> I believe so, yes.

I agree.  All of the reasoning was done by reasoning about local 
variables in C functions.  I do not see any code in the signal handling 
code that reaches into arbitrary local variables of another C function.

   -- MJF

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

[Jared Finder via Emacs development discussions.]

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



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?

With the code simplification in, this logic is now sharable between 
xterm-mouse and GPM.  Attached is an updated patch.

This patch does have one actual logic change: Previously 
handle_one_term_event might call gen_help_event if a GPM_MOVE_EVENT or 
GPM_DRAG_EVENT happened but the mouse position did not change.  With 
this patch, this is no longer the case.  From testing locally with 
running GPM mouse, this seems to not cause any user-visible change.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Face-changing-text-properties-and-help-echo-now-work.patch --]
[-- Type: text/x-diff; name=0001-Face-changing-text-properties-and-help-echo-now-work.patch, Size: 6680 bytes --]

From 98e18207726ddeb1f922e9b95f1fd4822502a66a Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 31 Oct 2020 21:25:47 -0800
Subject: [PATCH] Face-changing text properties and help-echo now work with
 xterm-mouse.

* src/dispnew.c (update_mouse_position): New function for mouse movement
logic in 'handle_one_term_event' that should be shared across GPM and
xterm-mouse.
(handle-lisp-mouse-motion): New lisp function, call it.
* lisp/xt-mouse.el (xterm-mouse--handle-mouse-motion): New function that
calls 'handle-lisp-mouse-motion'.
(xterm-mouse-translate-1): Call it.
* src/term.c (handle_one_term_event): Inline logic from
'term_mouse_movement' and call 'update_mouse_position'.
(term_mouse_movement): Delete.
---
 lisp/xt-mouse.el |  9 +++++++++
 src/dispextern.h |  1 +
 src/dispnew.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 src/term.c       | 46 ++++++++--------------------------------------
 4 files changed, 65 insertions(+), 38 deletions(-)

diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
index f9c08f9a17..37550276f8 100644
--- a/lisp/xt-mouse.el
+++ b/lisp/xt-mouse.el
@@ -77,6 +77,7 @@ xterm-mouse-translate-1
               (copy-sequence event))
 	vec)
        (is-move
+        (xterm-mouse--handle-mouse-motion)
         (if track-mouse vec
           ;; Mouse movement events are currently supposed to be
           ;; suppressed.  Return no event.
@@ -106,8 +107,16 @@ xterm-mouse-translate-1
 	      (if (null track-mouse)
 		  (vector drag)
 		(push drag unread-command-events)
+                (xterm-mouse--handle-mouse-motion)
 		(vector (list 'mouse-movement ev-data))))))))))))
 
+(defun xterm-mouse--handle-mouse-motion ()
+  "Handle mouse motion that was just generated for XTerm mouse."
+  (let ((frame (selected-frame)))
+    (handle-lisp-mouse-motion frame
+                              (terminal-parameter frame 'xterm-mouse-x)
+                              (terminal-parameter frame 'xterm-mouse-y))))
+
 ;; These two variables have been converted to terminal parameters.
 ;;
 ;;(defvar xterm-mouse-x 0
diff --git a/src/dispextern.h b/src/dispextern.h
index 848d3bcd20..da51772b37 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3606,6 +3606,7 @@ #define IMAGE_BACKGROUND_TRANSPARENT(img, f, mask)			      \
 extern void redraw_frame (struct frame *);
 extern bool update_frame (struct frame *, bool, bool);
 extern void update_frame_with_menu (struct frame *, int, int);
+extern int update_mouse_position (struct frame *, int, int);
 extern void bitch_at_user (void);
 extern void adjust_frame_glyphs (struct frame *);
 void free_glyphs (struct frame *);
diff --git a/src/dispnew.c b/src/dispnew.c
index 3f2ae3e6ad..545cae628f 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3323,6 +3323,52 @@ update_frame_with_menu (struct frame *f, int row, int col)
   display_completed = !paused_p;
 }
 
+/* Update the mouse position for a frame F.  This handles both
+   updating the display for mouse-face propreties and updating the
+   help echo text.
+
+   Returns the number of events generated.  */
+int
+update_mouse_position (struct frame *f, int x, int y)
+{
+  previous_help_echo_string = help_echo_string;
+  help_echo_string = Qnil;
+
+  note_mouse_highlight (f, x, y);
+
+  /* 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;
+      XSETFRAME(frame, f);
+
+      gen_help_event (help_echo_string, frame, help_echo_window,
+                      help_echo_object, help_echo_pos);
+      return 1;
+    }
+
+  return 0;
+}
+
+DEFUN ("handle-lisp-mouse-motion", Fhandle_lisp_mouse_motion,
+       Shandle_lisp_mouse_motion, 3, 3, 0,
+       doc: /* Handle mouse motion detected by Lisp code.
+
+This function should be called when Lisp code detects the mouse has
+moved, even if `track-mouse' is nil.  This handles updates that do not
+not rely on input events such as updating display for mouse-face
+proprties or updating the help echo text.  */)
+  (Lisp_Object frame, Lisp_Object mouse_x, Lisp_Object mouse_y)
+{
+  if (NILP (frame))
+    frame = selected_frame;
+
+  update_mouse_position (XFRAME (frame), XFIXNUM (mouse_x), XFIXNUM (mouse_y));
+  return Qnil;
+}
+
 \f
 /************************************************************************
 			 Window-based updates
@@ -6490,6 +6536,7 @@ syms_of_display (void)
 {
   defsubr (&Sredraw_frame);
   defsubr (&Sredraw_display);
+  defsubr (&Shandle_lisp_mouse_motion);
   defsubr (&Sframe_or_buffer_changed_p);
   defsubr (&Sopen_termscript);
   defsubr (&Sding);
diff --git a/src/term.c b/src/term.c
index 3a13da165e..df34983344 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2430,22 +2430,6 @@ tty_draw_row_with_mouse_face (struct window *w, struct glyph_row *row,
   cursor_to (f, save_y, save_x);
 }
 
-static bool
-term_mouse_movement (struct frame *frame, Gpm_Event *event)
-{
-  /* Has the mouse moved off the glyph it was on at the last sighting?  */
-  if (event->x != last_mouse_x || event->y != last_mouse_y)
-    {
-      frame->mouse_moved = 1;
-      note_mouse_highlight (frame, event->x, event->y);
-      /* Remember which glyph we're now on.  */
-      last_mouse_x = event->x;
-      last_mouse_y = event->y;
-      return 1;
-    }
-  return 0;
-}
-
 /* Return the current time, as a Time value.  Wrap around on overflow.  */
 static Time
 current_Time (void)
@@ -2562,30 +2546,16 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event)
 
   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));
 
-      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))
-	{
-	  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++;
-	}
+      /* Has the mouse moved off the glyph it was on at the last sighting?  */
+      if (event->x != last_mouse_x || event->y != last_mouse_y)
+        {
+          last_mouse_x = event->x;
+          last_mouse_y = event->y;
+          f->mouse_moved = 1;
+          count += update_mouse_position (f, event->x, event->y);
+        }
     }
   else
     {
-- 
2.20.1

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

[Stefan Monnier]

> With the code simplification in, this logic is now sharable between
> xterm-mouse and GPM.  Attached is an updated patch.

Thanks.

> This patch does have one actual logic change: Previously
> handle_one_term_event might call gen_help_event if a GPM_MOVE_EVENT or
> GPM_DRAG_EVENT happened but the mouse position did not change.  With this
> patch, this is no longer the case.  From testing locally with running GPM
> mouse, this seems to not cause any user-visible change.

I wouldn't worry about that, indeed.  It's typically the kind of minor
discrepancies that accrue when code is duplicated and which make merging
them back "fun".  You just have to choose which of the two behaviors is
preferable and I think here the better choice is to refrain from
generating an event when the position didn't actually change.

Further comments below.


        Stefan


> diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
> index f9c08f9a17..37550276f8 100644
> --- a/lisp/xt-mouse.el
> +++ b/lisp/xt-mouse.el
> @@ -77,6 +77,7 @@ xterm-mouse-translate-1
>                (copy-sequence event))
>  	vec)
>         (is-move
> +        (xterm-mouse--handle-mouse-motion)
>          (if track-mouse vec
>            ;; Mouse movement events are currently supposed to be
>            ;; suppressed.  Return no event.
> @@ -106,8 +107,16 @@ xterm-mouse-translate-1
>  	      (if (null track-mouse)
>  		  (vector drag)
>  		(push drag unread-command-events)
> +                (xterm-mouse--handle-mouse-motion)
>  		(vector (list 'mouse-movement ev-data))))))))))))

Hmm... `ev-data` includes the X/Y position, right?
Could we arrange to pass *that* data directly to
`xterm-mouse--handle-mouse-motion` rather than go through
(terminal-parameter frame 'xterm-mouse-x/y)?  It likely won't make
much difference in practice, but it would make the data flow more clear,
I think.

> +(defun xterm-mouse--handle-mouse-motion ()
> +  "Handle mouse motion that was just generated for XTerm mouse."
> +  (let ((frame (selected-frame)))
> +    (handle-lisp-mouse-motion frame
> +                              (terminal-parameter frame 'xterm-mouse-x)
> +                              (terminal-parameter frame 'xterm-mouse-y))))

This is the only caller to `handle-lisp-mouse-motion` and that function
has a "default frame to selected-frame" feature, so you could pass nil
instead of `frame`.  Better yet, drop that frame argument altogether.
And I think the function's name should make it clear it's for internal
use only, or otherwise try to use some prefix that indicates it's
related to the display.  Like `display-update-for-mouse-motion`?
[ I'm reminded here of the tension between using "mouse-motion" because it's
  shorter and using "mouse-movement" because that's the name of the
  event.  ]

> +This function should be called when Lisp code detects the mouse has
> +moved, even if `track-mouse' is nil.  This handles updates that do not
> +not rely on input events such as updating display for mouse-face

"not not"

> +proprties or updating the help echo text.  */)
      ^^
      e

> +  (Lisp_Object frame, Lisp_Object mouse_x, Lisp_Object mouse_y)
> +{
> +  if (NILP (frame))
> +    frame = selected_frame;
> +
> +  update_mouse_position (XFRAME (frame), XFIXNUM (mouse_x), XFIXNUM (mouse_y));
> +  return Qnil;
> +}

This will crash and burn if `frame` is an integer (and the XFIXNUM won't
crash and burn but they should also be preceded by CHECK_FIXNUMs).

>    if (event->type & (GPM_MOVE | GPM_DRAG))
>      {
[...]
> +      /* Has the mouse moved off the glyph it was on at the last sighting?  */
> +      if (event->x != last_mouse_x || event->y != last_mouse_y)
> +        {
> +          last_mouse_x = event->x;
> +          last_mouse_y = event->y;
> +          f->mouse_moved = 1;

Would it make sense to try and add this test to the "generic" part of
the code?


        Stefan

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

[Jared Finder via Emacs development discussions.]

On 2020-11-05 6:45 am, Stefan Monnier wrote:
>> 
>> diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
>> index f9c08f9a17..37550276f8 100644
>> --- a/lisp/xt-mouse.el
>> +++ b/lisp/xt-mouse.el
>> @@ -77,6 +77,7 @@ xterm-mouse-translate-1
>>                (copy-sequence event))
>>  	vec)
>>         (is-move
>> +        (xterm-mouse--handle-mouse-motion)
>>          (if track-mouse vec
>>            ;; Mouse movement events are currently supposed to be
>>            ;; suppressed.  Return no event.
>> @@ -106,8 +107,16 @@ xterm-mouse-translate-1
>>  	      (if (null track-mouse)
>>  		  (vector drag)
>>  		(push drag unread-command-events)
>> +                (xterm-mouse--handle-mouse-motion)
>>  		(vector (list 'mouse-movement ev-data))))))))))))
> 
> Hmm... `ev-data` includes the X/Y position, right?
> Could we arrange to pass *that* data directly to
> `xterm-mouse--handle-mouse-motion` rather than go through
> (terminal-parameter frame 'xterm-mouse-x/y)?  It likely won't make
> much difference in practice, but it would make the data flow more 
> clear,
> I think.

Using ev-data's x,y would add significant complexity.  ev-data is a 
posn, which is window part relative, not frame relative.

>> +(defun xterm-mouse--handle-mouse-motion ()
>> +  "Handle mouse motion that was just generated for XTerm mouse."
>> +  (let ((frame (selected-frame)))
>> +    (handle-lisp-mouse-motion frame
>> +                              (terminal-parameter frame 
>> 'xterm-mouse-x)
>> +                              (terminal-parameter frame 
>> 'xterm-mouse-y))))
> 
> This is the only caller to `handle-lisp-mouse-motion` and that function
> has a "default frame to selected-frame" feature, so you could pass nil
> instead of `frame`.  Better yet, drop that frame argument altogether.
> And I think the function's name should make it clear it's for internal
> use only, or otherwise try to use some prefix that indicates it's
> related to the display.  Like `display-update-for-mouse-motion`?

Is there a way I can name it that makes it clear this is an internal 
function and we may change the arguments in the future?  I was hoping 
that this function would work with background frames across different 
TTYs in case I can figure out how to get that working.

> [ I'm reminded here of the tension between using "mouse-motion" because 
> it's
>   shorter and using "mouse-movement" because that's the name of the
>   event.  ]

I squared this circle by using "mouse motion" for the concept and "mouse 
movement" for the event.  I could rename to mouse-movement if you'd 
prefer.

>> +This function should be called when Lisp code detects the mouse has
>> +moved, even if `track-mouse' is nil.  This handles updates that do 
>> not
>> +not rely on input events such as updating display for mouse-face
> 
> "not not"

Done.

>> +proprties or updating the help echo text.  */)
>       ^^
>       e

Done.

>> +  (Lisp_Object frame, Lisp_Object mouse_x, Lisp_Object mouse_y)
>> +{
>> +  if (NILP (frame))
>> +    frame = selected_frame;
>> +
>> +  update_mouse_position (XFRAME (frame), XFIXNUM (mouse_x), XFIXNUM 
>> (mouse_y));
>> +  return Qnil;
>> +}
> 
> This will crash and burn if `frame` is an integer (and the XFIXNUM 
> won't
> crash and burn but they should also be preceded by CHECK_FIXNUMs).

Done.

>>    if (event->type & (GPM_MOVE | GPM_DRAG))
>>      {
> [...]
>> +      /* Has the mouse moved off the glyph it was on at the last 
>> sighting?  */
>> +      if (event->x != last_mouse_x || event->y != last_mouse_y)
>> +        {
>> +          last_mouse_x = event->x;
>> +          last_mouse_y = event->y;
>> +          f->mouse_moved = 1;
> 
> Would it make sense to try and add this test to the "generic" part of
> the code?

This is not possible without much further work.  These are all tied with 
the C mouse event path: last_mouse_x, last_mouse_y, and 
frame.mouse_moved are all used by the mouse position hook and integrated 
with keyboard.c's mouse event generation.

Making this shared would require changing the way xterm-mouse currently 
reports all mouse events to not return data through input-decode-map and 
instead creating a Lisp interface to the C mouse event generation logic. 
  I tried playing around with this locally and it did seem like there is 
a path to make it work, but it would be a much bigger effort.


As there's no logic changes, I'll wait for my questions above to get 
addressed before sending an updated patch.

   -- MJF

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

[Stefan Monnier]

> Using ev-data's x,y would add significant complexity.  ev-data is a posn,
> which is window part relative, not frame relative.

Fair enough.

>>> +(defun xterm-mouse--handle-mouse-motion ()
>>> +  "Handle mouse motion that was just generated for XTerm mouse."
>>> +  (let ((frame (selected-frame)))
>>> +    (handle-lisp-mouse-motion frame
>>> +                              (terminal-parameter frame 'xterm-mouse-x)
>>> +                              (terminal-parameter frame
>>> 'xterm-mouse-y))))
>> This is the only caller to `handle-lisp-mouse-motion` and that function
>> has a "default frame to selected-frame" feature, so you could pass nil
>> instead of `frame`.  Better yet, drop that frame argument altogether.
>> And I think the function's name should make it clear it's for internal
>> use only, or otherwise try to use some prefix that indicates it's
>> related to the display.  Like `display-update-for-mouse-motion`?
> Is there a way I can name it that makes it clear this is an internal
> function and we may change the arguments in the future?

I'd suggest `display--update-for-mouse-action`, then.

> I was hoping that this function would work with background frames
> across different TTYs in case I can figure out how to get
> that working.

Not sure why that would require a frame that's not `selected_frame`.

>> [ I'm reminded here of the tension between using "mouse-motion"
>>   because it's shorter and using "mouse-movement" because that's the
>>   name of the event.  ]
> I squared this circle by using "mouse motion" for the concept and "mouse
> movement" for the event.

Sounds good.

>> Would it make sense to try and add this test to the "generic" part of
>> the code?
> This is not possible without much further work.  These are all tied with the
> C mouse event path: last_mouse_x, last_mouse_y, and frame.mouse_moved are
> all used by the mouse position hook and integrated with keyboard.c's mouse
> event generation.

Fair enough.  Maybe you can add a brief FIXME comment explaining just
that (or pointing to this discussion).


        Stefan

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

[Jared Finder via Emacs development discussions.]

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

All points addressed.  New patch attached.

I also attached a fix for mode-line-highlight that I noticed from this.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Face-changing-text-properties-and-help-echo-now-work.patch --]
[-- Type: text/x-diff; name=0001-Face-changing-text-properties-and-help-echo-now-work.patch, Size: 6992 bytes --]

From db70354d2282b7a03295d49ffbfdeca12e45d1b5 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 31 Oct 2020 21:25:47 -0800
Subject: [PATCH 1/2] Face-changing text properties and help-echo now work with
 xterm-mouse.

* src/dispnew.c (update_mouse_position): New function for mouse movement
logic in 'handle_one_term_event' that can be shared across different
mouse backends.
(display--update-for-mouse-movement): New lisp function, call it.
* lisp/xt-mouse.el (xterm-mouse--handle-mouse-movement): New function that
calls 'display--update-for-mouse-movement'.
(xterm-mouse-translate-1): Call it.
* src/term.c (handle_one_term_event): Inline logic from
'term_mouse_movement' and call 'update_mouse_position'.
(term_mouse_movement): Delete.
---
 lisp/xt-mouse.el |  7 +++++++
 src/dispextern.h |  1 +
 src/dispnew.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++
 src/term.c       | 52 +++++++++++++-----------------------------------
 4 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
index f9c08f9a17..9301476e81 100644
--- a/lisp/xt-mouse.el
+++ b/lisp/xt-mouse.el
@@ -77,6 +77,7 @@ xterm-mouse-translate-1
               (copy-sequence event))
 	vec)
        (is-move
+        (xterm-mouse--handle-mouse-movement)
         (if track-mouse vec
           ;; Mouse movement events are currently supposed to be
           ;; suppressed.  Return no event.
@@ -106,8 +107,14 @@ xterm-mouse-translate-1
 	      (if (null track-mouse)
 		  (vector drag)
 		(push drag unread-command-events)
+                (xterm-mouse--handle-mouse-movement)
 		(vector (list 'mouse-movement ev-data))))))))))))
 
+(defun xterm-mouse--handle-mouse-movement ()
+  "Handle mouse motion that was just generated for XTerm mouse."
+  (display--update-for-mouse-movement (terminal-parameter nil 'xterm-mouse-x)
+                                      (terminal-parameter nil 'xterm-mouse-y)))
+
 ;; These two variables have been converted to terminal parameters.
 ;;
 ;;(defvar xterm-mouse-x 0
diff --git a/src/dispextern.h b/src/dispextern.h
index 848d3bcd20..da51772b37 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3606,6 +3606,7 @@ #define IMAGE_BACKGROUND_TRANSPARENT(img, f, mask)			      \
 extern void redraw_frame (struct frame *);
 extern bool update_frame (struct frame *, bool, bool);
 extern void update_frame_with_menu (struct frame *, int, int);
+extern int update_mouse_position (struct frame *, int, int);
 extern void bitch_at_user (void);
 extern void adjust_frame_glyphs (struct frame *);
 void free_glyphs (struct frame *);
diff --git a/src/dispnew.c b/src/dispnew.c
index 3f2ae3e6ad..f51ed2709a 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3323,6 +3323,53 @@ update_frame_with_menu (struct frame *f, int row, int col)
   display_completed = !paused_p;
 }
 
+/* Update the mouse position for a frame F.  This handles both
+   updating the display for mouse-face propreties and updating the
+   help echo text.
+
+   Returns the number of events generated.  */
+int
+update_mouse_position (struct frame *f, int x, int y)
+{
+  previous_help_echo_string = help_echo_string;
+  help_echo_string = Qnil;
+
+  note_mouse_highlight (f, x, y);
+
+  /* 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;
+      XSETFRAME(frame, f);
+
+      gen_help_event (help_echo_string, frame, help_echo_window,
+                      help_echo_object, help_echo_pos);
+      return 1;
+    }
+
+  return 0;
+}
+
+DEFUN ("display--update-for-mouse-movement", Fdisplay__update_for_mouse_movement,
+       Sdisplay__update_for_mouse_movement, 2, 2, 0,
+       doc: /* Handle mouse movement detected by Lisp code.
+
+This function should be called when Lisp code detects the mouse has
+moved, even if `track-mouse' is nil.  This handles updates that do not
+rely on input events such as updating display for mouse-face
+properties or updating the help echo text.  */)
+  (Lisp_Object mouse_x, Lisp_Object mouse_y)
+{
+  CHECK_FIXNUM (mouse_x);
+  CHECK_FIXNUM (mouse_y);
+
+  update_mouse_position (XFRAME (selected_frame), XFIXNUM (mouse_x),
+                         XFIXNUM (mouse_y));
+  return Qnil;
+}
+
 \f
 /************************************************************************
 			 Window-based updates
@@ -6490,6 +6537,7 @@ syms_of_display (void)
 {
   defsubr (&Sredraw_frame);
   defsubr (&Sredraw_display);
+  defsubr (&Sdisplay__update_for_mouse_movement);
   defsubr (&Sframe_or_buffer_changed_p);
   defsubr (&Sopen_termscript);
   defsubr (&Sding);
diff --git a/src/term.c b/src/term.c
index 3a13da165e..a0738594bf 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2430,22 +2430,6 @@ tty_draw_row_with_mouse_face (struct window *w, struct glyph_row *row,
   cursor_to (f, save_y, save_x);
 }
 
-static bool
-term_mouse_movement (struct frame *frame, Gpm_Event *event)
-{
-  /* Has the mouse moved off the glyph it was on at the last sighting?  */
-  if (event->x != last_mouse_x || event->y != last_mouse_y)
-    {
-      frame->mouse_moved = 1;
-      note_mouse_highlight (frame, event->x, event->y);
-      /* Remember which glyph we're now on.  */
-      last_mouse_x = event->x;
-      last_mouse_y = event->y;
-      return 1;
-    }
-  return 0;
-}
-
 /* Return the current time, as a Time value.  Wrap around on overflow.  */
 static Time
 current_Time (void)
@@ -2562,30 +2546,22 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event)
 
   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));
 
-      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))
-	{
-	  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++;
-	}
+      /* Has the mouse moved off the glyph it was on at the last
+         sighting?  */
+      if (event->x != last_mouse_x || event->y != last_mouse_y)
+        {
+          /* FIXME: These three lines can not be moved into
+             update_mouse_position unless xterm-mouse gets updated to
+             generate mouse events via C code.  See
+             https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg00163.html */
+          last_mouse_x = event->x;
+          last_mouse_y = event->y;
+          f->mouse_moved = 1;
+
+          count += update_mouse_position (f, event->x, event->y);
+        }
     }
   else
     {
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-lisp-faces.el-mode-line-highlight-Use-box-only-on-no.patch --]
[-- Type: text/x-diff; name=0002-lisp-faces.el-mode-line-highlight-Use-box-only-on-no.patch, Size: 730 bytes --]

From bc569c9cc18072fa943afea4d4dafdf8655b997d Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 5 Nov 2020 21:15:08 -0800
Subject: [PATCH 2/2] * lisp/faces.el (mode-line-highlight): Use :box only on
 non-TTYs.

---
 lisp/faces.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 728f8b0fe6..875bee9910 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2578,7 +2578,7 @@ mode-line-inactive
   :group 'basic-faces)
 
 (defface mode-line-highlight
-  '((((class color) (min-colors 88))
+  '((((type graphic) (class color) (min-colors 88))
      :box (:line-width 2 :color "grey40" :style released-button))
     (t
      :inherit highlight))
-- 
2.20.1

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

[Eli Zaretskii]

> Date: Thu, 05 Nov 2020 21:23:26 -0800
> Cc: "Jared Finder via \"Emacs development discussions.\"" <emacs-devel@gnu.org>
> From: Jared Finder via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> All points addressed.  New patch attached.

Thanks.

> * src/dispnew.c (update_mouse_position): New function for mouse movement
> logic in 'handle_one_term_event' that can be shared across different
> mouse backends.
> (display--update-for-mouse-movement): New lisp function, call it.
> * lisp/xt-mouse.el (xterm-mouse--handle-mouse-movement): New function that
> calls 'display--update-for-mouse-movement'.
> (xterm-mouse-translate-1): Call it.
> * src/term.c (handle_one_term_event): Inline logic from
> 'term_mouse_movement' and call 'update_mouse_position'.
> (term_mouse_movement): Delete.

Nitpicking: the lines in the change log are too long, they will
overflow 80 columns when indented by TABs (which happens when we
generate a ChangeLog file from Git log).  Please use one of the Emacs
commands available for generating ChangeLog entries, they will keep
you from making these mistakes.

> +      XSETFRAME(frame, f);
                  ^
Please leave a space before the opening parenthesis, to conform to our
coding conventsions.

> +  update_mouse_position (XFRAME (selected_frame), XFIXNUM (mouse_x),
                            ^^^^^^^^^^^^^^^^^^^^^^^
A.k.a. SELECTED_FRAME().

>  (defface mode-line-highlight
> -  '((((class color) (min-colors 88))
> +  '((((type graphic) (class color) (min-colors 88))
>       :box (:line-width 2 :color "grey40" :style released-button))

I don't think I understand the rationale.  With TTYs supporting many
colors nowadays, and mode-line-highlight available on TTYs, what is
the problem you tried to fix here?

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

[Jared Finder via Emacs development discussions.]

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

On 2020-11-05 10:00 pm, Eli Zaretskii wrote:
>> Date: Thu, 05 Nov 2020 21:23:26 -0800
>> Cc: "Jared Finder via \"Emacs development discussions.\"" 
>> <emacs-devel@gnu.org>
>> From: Jared Finder via "Emacs development discussions." 
>> <emacs-devel@gnu.org>
>> 
>> All points addressed.  New patch attached.
> 
> Thanks.
> 
>> * src/dispnew.c (update_mouse_position): New function for mouse 
>> movement
>> logic in 'handle_one_term_event' that can be shared across different
>> mouse backends.
>> (display--update-for-mouse-movement): New lisp function, call it.
>> * lisp/xt-mouse.el (xterm-mouse--handle-mouse-movement): New function 
>> that
>> calls 'display--update-for-mouse-movement'.
>> (xterm-mouse-translate-1): Call it.
>> * src/term.c (handle_one_term_event): Inline logic from
>> 'term_mouse_movement' and call 'update_mouse_position'.
>> (term_mouse_movement): Delete.
> 
> Nitpicking: the lines in the change log are too long, they will
> overflow 80 columns when indented by TABs (which happens when we
> generate a ChangeLog file from Git log).  Please use one of the Emacs
> commands available for generating ChangeLog entries, they will keep
> you from making these mistakes.

Oops, sorry.  I hand-verified each row was 72 or less characters just 
now.  I will try to learn the Emacs commands for dealing with 
changelogs.

>> +      XSETFRAME(frame, f);
>                   ^
> Please leave a space before the opening parenthesis, to conform to our
> coding conventsions.

Done.

>> +  update_mouse_position (XFRAME (selected_frame), XFIXNUM (mouse_x),
>                             ^^^^^^^^^^^^^^^^^^^^^^^
> A.k.a. SELECTED_FRAME().

Done.

>>  (defface mode-line-highlight
>> -  '((((class color) (min-colors 88))
>> +  '((((type graphic) (class color) (min-colors 88))
>>       :box (:line-width 2 :color "grey40" :style released-button))
> 
> I don't think I understand the rationale.  With TTYs supporting many
> colors nowadays, and mode-line-highlight available on TTYs, what is
> the problem you tried to fix here?

Are there any TTYs that support :box?  None of the platforms I tested 
locally on do, they instead just ignore the :box aspect of any face.


Updated patches attached.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Face-changing-text-properties-and-help-echo-now-work.patch --]
[-- Type: text/x-diff; name=0001-Face-changing-text-properties-and-help-echo-now-work.patch, Size: 6987 bytes --]

From 039c399a5078a9e95d9f49cb8b5a9a941494bf57 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Sat, 31 Oct 2020 21:25:47 -0800
Subject: [PATCH 1/2] Face-changing text properties and help-echo now work with
 xterm-mouse.

* src/dispnew.c (update_mouse_position): New function for mouse
movement logic in 'handle_one_term_event' that can be shared across
different mouse backends.
(display--update-for-mouse-movement): New lisp function, call it.
* lisp/xt-mouse.el (xterm-mouse--handle-mouse-movement): New function
that calls 'display--update-for-mouse-movement'.
(xterm-mouse-translate-1): Call it.
* src/term.c (handle_one_term_event): Inline logic from
'term_mouse_movement' and call 'update_mouse_position'.
(term_mouse_movement): Delete.
---
 lisp/xt-mouse.el |  7 +++++++
 src/dispextern.h |  1 +
 src/dispnew.c    | 48 ++++++++++++++++++++++++++++++++++++++++++++
 src/term.c       | 52 +++++++++++++-----------------------------------
 4 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/lisp/xt-mouse.el b/lisp/xt-mouse.el
index f9c08f9a17..9301476e81 100644
--- a/lisp/xt-mouse.el
+++ b/lisp/xt-mouse.el
@@ -77,6 +77,7 @@ xterm-mouse-translate-1
               (copy-sequence event))
 	vec)
        (is-move
+        (xterm-mouse--handle-mouse-movement)
         (if track-mouse vec
           ;; Mouse movement events are currently supposed to be
           ;; suppressed.  Return no event.
@@ -106,8 +107,14 @@ xterm-mouse-translate-1
 	      (if (null track-mouse)
 		  (vector drag)
 		(push drag unread-command-events)
+                (xterm-mouse--handle-mouse-movement)
 		(vector (list 'mouse-movement ev-data))))))))))))
 
+(defun xterm-mouse--handle-mouse-movement ()
+  "Handle mouse motion that was just generated for XTerm mouse."
+  (display--update-for-mouse-movement (terminal-parameter nil 'xterm-mouse-x)
+                                      (terminal-parameter nil 'xterm-mouse-y)))
+
 ;; These two variables have been converted to terminal parameters.
 ;;
 ;;(defvar xterm-mouse-x 0
diff --git a/src/dispextern.h b/src/dispextern.h
index 848d3bcd20..da51772b37 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3606,6 +3606,7 @@ #define IMAGE_BACKGROUND_TRANSPARENT(img, f, mask)			      \
 extern void redraw_frame (struct frame *);
 extern bool update_frame (struct frame *, bool, bool);
 extern void update_frame_with_menu (struct frame *, int, int);
+extern int update_mouse_position (struct frame *, int, int);
 extern void bitch_at_user (void);
 extern void adjust_frame_glyphs (struct frame *);
 void free_glyphs (struct frame *);
diff --git a/src/dispnew.c b/src/dispnew.c
index 3f2ae3e6ad..2e40d458d1 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3323,6 +3323,53 @@ update_frame_with_menu (struct frame *f, int row, int col)
   display_completed = !paused_p;
 }
 
+/* Update the mouse position for a frame F.  This handles both
+   updating the display for mouse-face propreties and updating the
+   help echo text.
+
+   Returns the number of events generated.  */
+int
+update_mouse_position (struct frame *f, int x, int y)
+{
+  previous_help_echo_string = help_echo_string;
+  help_echo_string = Qnil;
+
+  note_mouse_highlight (f, x, y);
+
+  /* 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;
+      XSETFRAME (frame, f);
+
+      gen_help_event (help_echo_string, frame, help_echo_window,
+                      help_echo_object, help_echo_pos);
+      return 1;
+    }
+
+  return 0;
+}
+
+DEFUN ("display--update-for-mouse-movement", Fdisplay__update_for_mouse_movement,
+       Sdisplay__update_for_mouse_movement, 2, 2, 0,
+       doc: /* Handle mouse movement detected by Lisp code.
+
+This function should be called when Lisp code detects the mouse has
+moved, even if `track-mouse' is nil.  This handles updates that do not
+rely on input events such as updating display for mouse-face
+properties or updating the help echo text.  */)
+  (Lisp_Object mouse_x, Lisp_Object mouse_y)
+{
+  CHECK_FIXNUM (mouse_x);
+  CHECK_FIXNUM (mouse_y);
+
+  update_mouse_position (SELECTED_FRAME (), XFIXNUM (mouse_x),
+                         XFIXNUM (mouse_y));
+  return Qnil;
+}
+
 \f
 /************************************************************************
 			 Window-based updates
@@ -6490,6 +6537,7 @@ syms_of_display (void)
 {
   defsubr (&Sredraw_frame);
   defsubr (&Sredraw_display);
+  defsubr (&Sdisplay__update_for_mouse_movement);
   defsubr (&Sframe_or_buffer_changed_p);
   defsubr (&Sopen_termscript);
   defsubr (&Sding);
diff --git a/src/term.c b/src/term.c
index 3a13da165e..a0738594bf 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2430,22 +2430,6 @@ tty_draw_row_with_mouse_face (struct window *w, struct glyph_row *row,
   cursor_to (f, save_y, save_x);
 }
 
-static bool
-term_mouse_movement (struct frame *frame, Gpm_Event *event)
-{
-  /* Has the mouse moved off the glyph it was on at the last sighting?  */
-  if (event->x != last_mouse_x || event->y != last_mouse_y)
-    {
-      frame->mouse_moved = 1;
-      note_mouse_highlight (frame, event->x, event->y);
-      /* Remember which glyph we're now on.  */
-      last_mouse_x = event->x;
-      last_mouse_y = event->y;
-      return 1;
-    }
-  return 0;
-}
-
 /* Return the current time, as a Time value.  Wrap around on overflow.  */
 static Time
 current_Time (void)
@@ -2562,30 +2546,22 @@ handle_one_term_event (struct tty_display_info *tty, Gpm_Event *event)
 
   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));
 
-      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))
-	{
-	  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++;
-	}
+      /* Has the mouse moved off the glyph it was on at the last
+         sighting?  */
+      if (event->x != last_mouse_x || event->y != last_mouse_y)
+        {
+          /* FIXME: These three lines can not be moved into
+             update_mouse_position unless xterm-mouse gets updated to
+             generate mouse events via C code.  See
+             https://lists.gnu.org/archive/html/emacs-devel/2020-11/msg00163.html */
+          last_mouse_x = event->x;
+          last_mouse_y = event->y;
+          f->mouse_moved = 1;
+
+          count += update_mouse_position (f, event->x, event->y);
+        }
     }
   else
     {
-- 
2.20.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-lisp-faces.el-mode-line-highlight-Use-box-only-on-no.patch --]
[-- Type: text/x-diff; name=0002-lisp-faces.el-mode-line-highlight-Use-box-only-on-no.patch, Size: 730 bytes --]

From dcf2b7070f48676960fd99f497df6bcb1035ce2a Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 5 Nov 2020 21:15:08 -0800
Subject: [PATCH 2/2] * lisp/faces.el (mode-line-highlight): Use :box only on
 non-TTYs.

---
 lisp/faces.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 728f8b0fe6..875bee9910 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2578,7 +2578,7 @@ mode-line-inactive
   :group 'basic-faces)
 
 (defface mode-line-highlight
-  '((((class color) (min-colors 88))
+  '((((type graphic) (class color) (min-colors 88))
      :box (:line-width 2 :color "grey40" :style released-button))
     (t
      :inherit highlight))
-- 
2.20.1

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

[Eli Zaretskii]

> Date: Thu, 05 Nov 2020 22:46:32 -0800
> From: Jared Finder <jared@finder.org>
> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> >>  (defface mode-line-highlight
> >> -  '((((class color) (min-colors 88))
> >> +  '((((type graphic) (class color) (min-colors 88))
> >>       :box (:line-width 2 :color "grey40" :style released-button))
> > 
> > I don't think I understand the rationale.  With TTYs supporting many
> > colors nowadays, and mode-line-highlight available on TTYs, what is
> > the problem you tried to fix here?
> 
> Are there any TTYs that support :box?  None of the platforms I tested 
> locally on do, they instead just ignore the :box aspect of any face.

That's true, but having conditions where they aren't necessary is not
good for maintenance, because they aren't future-proof: they will need
changes should someone implement the :box attribute for TTYs.  We've
bumped into such problems many times during the last two decades: as
more and more display features (colors, menus, mouse) became supported
on TTYs, we time after time found them not working in some package,
because someone arbitrarily disabled that for TTYs on the assumption
that "TTYs cannot possibly do that".  So attributes that are silently
and harmlessly ignored should not in general be disabled.

If you want, we can add a more reasonable condition, using the
'supports' keyword, requesting specifically support for the :box
attribute.

Thanks.

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

[Jared Finder via Emacs development discussions.]

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

On 2020-11-05 11:39 pm, Eli Zaretskii wrote:
>> Date: Thu, 05 Nov 2020 22:46:32 -0800
>> From: Jared Finder <jared@finder.org>
>> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
>> 
>> Are there any TTYs that support :box?  None of the platforms I tested
>> locally on do, they instead just ignore the :box aspect of any face.
> 
> [...]
> 
> If you want, we can add a more reasonable condition, using the
> 'supports' keyword, requesting specifically support for the :box
> attribute.

Thank you!

I was not aware of the supports keyword for defface and it certainly is 
better here. Updated patch attached.

   -- MJF

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-lisp-faces.el-mode-line-highlight-Use-box-only-when-.patch --]
[-- Type: text/x-diff; name=0002-lisp-faces.el-mode-line-highlight-Use-box-only-when-.patch, Size: 736 bytes --]

From 7787c5370f2bdc4c56a7afd92b524445a3b713d3 Mon Sep 17 00:00:00 2001
From: Jared Finder <jared@finder.org>
Date: Thu, 5 Nov 2020 21:15:08 -0800
Subject: [PATCH 2/2] * lisp/faces.el (mode-line-highlight): Use :box only when
 supported.

---
 lisp/faces.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/faces.el b/lisp/faces.el
index 728f8b0fe6..7355e1dd0a 100644
--- a/lisp/faces.el
+++ b/lisp/faces.el
@@ -2578,7 +2578,7 @@ mode-line-inactive
   :group 'basic-faces)
 
 (defface mode-line-highlight
-  '((((class color) (min-colors 88))
+  '((((supports :box t) (class color) (min-colors 88))
      :box (:line-width 2 :color "grey40" :style released-button))
     (t
      :inherit highlight))
-- 
2.20.1

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

[Eli Zaretskii]

> Date: Thu, 05 Nov 2020 22:46:32 -0800
> From: Jared Finder <jared@finder.org>
> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> Updated patches attached.

Thanks, I pushed the first one to the master branch.

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

[Eli Zaretskii]

> Date: Fri, 06 Nov 2020 17:22:57 -0800
> From: Jared Finder <jared@finder.org>
> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> 
> > If you want, we can add a more reasonable condition, using the
> > 'supports' keyword, requesting specifically support for the :box
> > attribute.
> 
> Thank you!
> 
> I was not aware of the supports keyword for defface and it certainly is 
> better here. Updated patch attached.

Thanks, pushed to the master branch.