bug#62115: RFE: eglot: support window.showDocument LSP RPC

bug#62115: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Attn: Joao Tavora

The attached patch adds basic support to eglot for the
window.showDocument downcall, added in LSP 3.16, which enables the
server to request that
the client open an URL either in an external browser (e.g. as if by
the open(1) or xdg-open(1) command) or internally, in the editor.
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument

See also https://github.com/joaotavora/eglot/discussions/1180.

Thanks to @nemethf, whose unmerged PR
https://github.com/joaotavora/eglot/pull/855 provided numerous
improvements over my own first draft.

This patch can be applied to the base commit of 8ee205d. Please let me
know if you'd like it in some other form.

cheers
alan

[-- Attachment #2: eglot-showDocument.patch --]
[-- Type: application/octet-stream, Size: 3839 bytes --]


This patch adds basic support to eglot for the window.showDocument
downcall, added in LSP 3.16, which enables the server to request that
the client open an URL either in an external browser (e.g. as if by
the open(1) or xdg-open(1) command) or internally, in the editor.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument

See also https://github.com/joaotavora/eglot/discussions/1180.

Thanks to @nemethf, whose unmerged PR
https://github.com/joaotavora/eglot/pull/855 provided numerous
improvements over my own first draft.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 2f8d2002cd3..cb0bfdf603d 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -819,6 +819,7 @@ treated as in `eglot--dbind'."
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
+            :window '(:showDocument (:support t))
             :experimental eglot--{})))
 
 (cl-defgeneric eglot-workspace-folders (server)
@@ -2143,6 +2144,45 @@ COMMAND is a symbol naming the command."
   (_server (_method (eql window/logMessage)) &key _type _message)
   "Handle notification window/logMessage.") ;; noop, use events buffer
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key uri external takeFocus selection
+           &allow-other-keys)
+  "Handle a window/showDocument server->client request by opening the
+URL in a browser or within Emacs."
+  ;; Note: browse-url on a "file:" URL will execute open(1) or xdg-open(1),
+  ;; which may end up opening the file in Emacs (or some other editor that
+  ;; has registered the *.go extension), ignoring the optional selection.
+  ;; Typically servers send "External: false" for files.
+  (if (and external (not (eq external :json-false)))
+      (browse-url uri)
+    ;; Don't call find-file immediately (within the RPC handler) since
+    ;; find-file's go-mode hooks issue more LSP RPCs (e.g.
+    ;; textDocument/documentSymbol) from within this one, which then
+    ;; gets stuck. (Is that a bug in gopls?)
+    ;; So, make the call asynchronously from the idle loop.
+    ;; Of course this means we can't respond with the proper success value.
+    ;;(run-with-idle-timer 0 nil
+                         (funcall
+                         #'(lambda (filename noselect selection)
+                                   (with-current-buffer (if noselect
+                                                            (find-file-noselect filename)
+                                                          (x-focus-frame nil)
+                                                          (find-file filename))
+                                     (when selection
+                                       (save-restriction
+                                         (widen)
+                                         (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+                                           (if (equal beg end)
+                                               (goto-char beg)
+                                             (goto-char end)
+                                             (set-mark-command nil)
+                                             (goto-char beg))
+                                           (recenter))))))
+                         (eglot--uri-to-path uri) ; filename
+                         (or (null takeFocus) (eq takeFocus :json-false)) ; noselect
+                         selection))
+  '(:success t))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Apologies, that patch contained debugging stuff. Please use this one:



On Fri, 10 Mar 2023 at 10:34, Alan Donovan <adonovan@google.com> wrote:
>
> Attn: Joao Tavora
>
> The attached patch adds basic support to eglot for the
> window.showDocument downcall, added in LSP 3.16, which enables the
> server to request that
> the client open an URL either in an external browser (e.g. as if by
> the open(1) or xdg-open(1) command) or internally, in the editor.
> https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument
>
> See also https://github.com/joaotavora/eglot/discussions/1180.
>
> Thanks to @nemethf, whose unmerged PR
> https://github.com/joaotavora/eglot/pull/855 provided numerous
> improvements over my own first draft.
>
> This patch can be applied to the base commit of 8ee205d. Please let me
> know if you'd like it in some other form.
>
> cheers
> alan

[-- Attachment #2: eglot-showDocument.patch --]
[-- Type: application/octet-stream, Size: 3802 bytes --]


This patch adds basic support to eglot for the window.showDocument
downcall, added in LSP 3.16, which enables the server to request that
the client open an URL either in an external browser (e.g. as if by
the open(1) or xdg-open(1) command) or internally, in the editor.

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#window_showDocument

See also https://github.com/joaotavora/eglot/discussions/1180.

Thanks to @nemethf, whose unmerged PR
https://github.com/joaotavora/eglot/pull/855 provided numerous
improvements over my own first draft.

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 2f8d2002cd3..fb0c5cb1199 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -819,6 +819,7 @@ treated as in `eglot--dbind'."
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
+            :window '(:showDocument (:support t))
             :experimental eglot--{})))
 
 (cl-defgeneric eglot-workspace-folders (server)
@@ -2143,6 +2144,44 @@ COMMAND is a symbol naming the command."
   (_server (_method (eql window/logMessage)) &key _type _message)
   "Handle notification window/logMessage.") ;; noop, use events buffer
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key uri external takeFocus selection
+           &allow-other-keys)
+  "Handle a window/showDocument server->client request by opening the
+URL in a browser or within Emacs."
+  ;; Note: browse-url on a "file:" URL will execute open(1) or xdg-open(1),
+  ;; which may end up opening the file in Emacs (or some other editor that
+  ;; has registered the *.go extension), ignoring the optional selection.
+  ;; Typically servers send "External: false" for files.
+  (if (and external (not (eq external :json-false)))
+      (browse-url uri)
+    ;; Don't call find-file immediately (within the RPC handler) since
+    ;; find-file's go-mode hooks issue more LSP RPCs (e.g.
+    ;; textDocument/documentSymbol) from within this one, which then
+    ;; gets stuck. (Is that a bug in gopls?)
+    ;; So, make the call asynchronously from the idle loop.
+    ;; Of course this means we can't respond with the proper success value.
+    (run-with-idle-timer 0 nil
+                         #'(lambda (filename noselect selection)
+                                   (with-current-buffer (if noselect
+                                                            (find-file-noselect filename)
+                                                          (x-focus-frame nil)
+                                                          (find-file filename))
+                                     (when selection
+                                       (save-restriction
+                                         (widen)
+                                         (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+                                           (if (equal beg end)
+                                               (goto-char beg)
+                                             (goto-char end)
+                                             (set-mark-command nil)
+                                             (goto-char beg))
+                                           (recenter))))))
+                         (eglot--uri-to-path uri) ; filename
+                         (or (null takeFocus) (eq takeFocus :json-false)) ; noselect
+                         selection))
+  '(:success t))
+
 (cl-defmethod eglot-handle-notification
   (_server (_method (eql telemetry/event)) &rest _any)
   "Handle notification telemetry/event.") ;; noop, use events buffer

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Hello Alan!

> Apologies, that patch contained debugging stuff. Please use this one:

No problem.  Moderation in this bug tracker took a little bit longer
than usual, so this follow up message created a new bug (62116).  I've
closed the other one (62115).  I think future bug reports by you won't
suffer from the same problem.

