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

[João Távora]

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?

>  
>  (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

>  (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?

> +          (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.

> +          (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*".  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.  

> -      (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 

>  (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.
  
>  (defun eglot--region-bounds ()
>    "Region bounds if active, else bounds of things at point."