bug#60338: [PATCH] Add option to present server changes as diffs

[Philip Kaludercic]

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

> Philip Kaludercic <philipk@posteo.net> writes:
>
>> João, do you have anything to add to this discussion?
>
> Yes, a bit.
>
> To the "should it use diff" objection, I think it's OK to keep it as is.
> Especially if we frame this new 'diff' value to Eglot's
> eglot-confirm-server-initiated-edits as a "Present changes as 'diff',
> and user takes it from there".  I'd say most programmers can read diffs
> and apply them by hand if required.
>
> Anyway this usage of the entirely separate 'diff-mode' is the strongest
> point of this feature anyway, it keeps the concerns "propose changes"
> and "apply changes" neatly separated.
>
> I also think the second patch adding 'diff-apply-everything' is fine.
>
> Now I've tried your patch very briefly and I like it, in general.  But I
> have a few comments after my sig.
>
> João
>
>> @@ -108,6 +108,7 @@
>>  (require 'filenotify)
>>  (require 'ert)
>>  (require 'text-property-search nil t)
>> +(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
>> @@ -387,8 +388,13 @@ eglot-events-buffer-size
>>                   (integer :tag "Number of characters")))
>>  
>>  (defcustom eglot-confirm-server-initiated-edits 'confirm
>> -  "Non-nil if server-initiated edits should be confirmed with user."
>> +  "Control how server edits are applied.
>> +If set to `confirm', a prompt is presented to confirm server
>> +changes.  If set to `diff', a buffer will pop up with changes
>> +that can be applied manually.  If set to nil, modifications are
>> +made automatically."
>>    :type '(choice (const :tag "Don't show confirmation prompt" nil)
>> +                 (const :tag "Present as diffs in a buffer" diff)
>>                   (const :tag "Show confirmation prompt" confirm)))
>
> This is the most important comment.  Later on, you add a condition
>
>    (eq eglot-confirm-server-initiated-edits t)
>
> Which would seem to mean "confirm unconditionally".  But 't' is not
> presented as an option in the defcustom.  What meaning should it have?
> Should it use a prompt or a diff?

No, that was a thinko, the check has to be replaced with something else.

>>  
>>  (defcustom eglot-extend-to-xref nil
>> @@ -740,7 +746,8 @@ eglot-execute
>>     (eglot--dcase action
>>       (((Command)) (eglot--request server :workspace/executeCommand action))
>>       (((CodeAction) edit command)
>> -      (when edit (eglot--apply-workspace-edit edit))
>> +      (when edit
>> +        (eglot--apply-workspace-edit edit (eq 
>> eglot-confirm-server-initiated-edits t)))
>>        (when command (eglot--request server :workspace/executeCommand 
>> command))))))
>
>
> This is where the 't' value is used.  These types of edits are usually
> automatically confirmed.  Why?  Not easy to say, but normally when the
> server suggests them they are more localized.  Also by this point the
> client already knows what is about to change, whereas we don't know what
> diff the server will provide with the ones of "Command"-type, which
> require a further round trip.
>
> Anyway it would be very helpful to describe this in the manual and in
> the docstrings.  So that a user can say: "even in those simple edits I
> want a diff-style presentation of what is about to happen".
>
> This may require another custom option of more added syntax to the
> existing eglot-confirm-server-initiated-edits option.

> Though I would appreciate if you could take this opportunity to address
> them, you can also choose to simply not answer these tough questions.
> But in this case, this hunk should be reverted

I've gone ahead and fixed the check, to pass 'diff if
`eglot-confirm-server-initiated-edits' was set to 'diff, otherwise nil.
That seems like the most sensible option to me.

>>  (cl-defgeneric eglot-initialization-options (server)
>> @@ -3408,18 +3415,43 @@ eglot--apply-workspace-edit
>>          ;; 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
>> +      (cond
>> +       ((eq confirm 'diff)
>> +        (with-current-buffer (get-buffer-create " *Changes*")
>> +          (buffer-disable-undo (current-buffer))
>> +          (let ((buffer-read-only t))
>> +            (diff-mode))
>
> Why bind buffer-read-only to t here?  Why not re-enter diff-mode at the end
> of cosntruction?

I am not sure what you mean by re-enter?  The point of binding
`buffer-read-only' is just to enable `diff-mode-read-only' on enabling
the mode, but we can also set if afterwards.  That would require adding
a defvar though.