>> This patch can be applied to the base commit of 8ee205d. Please let me
>> know if you'd like it in some other form.

This form is fine.  Comments below.

> diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
> index 2f8d2002cd3..fb0c5cb1199 100644
> --- a/lisp/progmodes/eglot.el
> +++ b/lisp/progmodes/eglot.el
> @@ -819,6 +819,7 @@ treated as in `eglot--dbind'."
>                                           [,@(mapcar
>                                               #'car eglot--tag-faces)])))
>              :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
> +            :window '(:showDocument (:support t))
>              :experimental eglot--{})))
>  
>  (cl-defgeneric eglot-workspace-folders (server)
> @@ -2143,6 +2144,44 @@ COMMAND is a symbol naming the command."
>    (_server (_method (eql window/logMessage)) &key _type _message)
>    "Handle notification window/logMessage.") ;; noop, use events buffer
>  
> +(cl-defmethod eglot-handle-request
> +  (_server (_method (eql window/showDocument)) &key uri external takeFocus selection
> +           &allow-other-keys)
> +  "Handle a window/showDocument server->client request by opening the
> +URL in a browser or within Emacs."
> +  ;; Note: browse-url on a "file:" URL will execute open(1) or xdg-open(1),
> +  ;; which may end up opening the file in Emacs (or some other editor that
> +  ;; has registered the *.go extension), ignoring the optional selection.

Not fully correct, as browse-url may well open things within Emacs own
web browser, eww.  I propose you leave just the next line.

> +  ;; Typically servers send "External: false" for files.

In your experience, do servers differentiate between files inside the
LSP "workspace" (in Emacs' parlance, the project) and outside it?  A
file visited by find-file inside the project becomes "managed" by
Eglot+server, where a file outside the project becomes just a buffer
(that could be managed by a different server).  Not saying this is a
problem, just interested in how this is used in the wild.

> +  (if (and external (not (eq external :json-false)))
> +      (browse-url uri)
> +    ;; Don't call find-file immediately (within the RPC handler) since
> +    ;; find-file's go-mode hooks issue more LSP RPCs (e.g.
> +    ;; textDocument/documentSymbol) from within this one, which then
> +    ;; gets stuck. (Is that a bug in gopls?)
> +    ;; So, make the call asynchronously from the idle loop.
> +    ;; Of course this means we can't respond with the proper success value.

Yep, I don't like this, and that's only one of the reasons.  I'd only do
this if there is no better alternative, and I don't think we've
exhausted them yet (even then, you probably want simply
'run-with-timer').

First, I've made some changes to Eglot's usage of find-file-hook
recently, and I'd like to ensure you're running the latest eglot from
Emacs 29 or Emacs master (there are some pretest binaries for the
latter, I think).

Then I would ask you to try to find a backtrace of that
'textDocument/documentSymbol call that presumably Eglot sends
immediately after visiting the file.  Maybe the problem can be fixed
there or thereabouts.

One way to obtain that backtrace might be to do:

   (unwind-protect
       (progn
          (debug-on-entry 'jsonrpc-request)
          ;; synchronous find-file logic
       )
     (cancel-debug-on-entry 'jsonrpc-request))

If you can, repeat the process with 'jsonrpc-notify instead of as well
which will maybe produce a different backtrace.  If it does, show that
too.

It's quite likely that the problem can be fixed upstream, so that a more
naive implementation can work.

> +    (run-with-idle-timer 0 nil
> +                         #'(lambda (filename noselect selection)

Just some notes, since we're probably not going to use a lambda here.
First #'(lambda (...) ) is redudant here.  Also, since Emacs has lexical
scoping (for about a decade now), you can have an argless lambda and
access variables from the inside

> +                                   (with-current-buffer (if noselect
> +                                                            (find-file-noselect filename)

I don't think LSP's understanding of "focus" is the same as in
'find-file-noselect'.  In Emacs, I would model it on the concept of
selected window.  IOW

  (funcall (if focus #'pop-to-buffer #'display-buffer)
           (find-file-noselect filename))

might be closer to what you want.  But only testing can tell.


> +                                                          (x-focus-frame nil)

This is going to be problematic for some users, I suspect.  Let's leave
it for now.  What is the use case?  An interactive app with a "find
definition" interface?  That's cool, the Common Lisp IDE's I use all
have this.

> +                                                          (find-file filename))
> +                                     (when selection
> +                                       (save-restriction
> +                                         (widen)

You're saving the user's restriction, widening, then restoring it, but
why?  If the intended region is outside, it will have no effect, so
perhaps just check that 'beg' and 'end' are between 'point-min' and
'point-max'.  Alternatively, just nuke the user's restriction (you're
moving lots of other stuff anyway).

> +                                         (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
> +                                           (if (equal beg end)
> +                                               (goto-char beg)
> +                                             (goto-char end)
> +                                             (set-mark-command nil)
> +                                             (goto-char beg))
> +                                           (recenter))))))

If you can show me how, I'd like to test this myself.  i have gopls
installed, and a minimal toolchain, but I'm missing a project where I
can exercise this (and probably I'm also missing the mandatory "server
configuration" options).  So a simple recipe with the smallest project
possible would be nice.

We're probably also going to need some user-confirmation UI for this.
But that can come later.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Eli Zaretskii]

> Cc: Felician Nemeth <felician.nemeth@gmail.com>, 62116@debbugs.gnu.org
> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 11 Mar 2023 12:56:32 +0000
> 
> Hello Alan!
> 
> > Apologies, that patch contained debugging stuff. Please use this one:
> 
> No problem.  Moderation in this bug tracker took a little bit longer
> than usual, so this follow up message created a new bug (62116).  I've
> closed the other one (62115).  I think future bug reports by you won't
> suffer from the same problem.

Btw, Alan doesn't seem to have a copyright assignment on file, so
before we accept this welcome contribution, we need to have the legal
paperwork to be done.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> No problem.  Moderation in this bug tracker took a little bit longer
>> than usual, so this follow up message created a new bug (62116).  I've
>> closed the other one (62115).  I think future bug reports by you won't
>> suffer from the same problem.
>
> Btw, Alan doesn't seem to have a copyright assignment on file, so
> before we accept this welcome contribution, we need to have the legal
> paperwork to be done.

Yes, and I suggest you send Alan the copyright assignment form.  It's
always best to have this.

If this somehow becomes a blocker for this specific patch, we could also
opt to first merge Felicián's patch (which precedes this one), then work
on some adjustments.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: adonovan@google.com,  felician.nemeth@gmail.com,  62116@debbugs.gnu.org
> Date: Sat, 11 Mar 2023 20:20:23 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> No problem.  Moderation in this bug tracker took a little bit longer
> >> than usual, so this follow up message created a new bug (62116).  I've
> >> closed the other one (62115).  I think future bug reports by you won't
> >> suffer from the same problem.
> >
> > Btw, Alan doesn't seem to have a copyright assignment on file, so
> > before we accept this welcome contribution, we need to have the legal
> > paperwork to be done.
> 
> Yes, and I suggest you send Alan the copyright assignment form.  It's
> always best to have this.

Done off-list.

However, maybe this is not needed, since Alan's address is
@google.com.  Alan, can you confirm that your work is covered by
Google's blanket assignment of copyright?

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

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


This is a continuation of the thread 
  https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00206.html
, where João suggested we should proceed with my simpler patch until we
hear from Alan.  This message summarizes the current situation:
  https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00339.html

I tested my patch with Sebastian's ada example.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-support-window-showDocument-request.patch --]
[-- Type: text/x-diff, Size: 2207 bytes --]

From 468a8104183ba46f92d0a194862761c81943c505 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Sat, 22 Apr 2023 10:49:17 +0200
Subject: [PATCH] Eglot: support window/showDocument request

* eglot.el (eglot-client-capabilities): Add showDocument support.
(eglot-handle-request window/showDocument): New cl-defmethod.
---
 lisp/progmodes/eglot.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3134d55fc0..daabb4ff7b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -830,7 +830,8 @@ eglot-client-capabilities
                                        `(:valueSet
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
-            :window `(:workDoneProgress t)
+            :window `(:showDocument (:support t)
+                      :workDoneProgress t)
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
             :experimental eglot--{})))
 
