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

[Jared Finder]

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