Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Po Lu]

Dmitry Gutov <dgutov@yandex.ru> writes:

> branch: master
> commit 128ed5c9f17fab87fdb679326035aa2598612658
> Author: Dmitry Gutov <dmitry@gutov.dev>
> Commit: Dmitry Gutov <dmitry@gutov.dev>
>
>     Add one more mouse-set-point call to functions xref-find-*-at-mouse
>     
>     * lisp/progmodes/xref.el (xref-find-definitions-at-mouse)
>     (xref-find-references-at-mouse): Call mouse-set-point to ensure
>     that the search is initiated at the same place where
>     xref-backend-identifier-at-point was called (bug#65578).
> ---
>  lisp/progmodes/xref.el | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 3f75f8d7132..dbafa00c3ad 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -1638,7 +1638,9 @@ This command is intended to be bound to a mouse event."
>             (mouse-set-point event)
>             (xref-backend-identifier-at-point (xref-find-backend)))))
>      (if identifier
> -        (xref-find-definitions identifier)
> +        (progn
> +          (mouse-set-point event)
> +          (xref-find-definitions identifier))
>        (user-error "No identifier here"))))
>  
>  ;;;###autoload
> @@ -1652,6 +1654,7 @@ This command is intended to be bound to a mouse event."
>             (xref-backend-identifier-at-point (xref-find-backend)))))
>      (if identifier
>          (let ((xref-prompt-for-identifier nil))
> +          (mouse-set-point event)
>            (xref-find-references identifier))
>        (user-error "No identifier here"))))
>  

My experience with the entire touch screen affair says that many such
errors would never have been written had `xref-find-definitions' taken
an EVENT argument and used its posn-point if present.  Something to
consider, I suppose.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Dmitry Gutov]

On 31/08/2023 03:43, Po Lu wrote:
> My experience with the entire touch screen affair says that many such
> errors would never have been written had `xref-find-definitions' taken
> an EVENT argument and used its posn-point if present.  Something to
> consider, I suppose.

Sorry, I don't quite understand which change you have in mind.

The issue here was that we needed to move to the click's location both 
when detecting the identifier (at that point), and before using the 
identifier to find a set of locations.

If we used a POSN value, it would also have to be used in two places, I 
guess. One of which we could just as easily missed as we had here.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Po Lu]

Dmitry Gutov <dmitry@gutov.dev> writes:

> On 31/08/2023 03:43, Po Lu wrote:
>> My experience with the entire touch screen affair says that many such
>> errors would never have been written had `xref-find-definitions' taken
>> an EVENT argument and used its posn-point if present.  Something to
>> consider, I suppose.
>
> Sorry, I don't quite understand which change you have in mind.
>
> The issue here was that we needed to move to the click's location both
> when detecting the identifier (at that point), and before using the
> identifier to find a set of locations.
>
> If we used a POSN value, it would also have to be used in two places,
> I guess. One of which we could just as easily missed as we had here.

In my opinion, commands or functions that respond to the location of a
click should always take an EVENT argument, and use:

  (posn-point (event-start event))

if EVENT is present and incorporates a mouse position list.  Rather than
searching for an identifier at point, it should search for an identifier
at the point of the mouse click.  Adhering to such a convention is
almost guaranteed to eliminate every bug where Emacs consults something
under point instead of the mouse pointer when the user meant the latter.

Thanks.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Dmitry Gutov]

On 31/08/2023 04:10, Po Lu wrote:
>> On 31/08/2023 03:43, Po Lu wrote:
>>> My experience with the entire touch screen affair says that many such
>>> errors would never have been written had `xref-find-definitions' taken
>>> an EVENT argument and used its posn-point if present.  Something to
>>> consider, I suppose.
>> Sorry, I don't quite understand which change you have in mind.
>>
>> The issue here was that we needed to move to the click's location both
>> when detecting the identifier (at that point), and before using the
>> identifier to find a set of locations.
>>
>> If we used a POSN value, it would also have to be used in two places,
>> I guess. One of which we could just as easily missed as we had here.
> In my opinion, commands or functions that respond to the location of a
> click should always take an EVENT argument, and use:
> 
>    (posn-point (event-start event))
> 
> if EVENT is present and incorporates a mouse position list.  Rather than
> searching for an identifier at point, it should search for an identifier
> at the point of the mouse click.  Adhering to such a convention is
> almost guaranteed to eliminate every bug where Emacs consults something
> under point instead of the mouse pointer when the user meant the latter.