@@ -2345,6 +2346,28 @@ eglot-handle-request
   "Handle server request workspace/workspaceFolders."
   (eglot-workspace-folders server))
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key
+           uri external takeFocus selection)
+  "Handle request window/showDocument."
+  (if (eq external t)
+      (browse-url uri)
+    (let ((filename (eglot--uri-to-path uri)))
+      (if (eq takeFocus t)
+          (find-file filename)
+        (find-file-noselect filename))
+      (when selection
+        (with-current-buffer (get-file-buffer filename)
+          (save-restriction
+            (widen)
+            (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+              (if (equal beg end)
+                  (goto-char beg)
+                (goto-char end)
+                (set-mark-command nil)
+                (goto-char beg))))))))
+  '(:success t))
+
 (defun eglot--TextDocumentIdentifier ()
   "Compute TextDocumentIdentifier object for current buffer."
   `(:uri ,(eglot--path-to-uri (or buffer-file-name
-- 
2.30.2

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Eli Zaretskii]

> From: Felician Nemeth <felician.nemeth@gmail.com>
> Cc: João Távora <joaotavora@gmail.com>,
>   adonovan@google.com, Eli Zaretskii
>  <eliz@gnu.org>, Sebastian Poeplau <sebastian.poeplau@mailbox.org>
> Date: Sat, 22 Apr 2023 11:08:05 +0200
> 
> This is a continuation of the thread 
>   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00206.html
> , where João suggested we should proceed with my simpler patch until we
> hear from Alan.  This message summarizes the current situation:
>   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00339.html
> 
> I tested my patch with Sebastian's ada example.

João, any comments?  This discussion seems to have stalled.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

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

Sorry, I've been busier lately. I'm still very much interested in this
feature, but i would like to test it myself, and ideally in more than one
server.

João

On Fri, May 5, 2023, 07:02 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Felician Nemeth <felician.nemeth@gmail.com>
> > Cc: João Távora <joaotavora@gmail.com>,
> >   adonovan@google.com, Eli Zaretskii
> >  <eliz@gnu.org>, Sebastian Poeplau <sebastian.poeplau@mailbox.org>
> > Date: Sat, 22 Apr 2023 11:08:05 +0200
> >
> > This is a continuation of the thread
> >   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00206.html
> > , where João suggested we should proceed with my simpler patch until we
> > hear from Alan.  This message summarizes the current situation:
> >   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00339.html
> >
> > I tested my patch with Sebastian's ada example.
>
> João, any comments?  This discussion seems to have stalled.
>

[-- Attachment #2: Type: text/html, Size: 2007 bytes --]

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Felician Nemeth <felician.nemeth@gmail.com>
>> Cc: João Távora <joaotavora@gmail.com>,
>>   adonovan@google.com, Eli Zaretskii
>>  <eliz@gnu.org>, Sebastian Poeplau <sebastian.poeplau@mailbox.org>
>> Date: Sat, 22 Apr 2023 11:08:05 +0200
>> 
>> This is a continuation of the thread 
>>   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00206.html
>> , where João suggested we should proceed with my simpler patch until we
>> hear from Alan.  This message summarizes the current situation:
>>   https://lists.gnu.org/archive/html/emacs-devel/2023-04/msg00339.html
>> 
>> I tested my patch with Sebastian's ada example.

Felicián, can you show your full patch again, perhaps with a unit test
using the ada language server?

Also, can you comment on why you think Alan Donovan's patch has that
non-synchronous find-file mechanism?  Since Alan, doesn't reply, what do
you conjecture is the reason his patch goes through this trouble?

I'd also like someone to address my comments of that patch, which is
similar to yours:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62116#10.  Among others

1. if you think the Emacs frame should be raised and/or focused when a
   window.showDocument comes in.

2. if browse-url should be used for non-file: urls (it might open a
   browser window outside of Emacs, which IMO is fine.)

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Fri, 5 May 2023 at 12:48, João Távora <joaotavora@gmail.com> wrote:
> Also, can you comment on why you think Alan Donovan's patch has that
> non-synchronous find-file mechanism?  Since Alan, doesn't reply, what do
> you conjecture is the reason his patch goes through this trouble?

Hi Joao, sorry for the long radio silence. I've been meaning to return
to this but we're in a release crunch lately.

The reason for the async approach is to avoid nested RPCs between the
client and gopls (Go LSP) server. The server is effectively
single-threaded (the underlying JSON RPC implementation is full duplex
but gopls' handlers use a lock), so if a server RPC initiates a
showDocument downcall, which causes the client to open a file, then
the find-file hooks for that file will make another RPC to the server,
which causes it to get stuck. Arguably this is a problem in the design
of gopls, but I doubt it is easy to fix, and I wonder how many other
LSP servers have ended up taking the same approach. An alternative
solution would be for gopls never to make a synchronous showDocument
downcall, but instead to fire it off in another thread.

> 1. if you think the Emacs frame should be raised and/or focused when a window.showDocument comes in.

I think this is appropriate, since there's otherwise a good chance
that nothing would appear to happen in response to (e.g.) a click. But
perhaps we can play with it and get some experience. It may warrant a
configuration option, but let's try to avoid that if possible.

> 2. if browse-url should be used for non-file: urls (it might open a browser window outside of Emacs, which IMO is fine.)

What's the alternative? The very purpose of the operation is to open a
URL in a browser.

cheers
alan

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Alan Donovan <adonovan@google.com> writes:

> Hi Joao, sorry for the long radio silence. I've been meaning to return
> to this but we're in a release crunch lately.

Thanks for replying back.

> The reason for the async approach is to avoid nested RPCs between the
> client and gopls (Go LSP) server. The server is effectively
> single-threaded (the underlying JSON RPC implementation is full duplex
> but gopls' handlers use a lock), so if a server RPC initiates a
> showDocument downcall, which causes the client to open a file, then
> the find-file hooks for that file will make another RPC to the server,
> which causes it to get stuck. Arguably this is a problem in the design
> of gopls, but I doubt it is easy to fix, and I wonder how many other
> LSP servers have ended up taking the same approach. An alternative
> solution would be for gopls never to make a synchronous showDocument
> downcall, but instead to fire it off in another thread.

Yes, but I don't really think this is a design problem with gopls.  Or
at least I don't hink Eglot should not make these nested calls in the
first place.  So I would like to understand how this nested RPC requests
takes place.  From my reading of the eglot.el's code, there aren't any -- that
is, with Emacs -Q -- at least.

So I would like to reproduce this situation to analyse what can be done.

I have gopls on my archlinux machine, though I seldom program in Go.
How does one go about setting up gopls in terms of workspace
configuration, command line options, etc so that the feature is
exercised?  Also, can I somehow make use of this feature with any small
hello world project or do I need something bigger?  In that case can you
point me to a git repository that I can download, or attach that
project?

>> 1. if you think the Emacs frame should be raised and/or focused when a window.showDocument comes in.
>
> I think this is appropriate, since there's otherwise a good chance
> that nothing would appear to happen in response to (e.g.) a click. But
> perhaps we can play with it and get some experience. It may warrant a
> configuration option, but let's try to avoid that if possible.

Agree 100%.

My main curiosity is where does this "click" happen in the first
place...  (that's related to my lack of hands-on experience with this
feature, as described above)

>> 2. if browse-url should be used for non-file: urls (it might open a browser window outside of Emacs, which IMO is fine.)
>
> What's the alternative? The very purpose of the operation is to open a
> URL in a browser.

Not always, IIUC.  Sometimes it's just to open a file.  Also Emacs has a
web browser built-in.  We could theoretically force it to use this web
browser (though I don't think we should).

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Fri, 5 May 2023 at 13:17, João Távora <joaotavora@gmail.com> wrote:
> Yes, but I don't really think this is a design problem with gopls.  Or
> at least I don't think Eglot should not make these nested calls in the
> first place.  So I would like to understand how this nested RPC requests
> takes place.  From my reading of the eglot.el's code, there aren't any -- that
> is, with Emacs -Q -- at least.
>
> So I would like to reproduce this situation to analyse what can be done.
>
> I have gopls on my archlinux machine, though I seldom program in Go.
> How does one go about setting up gopls in terms of workspace
> configuration, command line options, etc so that the feature is
> exercised?  Also, can I somehow make use of this feature with any small
> hello world project or do I need something bigger?  In that case can you
> point me to a git repository that I can download, or attach that
> project?

Sure. You can reproduce the experimental setup like so:

1. git clone golang.org/x/tools

2. Download and checkout this commit:
https://go-review.git.corp.google.com/c/tools/+/474735
(It's a stack of 2 messy CLs on top of an old master, but it works.)

3. (cd gopls && go install)
This puts gopls on your $PATH.

4. killall gopls
So that eglot uses the new executable.

5. Load the definition of the eglot-handle-request window/showDocument handler.

6. Also load this function.
(defun gopls--package-docs ()
  "Open the documentation for this Go package in a browser."
  (interactive)
  (save-excursion
    ;; TODO(adonovan): We currently cram this functionality into the
    ;; textDocument/definition RPC applied to the "package" keyword.
    ;; Do something more principled.
    (goto-char 0)
    (re-search-forward "^package ")
    (beginning-of-line)
    (xref-find-definitions 'package)))

7. Visit a Go file, e.g. go/analysis/analysis.go. Move cursor to the
"package" keyword.

8. Run the gopls--package-docs command. This sends an "definitions"
RPC request for the package keyword. The server has been modified to
interpret this as a (strange) request to open a web page of package
documentation. The server sends a showDocument request
(http://localhost...) to the client. The client handler
eglot-handle-request opens a browser. You should see a page full of
links to source declarations.

9. Click on one of these links. This causes the web server (gopls) to
send another showDocument request, this time for file://..., to the
client, which causes gopls to open the file containing the declaration
and move to the correct position.

Removing the asynchrony causes the file:// downcall to get stuck
because it makes an upcall to gopls while the server is still waiting
for the showDocument downcall to finish.





> My main curiosity is where does this "click" happen in the first
> place...  (that's related to my lack of hands-on experience with this
> feature, as described above)

See above.

> >> 2. if browse-url should be used for non-file: urls (it might open a browser window outside of Emacs, which IMO is fine.)
> >
> > What's the alternative? The very purpose of the operation is to open a
> > URL in a browser.
>
> Not always, IIUC.  Sometimes it's just to open a file.

I think "file:" URLs should be opened by find-file in Emacs, and all
other URLs should go to browse-url (and thence Chrome, etc).

cheers
alan

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> 2. Download and checkout this commit:
> https://go-review.git.corp.google.com/c/tools/+/474735

Sorry, that's https://go.dev/cl/474735 for folks outside our network.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Alan Donovan <adonovan@google.com> writes:

> On Fri, 5 May 2023 at 13:17, João Távora <joaotavora@gmail.com> wrote:
>> Yes, but I don't really think this is a design problem with gopls.  Or
>> at least I don't think Eglot should not make these nested calls in the
>> first place.  So I would like to understand how this nested RPC requests
>> takes place.  From my reading of the eglot.el's code, there aren't any -- that
>> is, with Emacs -Q -- at least.
>>
>> So I would like to reproduce this situation to analyse what can be done.
>>
>> I have gopls on my archlinux machine, though I seldom program in Go.
>> How does one go about setting up gopls in terms of workspace
>> configuration, command line options, etc so that the feature is
>> exercised?  Also, can I somehow make use of this feature with any small
>> hello world project or do I need something bigger?  In that case can you
>> point me to a git repository that I can download, or attach that
>> project?
>
> Sure. You can reproduce the experimental setup like so:

Hi Alan,

I've now reproduced your setup more or less (though I had to C-u M-x
eglot and point it to ~/.golang/bin/gopls instead).

Instead of your original patch for window/showDocument, I used Feliciáns
simpler alternative, and as I predicted, I didn't run into any hangs.

When clicking links of the ephemeral web server spun by gopls, it
predictably sends a window/showMessage request to Eglot like for example
here.

  (:jsonrpc "2.0" :method "window/showDocument" :params
            (:uri "file:///home/capitaomorte/Source/Go/tools/go/analysis/validate.go" :takeFocus t :selection
                  (:start
                   (:line 21 :character 5)
                   :end
                   (:line 21 :character 5)))
            :id 15)

This causes Eglot to visit validate.go and shortly after a notification
is sent Eglot -> gopls

  (:jsonrpc "2.0" :method "textDocument/didOpen" :params
            (:textDocument

So there must be something more happening in your Emacs setup.  If some
kind of asynchronous behaviour is required, this recipe doesn't
demonstrate it.  I invite you to fine-tune your recipe and to start
Emacs the -Q flag so that we can rule out interference from other
packages or bits of configuration.

In the meantime, I think we should go ahead with Feliciáns patch.  We
can always adjust it later.

João

PS: I did find a bug in Eglot, I think when trying your experiment, but
it is unrelated to the matter at hand.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

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

> Felicián, can you show your full patch again, perhaps with a unit test
> using the ada language server?

I've attached the patch.

However, I failed to write a unit test.  At the end of
eglot-handle-request buffer-name shows the expected main.adb, but at the
end of the my ert-deftest it is the old, incorrect value.  This puzzles
my, because if I interactively repeat the test steps, everything is OK.
I copy the test code at the end of the email.

> Also, can you comment on why you think Alan Donovan's patch has that
> non-synchronous find-file mechanism?  Since Alan, doesn't reply, what do
> you conjecture is the reason his patch goes through this trouble?
>
> I'd also like someone to address my comments of that patch, which is
> similar to yours:
> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62116#10.  Among others
>
> 1. if you think the Emacs frame should be raised and/or focused when a
>    window.showDocument comes in.

I always use Emacs in single frame, so I have no experience in this
area, but I don't understand why x-focus-frame is necessary.  The
showDocument request should not come out of the blue, and if the user is
actively working on a source file, the frame is already in focus.

> 2. if browse-url should be used for non-file: urls (it might open a
>    browser window outside of Emacs, which IMO is fine.)

My original patch used browse-url as well.  browse-url is highly
customizable, and I think it is right thing to use here.  Users can
customize it to relay the URL to eww, firefox, etc even for specific
URLs.

If I understand it correctly, you argued that if takeFocus is false,
Eglot should still show the requested file, but the point should remain
were it was.  I think you're right, but I haven't updated my patch yet.

Alan's patch also contains a recenter call, which I think is unnecessary
and would probably annoy me, when the target point was already visible
before the showDocument request.

On the other hand, if the showDocument request contains a selection,
both patches could be enhanced to better visualize this selection.

Because of the timers, Alan's eglot-handle-request returns success even
if the inner lambda fails.  It should probably check if filename exists
before calling run-with-idle-timer.

However, I don't like approach of using timers.  I think a better
alterative is to put a modified lambda function in an event queue, and
Eglot should process the queue after it sent its reply to the client
request.

Finally, I think Alan's copyright status is still unclear.

And this is the my failed attempt at writing a test:

(ert-deftest eglot-test-window/showDocument ()
  "Test handling a window/showDocument server request."
  (skip-unless (executable-find "ada_language_server"))
  (let (server)
    (eglot--with-fixture
     '(("project" .
        (("project.gpr" .
          "Project P is\n   for Main use (\"main.adb\");\nend P;")
         ("main.ads" . "procedure Main;")
         ("main.adb" . "procedure Main is null;"))))
     (with-current-buffer
         (eglot--find-file-noselect "project/main.ads")
       (eglot--sniffing (:server-requests s-requests
                         :server-replies s-replies
                         :client-requests c-requests
                         :client-replies c-replies)
         (should (setq server (eglot--tests-connect)))
         (eglot-execute-command server "als-other-file"
                                (vector (eglot--TextDocumentIdentifier)))
         (let (cmd-id show-doc-id)
           (eglot--wait-for (c-requests 2)
               (&key id method &allow-other-keys)
               (setq cmd-id id)
               (string= method "workspace/executeCommand"))
           (eglot--wait-for (s-requests 2)
               (&key id method &allow-other-keys)
             (setq show-doc-id id)
             (string= method "window/showDocument"))
           (eglot--wait-for (c-replies 1)
               (&key id error &allow-other-keys)
               (and (eq id show-doc-id) (null error)))
           (eglot--wait-for (s-replies 1)
               (&key id &allow-other-keys)
               (eq id cmd-id))))
         (should (string-equal (buffer-name) "main.adb"))))))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-support-window-showDocument-request.patch --]
[-- Type: text/x-diff, Size: 2207 bytes --]

From 468a8104183ba46f92d0a194862761c81943c505 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Sat, 22 Apr 2023 10:49:17 +0200
Subject: [PATCH] Eglot: support window/showDocument request

* eglot.el (eglot-client-capabilities): Add showDocument support.
(eglot-handle-request window/showDocument): New cl-defmethod.
---
 lisp/progmodes/eglot.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3134d55fc0..daabb4ff7b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -830,7 +830,8 @@ eglot-client-capabilities
                                        `(:valueSet
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
-            :window `(:workDoneProgress t)
+            :window `(:showDocument (:support t)
+                      :workDoneProgress t)
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
             :experimental eglot--{})))
 
