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