João Távora writes: > Philip Kaludercic 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 (/) 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 > (/)" 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."