Eglot: revamp confirmation model for server-proposed edits

bug#60338

The variable 'eglot-confirm-server-edits' replaces the obsolete
'eglot-confirm-server-initiated-edits' and brings about a new
confirmation model, making it possible to have only certain commands
require user confirmation.  This was achieved careful usage of the
'this-command' and 'last-command' variables.

There are now two types of confirmation: the usual
minibuffer summary and a temporary 'diff-mode' buffer to display the
proposed changes, so the user can apply them one by one.

Thanks to Philip Kaludercic for the diff-mode idea and implementation.

Co-authored-by: Philip Kaludercic <philipk@posteo.net>
* doc/misc/eglot.texi (Eglot Variables): Describe
'eglot-confirm-server-edits'.

* etc/EGLOT-NEWS (latest): Mention change.

* lisp/progmodes/eglot.el (diff-mode): Require it.
(eglot-confirm-server-initiated-edits): Obsolete it.
(eglot-confirm-server-edits): New variable.
(eglot-handle-request workspace/applyEdit): Use 'last-command'
(eglot-execute t t): Use 'this-command'.
(eglot--apply-workspace-edit): Rework.
(eglot-rename): Use 'this-command'.

diff --git a/doc/misc/eglot.texi b/doc/misc/eglot.texi
index 6eb212ca841b6ac3781694fce511835d6606cc24..3338756c63c553f99f56ec45d621a3eece187628 100644 (file)
--- a/doc/misc/eglot.texi
+++ b/doc/misc/eglot.texi
@@ -831,12 +831,14 @@ last buffer managed by it is killed.  @xref{Shutting Down LSP Servers}.
 The default is @code{nil}; if you want to shut down a server, use
 @kbd{M-x eglot-shutdown} (@pxref{Eglot Commands}).
 
-@item eglot-confirm-server-initiated-edits
+@item eglot-confirm-server-edits
 Various Eglot commands and code actions result in the language server
 sending editing commands to Emacs.  If this option's value is
-non-@code{nil} (the default), Eglot will ask for confirmation before
-performing edits initiated by the server or edits whose scope affects
-buffers other than the one where the user initiated the request.
+non-@code{nil}, Eglot will ask for confirmation before performing
+edits proposed by the language server.  This option's value can be
+crafted to require this confirmation for specific commands or only
+when the edit affects files not yet visited by the user.  Consult this
+option's docstring for more information.
 
 @item eglot-ignored-server-capabilities
 This variable's value is a list of language server capabilities that

diff --git a/etc/EGLOT-NEWS b/etc/EGLOT-NEWS
index 01f0498eb81b5b61cc32ba2c78c328069053c13c..980b06030e8ec754e1aa4b4557c5cb1dd90749f0 100644 (file)
--- a/etc/EGLOT-NEWS
+++ b/etc/EGLOT-NEWS
@@ -20,6 +20,15 @@ https://github.com/joaotavora/eglot/issues/1234.
 \f
 * Changes in upcoming Eglot
 