>> +          (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))
>
> pop-to-buffer is fine, I guess, though I wonder if some users wouldn't
> prefer display-buffer.

I guess that this is a persistent disagreement between users and their
expectations.  I don't expect it to be resolved here, not at least
because I cannot put my own preferences into words.

>> +          (font-lock-ensure)))
>
> I think this logic should be in a new helper function.
>
> And the generated buffer name should be more Eglotish, and not hidden.
> It should read something like "*EGLOT (<project-name>/<modes>) proposed
> changes*".  

Oh, I was under the impression that this was just the name for
"internal" or hidden buffers, that don't interest the user directory.

>             Ideally the logic for generating the usual prefix "*EGLOT
> (<project-name>/<modes>)" should also be extracted, so that we can
> switch to something less ugly in the fugure.  In a different commit or
> patch, of course.  

Should we then keep it the way this is for now, and when this function
exists make use of it?

>> -      (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!")))))
>> +                           (mapconcat #'identity (mapcar #'car prepared) 
>> "\n  ")))))
>> +        (jsonrpc-error "User canceled server edit"))
>> +       ((cl-loop for (path edits version) in prepared
>> +                 do (with-current-buffer (find-file-noselect path)
>> +                      (eglot--apply-text-edits edits version))
>> +                 finally (eldoc) (eglot--message "Edit successful!")))))))
>
> This last bit of cl-loop change, though better, doesn't seem related to
> the new feature being developed.  It's not very important, but it does
> complicate reading the patch.  You can always provide such improvements 

Reverted.

>>  (defun eglot-rename (newname)
>>    "Rename the current symbol to NEWNAME."
>> @@ -3434,7 +3466,7 @@ eglot-rename
>>     (eglot--request (eglot--current-server-or-lose)
>>                     :textDocument/rename 
>> `(,@(eglot--TextDocumentPositionParams)
>>                                            :newName ,newname))
>> -   current-prefix-arg))
>> +   (and current-prefix-arg eglot-confirm-server-initiated-edits)))
>
> I get the idea and I like it.  Now one can preview the renames.  But if
> 'eglot-confirm-server-initiated-edits' is nil, now I can't use C-u to
> override this decision.  Not necessarily something we need to fix in
> your patch, but it should be taken in consideration with the first
> comment about documentating the semantics of this variable.

I've modified it, see below.

>>  (defun eglot--region-bounds ()
>>    "Region bounds if active, else bounds of things at point."

diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el
index 9c9c3152472..9c76d4abdef 100644
--- a/lisp/progmodes/eglot.el
+++ b/lisp/progmodes/eglot.el
@@ -747,7 +747,8 @@ eglot-execute
      (((Command)) (eglot--request server :workspace/executeCommand action))
      (((CodeAction) edit command)
       (when edit
-        (eglot--apply-workspace-edit edit (eq 
eglot-confirm-server-initiated-edits t)))
+        (let ((confirm (and (eq eglot-confirm-server-initiated-edits 'diff) 
'diff)))
+          (eglot--apply-workspace-edit edit confirm)))
       (when command (eglot--request server :workspace/executeCommand 
command))))))
 
 (cl-defgeneric eglot-initialization-options (server)
@@ -3401,7 +3402,9 @@ eglot--apply-text-edits
       (progress-reporter-done reporter))))
 
 (defun eglot--apply-workspace-edit (wedit &optional confirm)
-  "Apply the workspace edit WEDIT.  If CONFIRM, ask user first."
+  "Apply the workspace edit WEDIT.
+CONFIRM should have a value as specified in the user option
+`eglot-confirm-server-initiated-edits'."
   (eglot--dbind ((WorkspaceEdit) changes documentChanges) wedit
     (let ((prepared
            (mapcar (eglot--lambda ((TextDocumentEdit) textDocument edits)
@@ -3448,7 +3451,8 @@ eglot--apply-workspace-edit
                    (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 (path edits version) in prepared
+       ((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!")))))))
@@ -3466,7 +3470,7 @@ eglot-rename
    (eglot--request (eglot--current-server-or-lose)
                    :textDocument/rename `(,@(eglot--TextDocumentPositionParams)
                                           :newName ,newname))
-   (and current-prefix-arg eglot-confirm-server-initiated-edits)))
+   (and current-prefix-arg (or eglot-confirm-server-initiated-edits 
'confirm))))
 
 (defun eglot--region-bounds ()
   "Region bounds if active, else bounds of things at point."

-- 
Philip Kaludercic