@@ -2345,6 +2346,28 @@ eglot-handle-request
   "Handle server request workspace/workspaceFolders."
   (eglot-workspace-folders server))
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key
+           uri external takeFocus selection)
+  "Handle request window/showDocument."
+  (if (eq external t)
+      (browse-url uri)
+    (let ((filename (eglot--uri-to-path uri)))
+      (if (eq takeFocus t)
+          (find-file filename)
+        (find-file-noselect filename))
+      (when selection
+        (with-current-buffer (get-file-buffer filename)
+          (save-restriction
+            (widen)
+            (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+              (if (equal beg end)
+                  (goto-char beg)
+                (goto-char end)
+                (set-mark-command nil)
+                (goto-char beg))))))))
+  '(:success t))
+
 (defun eglot--TextDocumentIdentifier ()
   "Compute TextDocumentIdentifier object for current buffer."
   `(:uri ,(eglot--path-to-uri (or buffer-file-name
-- 
2.30.2

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

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

Felician Nemeth <felician.nemeth@gmail.com> writes:

> If I understand it correctly, you argued that if takeFocus is false,
> Eglot should still show the requested file, but the point should remain
> were it was.  I think you're right, but I haven't updated my patch yet.

It is enough to call display-buffer.  I've attached the new patch and a
simple test that can be run with "emacs -Q -l test.el".

Since it advances the status quo, can this be merged?  If not, how
should we proceed?

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Eglot-support-window-showDocument-request.patch --]
[-- Type: text/x-diff, Size: 2224 bytes --]

From 468a8104183ba46f92d0a194862761c81943c505 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Sat, 22 Apr 2023 10:49:17 +0200
Subject: [PATCH] Eglot: support window/showDocument request

* eglot.el (eglot-client-capabilities): Add showDocument support.
(eglot-handle-request window/showDocument): New cl-defmethod.
---
 lisp/progmodes/eglot.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 3134d55fc0..daabb4ff7b 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -830,7 +830,8 @@ eglot-client-capabilities
                                        `(:valueSet
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
-            :window `(:workDoneProgress t)
+            :window `(:showDocument (:support t)
+                      :workDoneProgress t)
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
             :experimental eglot--{})))
 
