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