all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Jared Finder <jared@finder.org>
Cc: "Jared Finder via \"Emacs development discussions.\""
	<emacs-devel@gnu.org>
Subject: Re: mouse-face and help echo support for xterm mouse
Date: Thu, 05 Nov 2020 09:45:46 -0500	[thread overview]
Message-ID: <jwvzh3vlw42.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <db6e9182ee40b74ae99c5aadbb50ad65@finder.org> (Jared Finder's message of "Thu, 05 Nov 2020 00:15:10 -0800")

> 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




  reply	other threads:[~2020-11-05 14:45 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-01  5:46 mouse-face and help echo support for xterm mouse Jared Finder via Emacs development discussions.
2020-11-01 13:39 ` Stefan Monnier
2020-11-01 15:56   ` Jared Finder via Emacs development discussions.
2020-11-04  6:54     ` Jared Finder via Emacs development discussions.
2020-11-04 14:13       ` Stefan Monnier
2020-11-04 15:46         ` Eli Zaretskii
2020-11-04 15:56           ` Stefan Monnier
2020-11-04 17:54             ` Jared Finder via Emacs development discussions.
2020-11-04 18:47               ` Stefan Monnier
2020-11-04 18:51                 ` Eli Zaretskii
2020-11-04 19:05                   ` Stefan Monnier
2020-11-04 19:10                     ` Jared Finder via Emacs development discussions.
2020-11-04 15:19       ` Eli Zaretskii
2020-11-05  8:15   ` Jared Finder via Emacs development discussions.
2020-11-05 14:45     ` Stefan Monnier [this message]
2020-11-05 19:58       ` Jared Finder via Emacs development discussions.
2020-11-05 20:18         ` Stefan Monnier
2020-11-06  5:23           ` Jared Finder via Emacs development discussions.
2020-11-06  6:00             ` Eli Zaretskii
2020-11-06  6:46               ` Jared Finder via Emacs development discussions.
2020-11-06  7:39                 ` Eli Zaretskii
2020-11-07  1:22                   ` Jared Finder via Emacs development discussions.
2020-11-14 12:38                     ` Eli Zaretskii
2020-11-14 12:35                 ` Eli Zaretskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwvzh3vlw42.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=emacs-devel@gnu.org \
    --cc=jared@finder.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.