@@ -2345,6 +2346,28 @@ eglot-handle-request
   "Handle server request workspace/workspaceFolders."
   (eglot-workspace-folders server))
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key
+           uri external takeFocus selection)
+  "Handle request window/showDocument."
+  (if (eq external t)
+      (browse-url uri)
+    (let ((filename (eglot--uri-to-path uri)))
+      (if (eq takeFocus t)
+          (find-file filename)
+        (display-buffer (find-file-noselect filename)))
+      (when selection
+        (with-current-buffer (get-file-buffer filename)
+          (save-restriction
+            (widen)
+            (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+              (if (equal beg end)
+                  (goto-char beg)
+                (goto-char end)
+                (set-mark-command nil)
+                (goto-char beg))))))))
+  '(:success t))
+
 (defun eglot--TextDocumentIdentifier ()
   "Compute TextDocumentIdentifier object for current buffer."
   `(:uri ,(eglot--path-to-uri (or buffer-file-name
-- 
2.30.2


[-- Attachment #3: test-showDucument-ada.tgz --]
[-- Type: application/x-gtar-compressed, Size: 704 bytes --]

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

On Mon, May 8, 2023 at 2:23 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
> > If I understand it correctly, you argued that if takeFocus is false,
> > Eglot should still show the requested file, but the point should remain
> > were it was.  I think you're right, but I haven't updated my patch yet.
>
> It is enough to call display-buffer.  I've attached the new patch and a
> simple test that can be run with "emacs -Q -l test.el".
>
> Since it advances the status quo, can this be merged?  If not, how
> should we proceed?

Haven't looked at it, but I'd say yes.  However, Alan Donovan
has replied recently, with a recipe showcasing his particular
use case.  I dont' have time to try it right now, but if you
could try his recipe/use case with your solution, it would be
great.

One note that I didn't yet reply to is that IMO (likely in Alan's
opinion too), it does make sense to raise the frame.  The
showDocument may indeed come from somewhere else which is not
Emacs at all.  I used to work with Common Lisp IDEs like that
allowed the application being developed to issue requests to
the editor and pop it up to the user.  It was extremely convenient.


João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

João Távora <joaotavora@gmail.com> writes:

>> Since it advances the status quo, can this be merged?  If not, how
>> should we proceed?
>
> Haven't looked at it, but I'd say yes.  However, Alan Donovan
> has replied recently, with a recipe showcasing his particular
> use case.  I dont' have time to try it right now, but if you
> could try his recipe/use case with your solution, it would be
> great.

It requires to patch and recompile gopls, which would be too much effort
for me, unfortunately.

> One note that I didn't yet reply to is that IMO (likely in Alan's
> opinion too), it does make sense to raise the frame.  The
> showDocument may indeed come from somewhere else which is not
> Emacs at all.  I used to work with Common Lisp IDEs like that
> allowed the application being developed to issue requests to
> the editor and pop it up to the user.  It was extremely convenient.

I've looked into this.  The docstring of x-focus-frame says "If there is
no window system support, this function does nothing."  However, this
command

    emacs -Q -nw --eval "(x-focus-frame nil)"

errors out instead of doing nothing.  But supporting the frame raising
feature is approximately as simple as

    (let ((frame (window-frame (get-buffer-window buf t))))
      (raise-frame frame)
      (x-focus-frame frame))

However, I don't work with multiple frames, so I don't know whether the
all-frames parameter of the get-buffer-window should really be t.  I
hope the latest patch can be merged as is, and someone with more
experience with multiple frames can later step in and provide an
additional patch for this extra feature.

Thanks.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

On Tue, 9 May 2023 at 13:03, Felician Nemeth <felician.nemeth@gmail.com> wrote:
> It requires to patch and recompile gopls,

FWIW, the commands to do that are:

git clone golang.org/x/tools
cd tools
git fetch https://go.googlesource.com/tools refs/changes/35/474735/3
&& git cherry-pick FETCH_HEAD
cd gopls
go install

Hope that helps.

cheers
alan

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Alan Donovan <adonovan@google.com> writes:

> On Tue, 9 May 2023 at 13:03, Felician Nemeth <felician.nemeth@gmail.com> wrote:
>> It requires to patch and recompile gopls,
>
> FWIW, the commands to do that are:
>
> git clone golang.org/x/tools
> cd tools
> git fetch https://go.googlesource.com/tools refs/changes/35/474735/3
> && git cherry-pick FETCH_HEAD

FTR, the first command doesn't work, not with my Git at least.  If I add
a https:// in front, then the remote will tell me to fetch from
go.googlesource.com instead.  If I do that and try the cherry-pick, I
get conflicts.

So I'm just using `git checkout FETCH_HEAD` instead and hoping it will
work.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

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

Felician Nemeth <felician.nemeth@gmail.com> writes:

> João Távora <joaotavora@gmail.com> writes:
>
>>> Since it advances the status quo, can this be merged?  If not, how
>>> should we proceed?
>>
>> Haven't looked at it, but I'd say yes.  However, Alan Donovan
>> has replied recently, with a recipe showcasing his particular
>> use case.  I dont' have time to try it right now, but if you
>> could try his recipe/use case with your solution, it would be
>> great.
>
> It requires to patch and recompile gopls, which would be too much effort
> for me, unfortunately.

I've now followed Alan's recipe and played around a bit with this.
Starting from your version, I came up with this simpler patch.

I was about to push it, but let's hear your opinions first (though we
can always push and tweak it later).

João


[-- Attachment #2: 0001-Eglot-support-window-showRequest.patch --]
[-- Type: text/x-patch, Size: 2612 bytes --]

From 1acbbc188a7c43063f9d6a4d25e3d854052cc956 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Felici=C3=A1n=20N=C3=A9meth?= <felician.nemeth@gmail.com>
Date: Fri, 12 May 2023 01:50:05 +0100
Subject: [PATCH] Eglot: support window/showRequest (bug#62116)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: João Távora <joaotavora@gmail.com>

* lisp/progmodes/eglot.el (eglot-client-capabilities): Advertise
window/showDocument.
(eglot-handle-request window/showDocument): New handler.
---
 lisp/progmodes/eglot.el | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 66d893a14b5..291ae9b83d9 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -845,7 +845,8 @@ eglot-client-capabilities
                                        `(:valueSet
                                          [,@(mapcar
                                              #'car eglot--tag-faces)])))
-            :window `(:workDoneProgress t)
+            :window `(:showDocument (:support t)
+                      :workDoneProgress t)
             :general (list :positionEncodings ["utf-32" "utf-8" "utf-16"])
             :experimental eglot--{})))
 
@@ -2366,6 +2367,27 @@ eglot-handle-request
   "Handle server request workspace/workspaceFolders."
   (eglot-workspace-folders server))
 
