From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Newsgroups: gmane.emacs.bugs Subject: bug#60338: [PATCH] Add option to present server changes as diffs Date: Sun, 18 Jun 2023 16:18:12 +0100 Message-ID: <87a5ww7qa3.fsf@gmail.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="32587"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Eli Zaretskii , 60338@debbugs.gnu.org, Eshel Yaron To: Philip Kaludercic Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Jun 18 17:16:26 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 1qAu8f-0008Gw-Ev for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 18 Jun 2023 17:16:25 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qAu8K-00073O-Su; Sun, 18 Jun 2023 11:16: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 1qAu8I-000736-RG for bug-gnu-emacs@gnu.org; Sun, 18 Jun 2023 11:16: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 1qAu8I-00064P-J8 for bug-gnu-emacs@gnu.org; Sun, 18 Jun 2023 11:16:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qAu8I-0004F6-FJ for bug-gnu-emacs@gnu.org; Sun, 18 Jun 2023 11:16:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 18 Jun 2023 15:16:02 +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.168710135316285 (code B ref 60338); Sun, 18 Jun 2023 15:16:02 +0000 Original-Received: (at 60338) by debbugs.gnu.org; 18 Jun 2023 15:15:53 +0000 Original-Received: from localhost ([127.0.0.1]:54832 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qAu89-0004Eb-27 for submit@debbugs.gnu.org; Sun, 18 Jun 2023 11:15:53 -0400 Original-Received: from mail-wr1-f44.google.com ([209.85.221.44]:53423) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qAu87-0004EL-0L for 60338@debbugs.gnu.org; Sun, 18 Jun 2023 11:15:51 -0400 Original-Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-31126037f41so2088440f8f.2 for <60338@debbugs.gnu.org>; Sun, 18 Jun 2023 08:15:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1687101345; x=1689693345; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=Z1FUMNTOLbznxkMKblay5vYLZhZYXsEY0SX4gtkBUNw=; b=q9OzubXjxWNLsr/08SdetfbziFNfKYbbIrEBaevJ5rrNCxqbbTnOOCLycDzpzQTExN MIiFyl8QT462Ck8XDEb1P+2Y94VeR1/itVewxfcgb/fDF6e5uKKcMSjbGtG1EabBbRrU /8RmqIFXnNowjaw9zNY8RMj/X4pLYahyBqSAo48OyqiY4mx1UPQZIzPAubCzgpa3sI5Z YA2xEM8yVKB5SHd/6HXDHvfJSZ4iT+7Kw2J00cuRvpRLaIooTE09iJuzY5lgd9iVR9LC 58FOZuh6hpniRU7TvhnBLz9jrW/DY8yrP3GHrdT0GQe8iyXRd66/xnCFkTNiaDEKyrdc 8pkA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687101345; x=1689693345; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=Z1FUMNTOLbznxkMKblay5vYLZhZYXsEY0SX4gtkBUNw=; b=fhOHJVW0lqkmIpTtU0RnPG+sYggBaPScPM0goxK9zagPiOMzGcQSuPTFNfLWm9l1v3 2aIMUKmkFH3tJB5034d4E/Dqt9fI6v+9Bmg8mTmu84r+Adv4jJTIkLv+q6C/ePKuTPdg I+qNdiaUkPXk20nUhSd28wAXElQ9aixHLz1Mc/2crr142L9FN+D6DSSEoUFj4Mm9HLWu VcSm7fNcJgwdVhYOjILiI7e3EjpZ13O6IzKGysneijpYprsMBKOZRw3IirxColg1modj KZB4pglBJdWOL+QrKahS00hbuugqN2kWc8RVlP7e8ALmYfuqZOaYkUu8qKKQoWgXEdVl hpYg== X-Gm-Message-State: AC+VfDz1V5TLjac31SUjRcDHbKsh79H+UoMoAy09uLW43PoSLm/8I9Tx 32wMCT39q+iO8ZUFrOen6ZkeLdOwGeM= X-Google-Smtp-Source: ACHHUZ7LvcCllpLy3vlmNn3hXq2sng0yjmhYmvl+nNL9ckQ26FZHwOZuIs3zSNh+iD1us15cwg/lcQ== X-Received: by 2002:adf:fc4e:0:b0:30f:cf52:9194 with SMTP id e14-20020adffc4e000000b0030fcf529194mr5990884wrs.58.1687101344569; Sun, 18 Jun 2023 08:15:44 -0700 (PDT) Original-Received: from krug ([87.196.73.15]) by smtp.gmail.com with ESMTPSA id e16-20020adffc50000000b0030fbb834074sm21183971wrs.15.2023.06.18.08.15.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 18 Jun 2023 08:15:44 -0700 (PDT) In-Reply-To: <871qi9q9uc.fsf@posteo.net> (Philip Kaludercic's message of "Sun, 18 Jun 2023 11:38:19 +0000") 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:263639 Archived-At: 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? >=20=20 > (defcustom eglot-extend-to-xref nil > @@ -740,7 +746,8 @@ eglot-execute > (eglot--dcase action > (((Command)) (eglot--request server :workspace/executeCommand actio= n)) > (((CodeAction) edit command) > - (when edit (eglot--apply-workspace-edit edit)) > + (when edit > + (eglot--apply-workspace-edit edit (eq eglot-confirm-server-initi= ated-edits t))) > (when command (eglot--request server :workspace/executeCommand com= mand)))))) 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 (/) proposed changes*". 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 > - (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 > (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--TextDocumentPosition= Params) > :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. =20=20 > (defun eglot--region-bounds () > "Region bounds if active, else bounds of things at point."