From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.bugs Subject: bug#60338: [PATCH] Add option to present server changes as diffs Date: Sat, 24 Jun 2023 16:53:50 +0000 Message-ID: <87jzvsdco1.fsf@posteo.net> References: <87ilhy1dub.fsf@posteo.net> <877cs97kgb.fsf@posteo.net> <83jzw8yjv5.fsf@gnu.org> <877cs8g8oy.fsf@posteo.net> <83fs6wyhak.fsf@gnu.org> <87y1koerm6.fsf@posteo.net> <83bkhkyeze.fsf@gnu.org> <87v8fr6o86.fsf@posteo.net> <87zg52e12n.fsf@posteo.net> <871qi9q9uc.fsf@posteo.net> <87a5ww7qa3.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="24135"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Eli Zaretskii , 60338@debbugs.gnu.org, Eshel Yaron To: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sat Jun 24 18:55:23 2023 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qD6Xj-00061v-O5 for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 24 Jun 2023 18:55:23 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qD6XQ-000424-2C; Sat, 24 Jun 2023 12:55:04 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qD6XO-00041w-Oi for bug-gnu-emacs@gnu.org; Sat, 24 Jun 2023 12:55:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qD6XO-0007LI-Ff for bug-gnu-emacs@gnu.org; Sat, 24 Jun 2023 12:55:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qD6XN-0002M2-Qp for bug-gnu-emacs@gnu.org; Sat, 24 Jun 2023 12:55:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Philip Kaludercic Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 24 Jun 2023 16:55:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 60338 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 60338-submit@debbugs.gnu.org id=B60338.16876256538991 (code B ref 60338); Sat, 24 Jun 2023 16:55:01 +0000 Original-Received: (at 60338) by debbugs.gnu.org; 24 Jun 2023 16:54:13 +0000 Original-Received: from localhost ([127.0.0.1]:41392 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qD6WM-0002KR-QY for submit@debbugs.gnu.org; Sat, 24 Jun 2023 12:54:13 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]:58825) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qD6WL-0002KE-2G for 60338@debbugs.gnu.org; Sat, 24 Jun 2023 12:53:58 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 19F6E240029 for <60338@debbugs.gnu.org>; Sat, 24 Jun 2023 18:53:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1687625631; bh=ZD4xC/SCGK/TQhNO+Xyuy8/lwgRXdrGG4BjSOWWwRCE=; h=From:To:Cc:Subject:Autocrypt:Date:Message-ID:MIME-Version:From; b=fK4E/k6rThmuC9R2FqXf3ez11f13RU4ZvC9MLTFpMb7DVUDbQa0nq4J/pMQ4UurX0 Y8YR1J5cNhH6tITHKGDq/8YQ5/z6Xxtx2mQloupSD5tASJ7IQ7WdZ/ywOZltxPluaR R4qrQ2uzX8G+Hn04OmCYlOhtqM2gU+lAoB2hWXix8eBLxnJuYfOwgJ9FWvS5ojF0zs 2IYe5ebMHMQGnwPf8k2BoyroxzLKXR/LlIxm5RhA9TvAcDwLb16+m+c1UDbK1QWbAA ziPZ8TP1Ry/+QDNmKHyyTsqOWz1jvjZZtRsiUVTBlyvxlkQ9BQ+6ptg6K5oGEO/EFq 3oYwXCRH3QqGg== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4QpKv23YkTz9rxN; Sat, 24 Jun 2023 18:53:50 +0200 (CEST) In-Reply-To: <87a5ww7qa3.fsf@gmail.com> ("=?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?="'s message of "Sun, 18 Jun 2023 16:18:12 +0100") X-Hashcash: 1:20:230624:60338@debbugs.gnu.org::E9JvDwuu2Nsq+lvN:3KJ X-Hashcash: 1:20:230624:me@eshelyaron.com::b5AlSDyIda6dd03r:+JH X-Hashcash: 1:20:230624:joaotavora@gmail.com::XaPLezkUKbAcHSTB:0J8r X-Hashcash: 1:20:230624:eliz@gnu.org::zp1j9jZevM1awxFr:0MZl Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:264005 Archived-At: --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Jo=C3=A3o T=C3=A1vora writes: > Philip Kaludercic writes: > >> Jo=C3=A3o, 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=C3=A3o > >> @@ -108,6 +108,7 @@ >> (require 'filenotify) >> (require 'ert) >> (require 'text-property-search nil t) >> +(require 'diff) >>=20=20 >> ;; 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"))) >>=20=20 >> (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. >>=20=20 >> (defcustom eglot-extend-to-xref nil >> @@ -740,7 +746,8 @@ eglot-execute >> (eglot--dcase action >> (((Command)) (eglot--request server :workspace/executeCommand acti= on)) >> (((CodeAction) edit command) >> - (when edit (eglot--apply-workspace-edit edit)) >> + (when edit >> + (eglot--apply-workspace-edit edit (eq eglot-confirm-server-init= iated-edits t))) >> (when command (eglot--request server :workspace/executeCommand co= mmand)))))) > > > 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) prepare= d))) >> - (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 e= nd > 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*".=20=20 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.=20=20 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) =3D 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=20 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--TextDocumentPositio= nParams) >> :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." --=-=-= Content-Type: text/plain Content-Disposition: inline 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." --=-=-= Content-Type: text/plain -- Philip Kaludercic --=-=-=--