Sorry, I'm still not seeing your point. Especially the last paragraph in 
my last email seems to remain unaddressed.

And both commands take EVENT as an argument already.

The problem is not where is searched for an identifier, but *from where* 
it searched for the identifier's locations. And that position is 
important only for particular Xref backends (both LSP-based ones, I'm 
assuming).

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Po Lu]

Dmitry Gutov <dmitry@gutov.dev> writes:

> Sorry, I'm still not seeing your point. Especially the last paragraph
> in my last email seems to remain unaddressed.
>
> And both commands take EVENT as an argument already.
>
> The problem is not where is searched for an identifier, but *from
> where* it searched for the identifier's locations. And that position
> is important only for particular Xref backends (both LSP-based ones,
> I'm assuming).

My point is this: xref-find-definitions itself, not its callers, should
take EVENT, then call (goto-char (posn-point (event-start event)))
within.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Juri Linkov]

> My point is this: xref-find-definitions itself, not its callers, should
> take EVENT, then call (goto-char (posn-point (event-start event)))
> within.

Alas, most of the commands don't support EVENT.  It's a real problem
for menu items at the bottom of the context menu where submenus are
constructed from major/minor modes.  For example, in emacs-lisp-mode
clicking on "Emacs-Lisp -> Indent Line" operates on the line where point
is located, not on the clicked line.  And the rest of commands do the same.

Maybe while constructing the context menu, some commands should be wrapped
with a new function that will use '(save-excursion (mouse-set-point event))'.
The problem is that this is necessary only for some of them, not all:
only for the commands that rely on the position of the mouse click,
not for the commands that operate on the whole buffer.  For example,
in emacs-lisp-mode only for the menu item "Evaluate Last S-expression",
but not for "Evaluate Buffer".

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Po Lu]

Juri Linkov <juri@linkov.net> writes:

> The problem is that this is necessary only for some of them, not all:
> only for the commands that rely on the position of the mouse click,
> not for the commands that operate on the whole buffer.  For example,
> in emacs-lisp-mode only for the menu item "Evaluate Last S-expression",
> but not for "Evaluate Buffer".

Mind however that many of these commands are also meant to operate from
the mode line lighters, in which case this is precisely the desired
behavior.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Juri Linkov]

>> The problem is that this is necessary only for some of them, not all:
>> only for the commands that rely on the position of the mouse click,
>> not for the commands that operate on the whole buffer.  For example,
>> in emacs-lisp-mode only for the menu item "Evaluate Last S-expression",
>> but not for "Evaluate Buffer".
>
> Mind however that many of these commands are also meant to operate from
> the mode line lighters, in which case this is precisely the desired
> behavior.

I see that such events show they originate from 'mode-line',
and events from the menu bar contain 'menu-bar'.  So maybe
it would be sufficient to filter out 'mode-line' and 'menu-bar'.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Juri Linkov]

>> Mind however that many of these commands are also meant to operate from
>> the mode line lighters, in which case this is precisely the desired
>> behavior.
>
> I see that such events show they originate from 'mode-line',
> and events from the menu bar contain 'menu-bar'.  So maybe
> it would be sufficient to filter out 'mode-line' and 'menu-bar'.

Instead of checking 'posn-area', much simpler would be to implement this
only for the context-menu with a new option similar to 'mouse-yank-at-point'.

