bug#65578: Eglot with mouse

bug#65578: Eglot with mouse

[Juri Linkov]

X-Debbugs-CC: João Távora <joaotavora@gmail.com>

0. emacs -Q
1. enable context-menu-mode
2. enable eglot in a suitable buffer
3. move point to one identifier
4. click the right mouse button on another identifier
5. in the context menu click "Find Definition"

The problem is that a wrong identifier is visited.

Whereas 'xref-find-definitions-at-mouse' takes care
about getting the right identifier at the clicked position,
Eglot returns a fictitious identifier "LSP identifier at point"
that can't be used to find the identifier at the mouse click.

xref-find-definitions-at-mouse uses save-excursion with
mouse-set-point before calling xref-backend-identifier-at-point.
But the same save-excursion with mouse-set-point can't be added
around xref-find-definitions because save-excursion will restore
the original position after visiting the found identifier.

Also tried to add xref-find-definitions-at-mouse to the list
xref-prompt-for-identifier, no effect.

Then tried to remove '(xref--prompt-p this-command)'
from 'xref-backend-identifier-at-point ((_backend (eql eglot)))'.
This fixed xref-find-definitions-at-mouse, but still
doesn't work for xref-find-references-at-mouse.
Also removing '(xref--prompt-p this-command)' has such a strange effect
that it started to find approximate fuzzy matches that sound like
the original identifier.

bug#65578: Eglot with mouse

[João Távora]

Juri Linkov <juri@linkov.net> writes:

> X-Debbugs-CC: João Távora <joaotavora@gmail.com>
>
> 0. emacs -Q
> 1. enable context-menu-mode
> 2. enable eglot in a suitable buffer
> 3. move point to one identifier
> 4. click the right mouse button on another identifier
> 5. in the context menu click "Find Definition"
>
> The problem is that a wrong identifier is visited.
>
> Whereas 'xref-find-definitions-at-mouse' takes care
> about getting the right identifier at the clicked position,
> Eglot returns a fictitious identifier "LSP identifier at point"
> that can't be used to find the identifier at the mouse click.

I don't think the name Eglot gives to the identifier matters here.  What
matters is that the LSP request is made with the correct buffer position
(where the user clicked).

So if 'xref-find-definitions-at-mouse' already has logic to protect
that, it should work.

> xref-find-definitions-at-mouse uses save-excursion with
> mouse-set-point before calling xref-backend-identifier-at-point.
> But the same save-excursion with mouse-set-point can't be added
> around xref-find-definitions because save-excursion will restore
> the original position after visiting the found identifier.

Alas, somehow, the position obtained when mouse is in the correct spot
in the first step must be recorded somehow and survive until the second
step.  Else, with LSP, it just won't work, not in the near future at
least.  I'd say this is something to be changed in Xref.

> Also tried to add xref-find-definitions-at-mouse to the list
> xref-prompt-for-identifier, no effect.
>
> Then tried to remove '(xref--prompt-p this-command)'
> from 'xref-backend-identifier-at-point ((_backend (eql eglot)))'.
> This fixed xref-find-definitions-at-mouse, but still
> doesn't work for xref-find-references-at-mouse.
> Also removing '(xref--prompt-p this-command)' has such a strange effect
> that it started to find approximate fuzzy matches that sound like
> the original identifier.

I don't think any of this is the solution.

This is a more-or-less typical case where the workhorse function
non-mouse function called by the mouse function does too much.  An
elegant (perhaps verbose) solution would separate the
definition-requesting from the definition-visiting parts of the former,
and then the mouse function would use the two parts separately.

But in practice I think something like a variable shared set in the
interactive spec and read in the function body.  Or maybe a text
property in the dummy "LSP Identifier at point" string.  Let's see what
Dmitry has to say.

João

bug#65578: Eglot with mouse

[Dmitry Gutov]

On 28/08/2023 10:13, Juri Linkov wrote:
> xref-find-definitions-at-mouse uses save-excursion with
> mouse-set-point before calling xref-backend-identifier-at-point.
> But the same save-excursion with mouse-set-point can't be added
> around xref-find-definitions because save-excursion will restore
> the original position after visiting the found identifier.