+** Diff previews of edits and new variable 'eglot-confirm-server-edits'
+
+The variable 'eglot-confirm-server-edits' replaces the obsolete
+'eglot-confirm-server-initiated-edits' and brings about a new
+confirmation model, making it possible to have only certain commands
+require user confirmation.  The type of confirmation has also been
+enhanced.  In particular it allows a temporary 'diff-mode' buffer to
+display the proposed changes, so the user can apply them one by one.
+
 ** Optimized file-watching capability
 
 Some servers, like the Pyright language server, issue too many file

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 6f08f26857b5805df851db3da9236df0be7710d5..8d95019c3ed1ea6bedcb901783c9b4a96ab4d17d 100644 (file)
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -108,6 +108,8 @@
 (require 'filenotify)
 (require 'ert)
 (require 'text-property-search nil t)
+(require 'diff-mode)
+(require 'diff)
 
 ;; These dependencies are also GNU ELPA core packages.  Because of
 ;; bug#62576, since there is a risk that M-x package-install, despite
@@ -388,10 +390,35 @@ done by `eglot-reconnect'."
   :type '(choice (const :tag "No limit" nil)
                  (integer :tag "Number of characters")))
 
-(defcustom eglot-confirm-server-initiated-edits 'confirm
-  "Non-nil if server-initiated edits should be confirmed with user."
-  :type '(choice (const :tag "Don't show confirmation prompt" nil)
-                 (const :tag "Show confirmation prompt" confirm)))
+(define-obsolete-variable-alias 'eglot-confirm-server-initiated-edits
+  'eglot-confirm-server-edits "1.16")
+
+(defcustom eglot-confirm-server-edits '((eglot-rename . nil)
+                                        (t . maybe-summary))
+  "Control if changes proposed by LSP should be confirmed with user.
+
+If this variable's value is the symbol `diff', a diff buffer is
+popped up, allowing the user to apply each change individually.
+If the symbol `summary' or any other non-nil value a short
+summary of changes is presented to the user in a
+minibuffer-prompt.  The symbols `maybe-diff' and `maybe-summary'
+are also accepted and mean that the confirmation is presented to
+the user if the changes target visited files only.  A nil value
+means the change is applied directly to visited and non-visited
+files, without any confirmation.
+
+If this variable's value is a list, it should be an
+alist ((COMMAND . ACTION) ...) where COMMAND is a symbol
+designating a command, such as `eglot-rename',
+`eglot-code-actions', `eglot-code-action-quickfix', etc.  ACTION
+is one of the symbols described above.  The value `t' for COMMAND
+is accepted and its ACTION is the default value."
+  :type '(choice (const :tag "Use diff" diff)
+                 (const :tag "Summarize and prompt" summary)
+                 (const :tag "Maybe use diff" maybe-diff)
+                 (const :tag "Maybe summarize and prompt" maybe-summary)
+                 (const :tag "Don't confirm" nil)
+                 (alist :tag "Per-command alist")))
 
 (defcustom eglot-extend-to-xref nil
   "If non-nil, activate Eglot in cross-referenced non-project files."
@@ -753,7 +780,7 @@ ACTION is an LSP object of either `CodeAction' or `Command' type."
       (if (and (null edit) (null command) data
                (eglot--server-capable :codeActionProvider :resolveProvider))
           (eglot-execute server (eglot--request server :codeAction/resolve action))
-        (when edit (eglot--apply-workspace-edit edit))
+        (when edit (eglot--apply-workspace-edit edit this-command))
         (when command (eglot--request server :workspace/executeCommand command)))))))
 
 (cl-defgeneric eglot-initialization-options (server)
@@ -2379,7 +2406,7 @@ THINGS are either registrations or unregisterations (sic)."
 (cl-defmethod eglot-handle-request
   (_server (_method (eql workspace/applyEdit)) &key _label edit)
   "Handle server request workspace/applyEdit."
-  (eglot--apply-workspace-edit edit eglot-confirm-server-initiated-edits)
+  (eglot--apply-workspace-edit edit last-command)
   `(:applied t))
 
 (cl-defmethod eglot-handle-request
@@ -3431,8 +3458,42 @@ If SILENT, don't echo progress in mode-line."
       (when reporter
         (progress-reporter-done reporter)))))
 
-(defun eglot--apply-workspace-edit (wedit &optional confirm)
-  "Apply the workspace edit WEDIT.  If CONFIRM, ask user first."
+(defun eglot--confirm-server-edits (origin _prepared)
+  (let (v)
+    (cond ((symbolp eglot-confirm-server-edits) eglot-confirm-server-edits)
+          ((setq v (assoc origin eglot-confirm-server-edits)) (cdr v))
+          ((setq v (assoc t eglot-confirm-server-edits)) (cdr v)))))
+
+(defun eglot--propose-changes-as-diff (prepared)
+  "Helper for `eglot--apply-workspace-edit'.
+PREPARED is a list ((FILENAME EDITS VERSION)...)."
+  (with-current-buffer (get-buffer-create "*EGLOT proposed server changes*")
+    (buffer-disable-undo (current-buffer))
+    (let ((buffer-read-only t))
+      (diff-mode))
+    (let ((inhibit-read-only t)
+          (target (current-buffer)))
+      (erase-buffer)
+      (pcase-dolist (`(,path ,edits ,_) prepared)
+        (with-temp-buffer
+          (let ((diff (current-buffer)))
+            (with-temp-buffer
+              (insert-file-contents path)
+              (eglot--apply-text-edits edits)
+              (diff-no-select path (current-buffer)
+                              nil t diff))
+            (with-current-buffer target
+              (insert-buffer-substring diff))))))
+    (setq-local buffer-read-only t)
+    (buffer-enable-undo (current-buffer))
+    (goto-char (point-min))
+    (pop-to-buffer (current-buffer))
+    (font-lock-ensure)))
+
+(defun eglot--apply-workspace-edit (wedit origin)
+  "Apply the workspace edit WEDIT.
+ORIGIN is a symbol designating the command that originated this
+edit proposed by the server."
   (eglot--dbind ((WorkspaceEdit) changes documentChanges) wedit
     (let ((prepared
            (mapcar (eglot--lambda ((TextDocumentEdit) textDocument edits)
@@ -3446,18 +3507,36 @@ If SILENT, don't echo progress in mode-line."
         ;; prefer documentChanges over changes.
         (cl-loop for (uri edits) on changes by #'cddr
                  do (push (list (eglot--uri-to-path uri) edits) prepared)))
-      (if (or confirm
-              (cl-notevery #'find-buffer-visiting
-                           (mapcar #'car prepared)))
-          (unless (y-or-n-p
-                   (format "[eglot] Server wants to edit:\n  %s\n Proceed? "
-                           (mapconcat #'identity (mapcar #'car prepared) "\n  ")))
-            (jsonrpc-error "User canceled server edit")))
-      (cl-loop for edit in prepared
-               for (path edits version) = edit
-               do (with-current-buffer (find-file-noselect path)
-                    (eglot--apply-text-edits edits version))
-               finally (eldoc) (eglot--message "Edit successful!")))))
+      (cl-flet ((notevery-visited-p ()
+                  (cl-notevery #'find-buffer-visiting
+                               (mapcar #'car prepared)))
+                (prompt ()
+                  (unless (y-or-n-p
+                           (format "[eglot] Server wants to edit:\n%sProceed? "
+                                   (cl-loop
+                                    for (f eds _) in prepared
+                                    concat (format
+                                            "  %s (%d change%s)\n"
+                                            f (length eds)
+                                            (if (> (length eds) 1) "s" "")))))
+                    (jsonrpc-error "User canceled server edit")))
+                (apply ()
+                  (cl-loop for edit in prepared
+                   for (path edits version) = edit
+                   do (with-current-buffer (find-file-noselect path)
+                        (eglot--apply-text-edits edits version))
+                   finally (eldoc) (eglot--message "Edit successful!"))))
+        (let ((decision (eglot--confirm-server-edits origin prepared)))
+          (cond
+           ((or (eq decision 'diff)
+                (and (eq decision 'maybe-diff) (notevery-visited-p)))
+            (eglot--propose-changes-as-diff prepared))
+           ((or (eq decision 'summary)
+                (and (eq decision 'maybe-summary) (notevery-visited-p)))
+            (prompt)
+            (apply))
+           (t
+            (apply))))))))
 
 (defun eglot-rename (newname)
   "Rename the current symbol to NEWNAME."
@@ -3472,7 +3551,7 @@ If SILENT, don't echo progress in mode-line."
    (eglot--request (eglot--current-server-or-lose)
                    :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
                                           :newName ,newname))
-   current-prefix-arg))
+   this-command))
 
 (defun eglot--region-bounds ()
   "Region bounds if active, else bounds of things at point."