Any objections to enabling this by default?

```
diff --git a/lisp/mouse.el b/lisp/mouse.el
index 49549093985..48a3ab1acd3 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -407,6 +407,11 @@ context-menu-filter-function
   :type '(choice (const nil) function)
   :version "28.1")
 
+(defcustom context-menu-move-point nil
+  "If non-nil, move point to the mouse click before opening context menu."
+  :type 'boolean
+  :version "30.1")
+
 (defun context-menu-map (&optional click)
   "Return menu map constructed for context near mouse CLICK.
 The menu is populated by calling functions from `context-menu-functions'.
@@ -426,6 +431,9 @@ context-menu-map
     (unless (eq (selected-window) window)
       (select-window window))
 
+    (when context-menu-move-point
+      (goto-char (posn-point (event-start click))))
+
     (if (functionp fun)
         (setq menu (funcall fun menu click))
       (run-hook-wrapped 'context-menu-functions
```

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Juri Linkov]

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

>>> Mind however that many of these commands are also meant to operate from
>>> the mode line lighters, in which case this is precisely the desired
>>> behavior.
>>
>> I see that such events show they originate from 'mode-line',
>> and events from the menu bar contain 'menu-bar'.  So maybe
>> it would be sufficient to filter out 'mode-line' and 'menu-bar'.
>
> Instead of checking 'posn-area', much simpler would be to implement this
> only for the context-menu with a new option similar to 'mouse-yank-at-point'.
>
> +(defcustom context-menu-move-point nil
> +  "If non-nil, move point to the mouse click before opening context menu."
> +  :type 'boolean
> +  :version "30.1")

This turned out to be quite useless.  Because it's too annoying when
a command that operates on the whole buffer moves point to a random place
where the mouse happened to be clicked.

This means there is a need to distinguish commands for which the context
is the whole buffer from the commands where context is narrowed to the
clicked position.

Two examples:

1. 'outline-hide-body' hides body lines in the whole buffer,
   thus should be declared with context of the buffer:

```
(defun outline-hide-body ()
  "Hide all body lines in buffer, leaving all headings visible.
Note that this does not hide the lines preceding the first heading line."
  (declare (context buffer))
  (interactive)
  (outline-hide-region-body (point-min) (point-max)))
```

2. 'outline-hide-entry' hides body after the clicked heading,
   so should be declared with context of point:

```
(defun outline-hide-entry ()
  "Hide the body directly following this heading."
  (declare (context point))
  (interactive)
  (save-excursion
    (outline-back-to-heading)
    (outline-end-of-heading)
    (outline-flag-region (point) (progn (outline-next-preface) (point)) t)))
```

Here is a draft that works surprisingly well:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: declare-context.patch --]
[-- Type: text/x-diff, Size: 2530 bytes --]

diff --git a/lisp/outline.el b/lisp/outline.el
index e6365629197..9874e318afe 100644
--- a/lisp/outline.el
+++ b/lisp/outline.el
@@ -1067,6 +1067,7 @@ outline-isearch-open-invisible
 \f
 (defun outline-hide-entry ()
   "Hide the body directly following this heading."
+  (declare (context point))
   (interactive)
   (save-excursion
     (outline-back-to-heading)
@@ -1094,6 +1095,7 @@ 'show-entry
 (defun outline-hide-body ()
   "Hide all body lines in buffer, leaving all headings visible.
 Note that this does not hide the lines preceding the first heading line."
+  (declare (context buffer))
   (interactive)
   (outline-hide-region-body (point-min) (point-max)))
 
diff --git a/lisp/simple.el b/lisp/simple.el
index ff665111a5d..9026f0950c8 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2772,6 +2772,12 @@ command-execute
 executing a special event, so ignore the prefix argument and
 don't clear it."
   (setq debug-on-next-call nil)
+  (when (and (mouse-event-p last-nonmenu-event)
+             (not (memq (posn-area (event-start last-nonmenu-event))
+                        '(mode-line menu-bar)))
+             (symbolp this-command)
+             (memq 'point (function-get this-command 'context)))
+    (posn-set-point (event-start last-nonmenu-event)))
   (let ((prefixarg (unless special
                      ;; FIXME: This should probably be done around
                      ;; pre-command-hook rather than here!
diff --git a/lisp/emacs-lisp/byte-run.el b/lisp/emacs-lisp/byte-run.el
index a377ec395e1..24a1d11d960 100644
--- a/lisp/emacs-lisp/byte-run.el
+++ b/lisp/emacs-lisp/byte-run.el
@@ -217,6 +217,11 @@ 'byte-run--set-interactive-args
                  (cadr elem)))
               val)))))
 
+(defalias 'byte-run--set-context
+  #'(lambda (f _args &rest val)
+      (list 'function-put (list 'quote f)
+            ''context (list 'quote val))))
+
 ;; Add any new entries to info node `(elisp)Declare Form'.
 (defvar defun-declarations-alist
   (list
@@ -239,7 +244,8 @@ defun-declarations-alist
    (list 'speed #'byte-run--set-speed)
    (list 'completion #'byte-run--set-completion)
    (list 'modes #'byte-run--set-modes)
-   (list 'interactive-args #'byte-run--set-interactive-args))
+   (list 'interactive-args #'byte-run--set-interactive-args)
+   (list 'context #'byte-run--set-context))
   "List associating function properties to their macro expansion.
 Each element of the list takes the form (PROP FUN) where FUN is
 a function.  For each (PROP . VALUES) in a function's declaration,

[-- Attachment #3: Type: text/plain, Size: 154 bytes --]


PS:
More contexts could be added later, such as a lambda that defines a region.
And a property requiring to move point back after finishing the command.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Dmitry Gutov]

On 31/08/2023 05:51, Po Lu wrote:
> Dmitry Gutov<dmitry@gutov.dev>  writes:
> 
>> Sorry, I'm still not seeing your point. Especially the last paragraph
>> in my last email seems to remain unaddressed.
>>
>> And both commands take EVENT as an argument already.
>>
>> The problem is not where is searched for an identifier, but *from
>> where* it searched for the identifier's locations. And that position
>> is important only for particular Xref backends (both LSP-based ones,
>> I'm assuming).
> My point is this: xref-find-definitions itself, not its callers, should
> take EVENT, then call (goto-char (posn-point (event-start event)))
> within.

All right, but even that wouldn't save us from having to move to the 
position twice (one in the interactive form, and once in the function body).

Otherwise, I see your point and cause of frustration, but extending the 
convention to many (every?) interactive function seems difficult.

OTOH, Juri's line of thought with menu-handling code doing the move 
sounds feasible. Maybe there would be a new interactive spec for such 
commands, to choose which ones would need this specifically.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Po Lu]

Dmitry Gutov <dmitry@gutov.dev> writes:

> All right, but even that wouldn't save us from having to move to the
> position twice (one in the interactive form, and once in the function
> body).
>
> Otherwise, I see your point and cause of frustration, but extending
> the convention to many (every?) interactive function seems difficult.

Difficult but ultimately worthwhile, IMHO.  Our code is replete with
commands that would benefit from such treatment, some of which I've
already been compelled to fix.

Re: master 128ed5c9f17: Add one more mouse-set-point call to functions xref-find-*-at-mouse

[Juri Linkov]

> OTOH, Juri's line of thought with menu-handling code doing the move sounds
> feasible. Maybe there would be a new interactive spec for such commands, to
> choose which ones would need this specifically.

A new interactive spec makes sense with at least two possible values:
e.g. 'mouse-set-point' that will instruct the command to move point
to the position of the event before the command is executed, and
'save-excursion' to move point back to the original position afterwards.