Could we do that without additional save-excursion?

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 0666b18cba1..0c3e34fe0a5 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
             (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

bug#65578: Eglot with mouse

[Juri Linkov]

>> xref-find-definitions-at-mouse uses save-excursion with
>> mouse-set-point before calling xref-backend-identifier-at-point.
>> But the same save-excursion with mouse-set-point can't be added
>> around xref-find-definitions because save-excursion will restore
>> the original position after visiting the found identifier.
>
> Could we do that without additional save-excursion?
>
> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> index 0666b18cba1..0c3e34fe0a5 100644
> --- a/lisp/progmodes/xref.el
> +++ b/lisp/progmodes/xref.el
> @@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
>             (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"))))

The problem with this solution is that when it doesn't find
the identifier then it leaves point at the wrong place.

bug#65578: Eglot with mouse

[Dmitry Gutov]

On 28/08/2023 19:42, Juri Linkov wrote:
>>> xref-find-definitions-at-mouse uses save-excursion with
>>> mouse-set-point before calling xref-backend-identifier-at-point.
>>> But the same save-excursion with mouse-set-point can't be added
>>> around xref-find-definitions because save-excursion will restore
>>> the original position after visiting the found identifier.
>> Could we do that without additional save-excursion?
>>
>> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
>> index 0666b18cba1..0c3e34fe0a5 100644
>> --- a/lisp/progmodes/xref.el
>> +++ b/lisp/progmodes/xref.el
>> @@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
>>              (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"))))
> The problem with this solution is that when it doesn't find
> the identifier then it leaves point at the wrong place.

I'm not sure is a big problem, but we could save the original position 
and restore it in case of error too:

diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
index 0666b18cba1..fb7380dee77 100644
--- a/lisp/progmodes/xref.el
+++ b/lisp/progmodes/xref.el
@@ -1635,9 +1635,18 @@ xref-find-definitions-at-mouse
    (let ((identifier
           (save-excursion
             (mouse-set-point event)
-           (xref-backend-identifier-at-point (xref-find-backend)))))
+           (xref-backend-identifier-at-point (xref-find-backend))))
+        (buf (current-buffer))
+        (pos (point)))
      (if identifier
-        (xref-find-definitions identifier)
+        (condition-case err
+          (progn
+            (mouse-set-point event)
+            (xref-find-definitions identifier))
+          (user-error
+           (set-buffer buf)
+           (goto-char pos)
+           (user-error (message (error-message-string err)))))
        (user-error "No identifier here"))))

  ;;;###autoload

bug#65578: Eglot with mouse

[João Távora]

On Mon, Aug 28, 2023 at 6:00 PM Dmitry Gutov <dmitry@gutov.dev> wrote:
>
> On 28/08/2023 19:42, Juri Linkov wrote:
> >>> xref-find-definitions-at-mouse uses save-excursion with
> >>> mouse-set-point before calling xref-backend-identifier-at-point.
> >>> But the same save-excursion with mouse-set-point can't be added
> >>> around xref-find-definitions because save-excursion will restore
> >>> the original position after visiting the found identifier.
> >> Could we do that without additional save-excursion?
> >>
> >> diff --git a/lisp/progmodes/xref.el b/lisp/progmodes/xref.el
> >> index 0666b18cba1..0c3e34fe0a5 100644
> >> --- a/lisp/progmodes/xref.el
> >> +++ b/lisp/progmodes/xref.el
> >> @@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
> >>              (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"))))
> > The problem with this solution is that when it doesn't find
> > the identifier then it leaves point at the wrong place.
>
> I'm not sure is a big problem, but we could save the original position
> and restore it in case of error too:

Have to agree with Dmitry.  If I click a position to get a xref to
somewhere else, I'm kind of already expecting/hoping that point
moves.  So the fact that, in the error/exception case, it sticks
to the place where I right-clicked is really not so bad.  Personally,
I'd not complicate the implementation beyond what Dmitry proposed
initially.

bug#65578: Eglot with mouse

[Juri Linkov]

>>> @@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
>>> -        (xref-find-definitions identifier)
>>> +        (progn
>>> +          (mouse-set-point event)
>>> +          (xref-find-definitions identifier))
>>>         (user-error "No identifier here"))))
>> The problem with this solution is that when it doesn't find
>> the identifier then it leaves point at the wrong place.
>
> I'm not sure is a big problem, but we could save the original position and
> restore it in case of error too:

Thanks.  I tend to agree with João that your initial version would be
sufficient to leave point at the position of right-clicking.

Then the same change is also needed in xref-find-references-at-mouse
with mouse-set-point before xref-find-references.

bug#65578: Eglot with mouse

[Dmitry Gutov]

Version: 30.1

On 30/08/2023 19:35, Juri Linkov wrote:
>>>> @@ -1637,7 +1637,9 @@ xref-find-definitions-at-mouse
>>>> -        (xref-find-definitions identifier)
>>>> +        (progn
>>>> +          (mouse-set-point event)
>>>> +          (xref-find-definitions identifier))
>>>>          (user-error "No identifier here"))))
>>> The problem with this solution is that when it doesn't find
>>> the identifier then it leaves point at the wrong place.
>> I'm not sure is a big problem, but we could save the original position and
>> restore it in case of error too:
> Thanks.  I tend to agree with João that your initial version would be
> sufficient to leave point at the position of right-clicking.
> 
> Then the same change is also needed in xref-find-references-at-mouse
> with mouse-set-point before xref-find-references.

Very good, if you don't mind the side-effect too, I've pushed the fix 
for both functions to master. And closing.

BTW, this context-menu-mode is looking pretty nifty. It'd be nice to 
turn it on by default someday.