+(cl-defmethod eglot-handle-request
+  (_server (_method (eql window/showDocument)) &key
+           uri external takeFocus selection)
+  "Handle request window/showDocument."
+  (if (eq external t) (browse-url uri)
+    (let* (;; requests run with a let-bound `eglot--cached-server',
+           ;; but when finding files from handlers, this fools
+           ;; `eglot--maybe-activate-editing-mode'.
+           (eglot--cached-server nil))
+      (with-current-buffer (find-file-noselect (eglot--uri-to-path uri))
+        (cond (takeFocus
+               (pop-to-buffer (current-buffer))
+               (select-frame-set-input-focus (selected-frame)))
+              ((display-buffer (current-buffer))))
+        (when selection
+          (eglot--widening
+           (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
+             (goto-char beg)
+             (pulse-momentary-highlight-region beg end 'highlight)))))))
+  '(:success t))
+
 (defun eglot--TextDocumentIdentifier ()
   "Compute TextDocumentIdentifier object for current buffer."
   `(:uri ,(eglot--path-to-uri (or buffer-file-name
-- 
2.39.2

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks Joao,

I quickly tried this patch, and found two problems.

The first is that, because eglot-widening uses save-excursion, it
doesn't leave the cursor in the correct position; it merely jumps
there for a moment and then goes back to wherever it was before.
Removing the eglot-widening is an effective workaround.

The second issue is that Emacs still often gets stuck making a
recursive RPC, as previously discussed. I interrupted it using
toggle-debug-on-quit and recorded the emacs Lisp function call stack.
I've lightly tidied it to omit arguments and non-function sexprs:

  accept-process-output
  jsonrpc-request textDocument/documentSymbol
  eglot-imenu
  run-hooks(after-change-major-mode-hook)
  run-mode-hooks(go-mode-hook)
  go-mode()
  set-auto-mode-0(go-mode nil)
  set-auto-mode--apply-alist
  set-auto-mode
  normal-mode
  after-find-file
  find-file-noselect-1 foo.go
  find-file-noselect foo.go
  eglot-handle-request window/showDocument
  jsonrpc-connection-receive window/showDocument
  jsonrpc--process-filter

In short the handling of the showDocument downcall causes eglot to
find-file a new Go source file, whose go-mode hooks cause a
documentSymbol upcall to be sent to the server, which then blocks
indefinitely as it is still in the  middle of whatever active request
sent the showDocument downcall.

I hope this was helpful.

cheers
alan




On Thu, 11 May 2023 at 20:51, João Távora <joaotavora@gmail.com> wrote:
>
> Felician Nemeth <felician.nemeth@gmail.com> writes:
>
> > João Távora <joaotavora@gmail.com> writes:
> >
> >>> Since it advances the status quo, can this be merged?  If not, how
> >>> should we proceed?
> >>
> >> Haven't looked at it, but I'd say yes.  However, Alan Donovan
> >> has replied recently, with a recipe showcasing his particular
> >> use case.  I dont' have time to try it right now, but if you
> >> could try his recipe/use case with your solution, it would be
> >> great.
> >
> > It requires to patch and recompile gopls, which would be too much effort
> > for me, unfortunately.
>
> I've now followed Alan's recipe and played around a bit with this.
> Starting from your version, I came up with this simpler patch.
>
> I was about to push it, but let's hear your opinions first (though we
> can always push and tweak it later).
>
> João
>

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

On Fri, May 12, 2023 at 9:46 PM Alan Donovan <adonovan@google.com> wrote:
>
> Thanks Joao,
>
> I quickly tried this patch, and found two problems.
>
> The first is that, because eglot-widening uses save-excursion, it
> doesn't leave the cursor in the correct position; it merely jumps
> there for a moment and then goes back to wherever it was before.
> Removing the eglot-widening is an effective workaround.

OK, I'll have a look.

> The second issue is that Emacs still often gets stuck making a
> recursive RPC, as previously discussed. I interrupted it using
> toggle-debug-on-quit and recorded the emacs Lisp function call stack.
> I've lightly tidied it to omit arguments and non-function sexprs:
>
>   accept-process-output
>   jsonrpc-request textDocument/documentSymbol
>   eglot-imenu
>   run-hooks(after-change-major-mode-hook)
>   run-mode-hooks(go-mode-hook)
>   go-mode()
>   set-auto-mode-0(go-mode nil)
>   set-auto-mode--apply-alist
>   set-auto-mode
>   normal-mode
>   after-find-file
>   find-file-noselect-1 foo.go
>   find-file-noselect foo.go
>   eglot-handle-request window/showDocument
>   jsonrpc-connection-receive window/showDocument
>   jsonrpc--process-filter
>
> In short the handling of the showDocument downcall causes eglot to
> find-file a new Go source file, whose go-mode hooks cause a
> documentSymbol upcall to be sent to the server, which then blocks
> indefinitely as it is still in the  middle of whatever active request
> sent the showDocument downcall.
>
> I hope this was helpful.

It was, at least to confirm that you do have eglot-imenu
in your go-mode-hook somehow.  I say "somehow" because it's still
not clear why eglot-imenu is in there. I've asked you this already
in the past, but maybe I wasn't clear.  Can you try to reproduce
these problems with a controlled experiment from Emacs -Q? That
means "Emacs without any customizations".

Also not immediately clear is if you tried the patch
I attached earlier or if you tried the most recent
code I ended up pushing to the emacs master branch.
From your backtrace, I'm fairly sure you tried the
former.  Please try the latter and report back (and
if possible try it from a non-user-configuration
Emacs -Q run).

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

João Távora <joaotavora@gmail.com> writes:

> I've now followed Alan's recipe and played around a bit with this.
> Starting from your version, I came up with this simpler patch.
>
> I was about to push it, but let's hear your opinions first

It is great, and I'd be happy if it went in as is.  Thank you.

However, ...

> +(cl-defmethod eglot-handle-request
> +  (_server (_method (eql window/showDocument)) &key
> +           uri external takeFocus selection)
> +  "Handle request window/showDocument."
> +  (if (eq external t) (browse-url uri)
> +    (let* (;; requests run with a let-bound `eglot--cached-server',
> +           ;; but when finding files from handlers, this fools
> +           ;; `eglot--maybe-activate-editing-mode'.
> +           (eglot--cached-server nil))

(I don't understand this part.)

> +      (with-current-buffer (find-file-noselect (eglot--uri-to-path uri))
> +        (cond (takeFocus
> +               (pop-to-buffer (current-buffer))

Somehow switch-to-buffer feels more natural to me.  But pop-to-buffer is
probably a better choice.

> +               (select-frame-set-input-focus (selected-frame)))
> +              ((display-buffer (current-buffer))))

Even when takeFous is nil, shouldn't we still at least raise the frame?
Otherwise Emacs might not show the document.

> +        (when selection
> +          (eglot--widening
> +           (pcase-let ((`(,beg . ,end) (eglot--range-region selection)))
> +             (goto-char beg)
> +             (pulse-momentary-highlight-region beg end 'highlight)))))))

This is a nice detail.

> +  '(:success t))

Regarding Alan's problem, one could argue that it can also be fixed on
the server-side.  However, there might be a general, but complex fix on
Emacs' side as well: imenu can be modified to asynchronously generate
its index when it just wants to show it in the menu (and not to users'
direct request).  Then the request of :textDocument/documentSymbol can
be non-blocking as well.  A simpler fix is to delay the opening of the
file.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

Felician Nemeth <felician.nemeth@gmail.com> writes:

>> +           uri external takeFocus selection)
>> +  "Handle request window/showDocument."
>> +  (if (eq external t) (browse-url uri)
>> +    (let* (;; requests run with a let-bound `eglot--cached-server',
>> +           ;; but when finding files from handlers, this fools
>> +           ;; `eglot--maybe-activate-editing-mode'.
>> +           (eglot--cached-server nil))
>
> (I don't understand this part.)

If you look up to near line 1308, you'll notice that the client handlers
for server requests run with `eglot--cached-server` let-bound.  This, in
turn makes every subsequent call to `eglot--maybe-a-e-m` activate the
eglot--managed-mode minor mode.  There are, unexpectedly, a couple of
calls to `eglot--maybe-a-e-m` but that's due to find-file's logic which
surprisingly temporarily activates another major mode when finding a
file.  Apparently this is how it's always been.  That first mode is
activated by a call to `major-mode` and then is replaced by a call to
the actual major-mode function, in this case `go-mode`.

>> +      (with-current-buffer (find-file-noselect (eglot--uri-to-path uri))
>> +        (cond (takeFocus
>> +               (pop-to-buffer (current-buffer))
>
> Somehow switch-to-buffer feels more natural to me.  But pop-to-buffer is
> probably a better choice.

I think you can do `display-buffer-alist` things to control it.

>> +               (select-frame-set-input-focus (selected-frame)))
>> +              ((display-buffer (current-buffer))))
>
> Even when takeFous is nil, shouldn't we still at least raise the frame?
> Otherwise Emacs might not show the document.

I don't understand.  Can you show this problem. , I think display-buffer
ensures the buffer is displayed.  It doesn't guarantee that it is
visible in the context of the windowing system, but that's another
matter.

>
>> +  '(:success t))
>
> Regarding Alan's problem, one could argue that it can also be fixed on
> the server-side.  However, there might be a general, but complex fix on
> Emacs' side as well: imenu can be modified to asynchronously generate
> its index when it just wants to show it in the menu (and not to users'
> direct request).  Then the request of :textDocument/documentSymbol can
> be non-blocking as well.  A simpler fix is to delay the opening of the
> file.

Yes, right.  Making imenu or textDocument/documentSymbol asynchronous
would be too complex and async doesn't really fit in there.  I think
Alan's problem indeed arises with something like imenu being in the
major-mode's hook, perhaps on behalf of which-func-mode.  But that was
never confirmed.  In the latest version of the patch I've just put a
run-with-timer in the Eglot request handler.  It should solve the
conjectured Alan problem and also doesn't require the obscure
eglot--cached-server hack above.

>> I was about to push it, but let's hear your opinions first
> It is great, and I'd be happy if it went in as is.  Thank you.

I pushed a version with the above adjustments to master, as we're
converging anyway and it's easier than trading patches.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Felician Nemeth]

>> Even when takeFous is nil, shouldn't we still at least raise the frame?
>> Otherwise Emacs might not show the document.
>
> I don't understand.  Can you show this problem. , I think display-buffer
> ensures the buffer is displayed.  It doesn't guarantee that it is
> visible in the context of the windowing system, but that's another
> matter.

I was worried about the case when there are two frames: frame A is in
focus, and frame B is minimized but contains the target buffer.  But
you're right.  display-buffer does raise frame B and keeps the focus in
frame A.

> I pushed a version with the above adjustments to master, as we're
> converging anyway and it's easier than trading patches.

The pushed version is good, but there is one minor problematic detail.
It returns "success" even before it tries to find-file the requested
URI.  So if it cannot open the file, because the user does not have the
required permissions, it is too late to send an error to the server.

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

On Sun, May 14, 2023 at 8:02 PM Felician Nemeth
<felician.nemeth@gmail.com> wrote:
>
> >> Even when takeFous is nil, shouldn't we still at least raise the frame?
> >> Otherwise Emacs might not show the document.
> >
> > I don't understand.  Can you show this problem. , I think display-buffer
> > ensures the buffer is displayed.  It doesn't guarantee that it is
> > visible in the context of the windowing system, but that's another
> > matter.
>
> I was worried about the case when there are two frames: frame A is in
> focus, and frame B is minimized but contains the target buffer.  But
> you're right.  display-buffer does raise frame B and keeps the focus in
> frame A.

I hadn't thought of this, but good to know that display-buffer handles
it.

> > I pushed a version with the above adjustments to master, as we're
> > converging anyway and it's easier than trading patches.
>
> The pushed version is good, but there is one minor problematic detail.
> It returns "success" even before it tries to find-file the requested
> URI.  So if it cannot open the file, because the user does not have the
> required permissions, it is too late to send an error to the server.

Oof, you're right.  That's why I didn't want the async version.  Hmm, so
either we go back to the sync version (and solve the problems that that may
bring -- which AFAICT are only speculations about imenu/which-func at this
point) or we do some file-readable-p checking.

Though the latter isn't horrible, ideally we would just proclaim that
putting request-generating hooks into the major-mode hook isn't allowed.
Because it's a bad idea anyway, since a mode hook is supposed to be cheap.
Maybe eglot-request could error out (or warn and return nil) when it
detects it's running in the mode hook.  FWIW my new breadcrumb.el extension,
which also uses imenu, _can_ be put in the mode hook because it calls the
imenu-building, request-generating function asynchronously.  So it isn't
a problem there.

I wonder if Alan could inform us about the contents of his go-mode-hook,
to check if which-function-mode lives there.  That would dispel the
speculation.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

On Sun, May 14, 2023 at 8:19 PM João Távora <joaotavora@gmail.com> wrote:

> > URI.  So if it cannot open the file, because the user does not have the
> > required permissions, it is too late to send an error to the server.
>
> Oof, you're right.  That's why I didn't want the async version.  Hmm, so
> either we go back to the sync version (and solve the problems that that may
> bring -- which AFAICT are only speculations about imenu/which-func at this
> point) or we do some file-readable-p checking.
>
> Though the latter isn't horrible, ideally we would just proclaim that
> putting request-generating hooks into the major-mode hook isn't allowed.
> Because it's a bad idea anyway, since a mode hook is supposed to be cheap.

Scratch that.  It's true that it's a bad idea in this particular case, but
my proposal is far from "ideal".  The right thing to do is to extend jsonrpc.el
so that it allows asynchronous request dispatchers.  Not only would this solve
the "needs to run in separate stack frame problem" but it would solve future
problems where Elisp JSONRPC endpoints need to contact other potentially
slow endpoints.

I'm working on this change to jsonrpc.el.  This will solve the Eglot
showDocument
problem by allowing the find-file _and_ the return code calculation to be async.

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[João Távora]

[ Cc'ing Dmitry mostly because of the question in the last paragraph ]

On Mon, May 15, 2023 at 11:45 AM João Távora <joaotavora@gmail.com> wrote:

> I'm working on this change to jsonrpc.el.  This will solve the Eglot
> showDocument
> problem by allowing the find-file _and_ the return code calculation to be async.

Continuing this soliloquy :-) I'm putting this more ambitious change
on the back burner.  It's not immediately clear how to implement this
cleanly.  The best I can think of is to change jsonrpc.el to allow the
request handler to return a function  that is then passed a "success"
and an "error" callbacks.  But not only is this not spectacularly clean,
it  doesn't really solve the "nested" request problem by itself.
Furthermore there aren't any other clients for these kinds of requests
yet, and there seems to be ongoing work for a future/promises system
which could make this cleaner.

So for window/showDocument specifically, I went with the much simpler
check of file-readable-p before reporting success (or failure).

I also addressed Alan's report of a problem with the widening.  I
used xref--goto-pos, which I normally wouldn't do, but that
internal function semantics is exactly what was needed here.
Maybe Dmitry can agree to export it?

João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Alan Donovan via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Thanks for following through with this change. I just tried the
committed version and it works nicely for me.

On Tue, 16 May 2023 at 14:32, João Távora <joaotavora@gmail.com> wrote:
>
> [ Cc'ing Dmitry mostly because of the question in the last paragraph ]
>
> On Mon, May 15, 2023 at 11:45 AM João Távora <joaotavora@gmail.com> wrote:
>
> > I'm working on this change to jsonrpc.el.  This will solve the Eglot
> > showDocument
> > problem by allowing the find-file _and_ the return code calculation to be async.
>
> Continuing this soliloquy :-) I'm putting this more ambitious change
> on the back burner.  It's not immediately clear how to implement this
> cleanly.  The best I can think of is to change jsonrpc.el to allow the
> request handler to return a function  that is then passed a "success"
> and an "error" callbacks.  But not only is this not spectacularly clean,
> it  doesn't really solve the "nested" request problem by itself.
> Furthermore there aren't any other clients for these kinds of requests
> yet, and there seems to be ongoing work for a future/promises system
> which could make this cleaner.
>
> So for window/showDocument specifically, I went with the much simpler
> check of file-readable-p before reporting success (or failure).
>
> I also addressed Alan's report of a problem with the widening.  I
> used xref--goto-pos, which I normally wouldn't do, but that
> internal function semantics is exactly what was needed here.
> Maybe Dmitry can agree to export it?
>
> João

bug#62116: RFE: eglot: support window.showDocument LSP RPC

[Dmitry Gutov]

On 16/05/2023 21:34, João Távora wrote:
> I also addressed Alan's report of a problem with the widening.  I
> used xref--goto-pos, which I normally wouldn't do, but that
> internal function semantics is exactly what was needed here.
> Maybe Dmitry can agree to export it?

I'm not sure it's a good idea to export it with that name: there is 
nothing xref-specific there. So you probably want a core function like that.

Or you can just copy the definition, it's quite small. There is no hurry 
either way.