Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make it possible to select server to shut down from eglot-shutdown #73

Closed
wants to merge 1 commit into from
Closed

Make it possible to select server to shut down from eglot-shutdown #73

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion eglot.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,9 @@ If PRESERVE-BUFFERS is non-nil (interactively, when called with a
prefix argument), do not kill events and output buffers of
SERVER. Don't leave this function with the server still
running."
(interactive (list (eglot--current-server-or-lose) t nil current-prefix-arg))
(interactive (list
(eglot--read-server "Shut down server: ")
t nil current-prefix-arg))
(eglot--message "Asking %s politely to terminate" (jsonrpc-name server))
(unwind-protect
(progn
Expand Down Expand Up @@ -295,6 +297,20 @@ running."
(push sym retval))))
retval))

(defun eglot--read-server (prompt)
"Read a server from the minibuffer with PROMPT."
(let ((alist
(mapcar
(lambda (server) (cons (jsonrpc-name server) server))
(delq nil
(cons (eglot--current-server)
(apply #'append
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few problems here

  • apply #'append is pathologically slow for large lists because of the variable arglist of append.
    A cl-loop can iterate through hash tables cheaply and append on the go.

  • there's no need to also cons a new alist or use hash-table-values

  • I don't understand the need for delq nil or delete-dups (though we've been forgetting to purge the hash table of empty entries, but that's a different problem).

  • We probably want to error if there are no servers and return the only server without prompting if it exists, or if it is the current server.

I came up with this lightly tested version:

(defun eglot--read-server (prompt &optional dont-if-just-the-one)
  "Read a running Eglot server from minibuffer using PROMPT.
If DONT-IF-JUST-THE-ONE and there's only one server, don't prompt
and just return it."
  (let ((servers (cl-loop for servers
                           being hash-values of eglot--servers-by-project
                           append servers)))
    (cond ((null servers)
           (eglot--error "No servers!"))
          ((or (cdr servers) (not dont-if-just-the-one))
           (let ((read (completing-read
                        prompt (mapcar #'jsonrpc-name servers)
                        nil t
                        (when-let ((current (eglot--current-server)))
                          (jsonrpc-name current)))))
             (cl-find read servers :key #'jsonrpc-name :test #'equal)))
          (t (car servers)))))

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apply #'append is pathologically slow for large lists because of the variable arglist of append.
A cl-loop can iterate through hash tables cheaply and append on the go.

I didn't think about that, I assumed that even with a large number of servers (I never use more than 3 myself), the delay would be unnoticeable.

I don't understand the need for delq nil or delete-dups (though we've been forgetting to purge the hash table of empty entries, but that's a different problem).

I'm prepending the value of eglot--current-server to the list, then deleting that element if it was nil. completing-read doesn't allow nil values. I'm using delete-dups because when I prepend eglot-current-server, it's still somewhere in the tail of the other list. I don't know if completing-read removes duplicates, but I use ivy, and it didn't delete them.

We probably want to error if there are no servers and return the only server without prompting if it exists, or if it is the current server.

I think it might be a bit unexpected when there is only one server and we kill it. I would have to always check how many servers there are before I call eglot-shutdown to make sure I don't kill the current server if I don't want to.

A cl-loop can iterate through hash tables cheaply and append on the go.

Thanks, I don't know 'cl very well. I will try to rewrite it, or you can push your version as mine will probably be very similar.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't think about that, I assumed that even with a large number of servers (I never use more than 3 myself), the delay would be unnoticeable.

If you (hash-table-count eglot-servers-by-project) the result may surprise you, but that's the bug I was referring to. Anyway, it's good practice in general to avoid applying to things whose lengths we don't control well.

I'm prepending the value of eglot--current-server to the list, then deleting that element if it was nil. completing-read doesn't allow nil values. I'm using delete-dups because when I prepend eglot-current-server, it's still somewhere in the tail of the other list. I don't know if completing-read removes duplicates, but I use ivy, and it didn't delete them.

OK, but it's not needed if you use the initial-value argument to completing-read. (What could be problematic is if you have two projects identically named)

I think it might be a bit unexpected when there is only one server and we kill it. I would have to always check how many servers there are before I call eglot-shutdown to make sure I don't kill the current server if I don't want to.

Then we can just call my version of eglot--read-server with eglot--current-server as the second argument.

Thanks, I don't know 'cl very well. I will try to rewrite it, or you can push your version as mine will probably be very similar.

OK I will push mine and I hope you don't mind the lisp pedancy.

(hash-table-values eglot--servers-by-project)))))))
(cdr
(assoc
(completing-read prompt (delete-dups alist) nil t nil nil (car alist))
alist))))

(defvar eglot--command-history nil
"History of CONTACT arguments to `eglot'.")

Expand Down