From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Michael Heerdegen Newsgroups: gmane.emacs.bugs Subject: bug#63676: cancelling editable dired causes UI problems with dired Date: Sun, 28 May 2023 03:36:50 +0200 Message-ID: <87a5xp6xu5.fsf@web.de> References: <83ilcing54.fsf@gnu.org> <87jzwupp7n.fsf@web.de> <83edn2jnqd.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21721"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: peter.mao@gmail.com, 63676@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun May 28 03:37:21 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 1q35LU-0005T7-VG for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 28 May 2023 03:37:21 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1q35LG-0005AG-Nx; Sat, 27 May 2023 21:37:06 -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 1q35LE-00059x-16 for bug-gnu-emacs@gnu.org; Sat, 27 May 2023 21:37:04 -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 1q35LC-00078p-7S for bug-gnu-emacs@gnu.org; Sat, 27 May 2023 21:37:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1q35LC-0002f6-2D for bug-gnu-emacs@gnu.org; Sat, 27 May 2023 21:37:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Michael Heerdegen Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 28 May 2023 01:37:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 63676 X-GNU-PR-Package: emacs Original-Received: via spool by 63676-submit@debbugs.gnu.org id=B63676.168523782110226 (code B ref 63676); Sun, 28 May 2023 01:37:02 +0000 Original-Received: (at 63676) by debbugs.gnu.org; 28 May 2023 01:37:01 +0000 Original-Received: from localhost ([127.0.0.1]:52822 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1q35LA-0002er-Q8 for submit@debbugs.gnu.org; Sat, 27 May 2023 21:37:01 -0400 Original-Received: from mout.web.de ([212.227.17.11]:36957) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1q35L8-0002ec-4I for 63676@debbugs.gnu.org; Sat, 27 May 2023 21:36:58 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=web.de; s=s29768273; t=1685237811; x=1685842611; i=michael_heerdegen@web.de; bh=HDjRemJuS99XF6vxbQDG0Jw34iLNDbVcdCfQr5bsgXU=; h=X-UI-Sender-Class:From:To:Cc:Subject:In-Reply-To:References:Date; b=qZ25SKu1jEe4BYcF8WN8ZE9CNNSQTzPWwHur4aS6/7RnNNXDDuqSCD2PFwVr/QWi64ifzVB ACSRAUN9oqRoL9ZUg6qtg/CSCOkYNPyRCu4AIzC0MxfkzaYuqMXwNub27JRQlJ5Bm0Br9mY// gF3BWAZjlmBBLMDzl+RsOvm27/CUoOCyTu7K+tIt4aM2B4HOkDIAX649wmRLTQCkNglwYtsmx uLjTgO5hBaUKdoWBYUYJIargt9t9yWc35yJJ6YqMzG7aG/A6f/OsojFOOJyT36DmTMu3hzo9q A7QLgdaz1t+4r5e6A3BfKq71szgd7wcm5fJqqlhbGD0CZXeP+p1g== X-UI-Sender-Class: 814a7b36-bfc1-4dae-8640-3722d8ec6cd6 Original-Received: from drachen.dragon ([178.14.74.62]) by smtp.web.de (mrweb105 [213.165.67.124]) with ESMTPSA (Nemesis) id 1MlbHE-1qSlRB0X8k-00ik9Z; Sun, 28 May 2023 03:36:51 +0200 In-Reply-To: <83edn2jnqd.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 27 May 2023 09:24:26 +0300") X-Provags-ID: V03:K1:vUADl2Ugnxet4agzJvboV3EaaYBoAC7tXbUBypHJn56ncFw7Zag i/W+sgdCvX14pWHdOcAW4sjo8h6uFN7eJ6f7wK09C2zAss6NMi8U4KVPWL3TClq2Mo1ji5Q 33i/ZwFjdQ+EF9kCZIT3JI6is521DQE0Bm2t6+p4W5V837ZLpBzUTi5AvRIhFuz7deAS2nu g5THr639mC/yX5zCNxOEg== UI-OutboundReport: notjunk:1;M01:P0:CBzhEBlbn1k=;iuxTq00lhQKy14n1zEv/aLtzZzb E3mPqLNl3sjNfigzI9YPCFPkt5sPUKiEW8/5bhO2hT5oHznfgTju3mIQp0W4IOXqnMw2h4eNP CPfJWil7Hc7BQoog8O7bdzwO3OTlZTLmXWE+90jQ/GOEqAXtYZ+3QQ2uC7R3UaTIp9yktBAZj K4U/izqgICi4vuc2XqwCPiwvsPLi7AlwMGadcyAspNHLLawAE0G+2U8MXvFjwkTHMXNlStqre 0Ydf9dgsQWbcOI9qYR+qc9ZpHdXVp5yZszvIHknm//zrrjNbwYoP0tp9LNaw+Uav62PBR/oq9 PA+3kla1Eh5hC8JvmHc7O91eJNcvATrOvw459SimM8wBlhaZyOLTwRzGruwmIglMszCryfnbG rsrqQhqpV6wdaOzBLo71rGvNgny7JD3+u8K/TFxSHejBWJjJMZ1uQTsSbt+N/Ps2Xg6s1a7Lb WPLpW8VfPUgubg335jbiZnAZBUP0ZIohWmcdASz3guSOJhjvgocQ9/S1a1HkD+0sCSyho5Iie 7UNSKB/ru06MzEN/k7f7komtxtTCyM8Xk0NdJNCj84JCGNaOhEUzqW7g9pXrsftMxKfdavlxH QG53YihAo11Ww9m4hzy0xW4F0aPXw/MH+7QzroqaySKxneFxUTGl9QVWQzMTfXI9Q13nYtMXd ZM5O+RLL54aYS2TaLVkbpIvGO+iPt48PGsfapakA1/iiLNonRQYT8AsyiVbz7yQ9qVj+couTS 8Hcoen2LgWO7umMZr3XeE9LFVAUbsJr634CwmYIT5ITfC5dcMjViwYYNcheaR+Emg12kcD5+ 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:262497 Archived-At: Eli Zaretskii writes: > > I think this should be appropriate [...]: First, sorry for my initial confusion. I had forgotten where I was using a modified version of wdired code. But please be assured that I did not make this suggestion lightly. > Thanks, but why removal of the comment? is the comment incorrect or > inaccurate? I think having comments that explain why we do > non-trivial things is an advantage. I think it was inaccurate: enabling and aborting wdired does not touch local variables (unlike major mode changes that delete locals). We only need to revert the things that have been changed explicitly (plus anything the user might have done in the meantime). The code already does do this carefully. Going back to a fully functional dired buffer is not something that was not being addressed. This issue is a bit special because it was not trivially foreseeable: because the old buffer contents are being restored, marker positions are shredded. I have verified that `dired-subdir-alist' is the only local variable carrying markers. > > Background: Aborting wdired (`wdired-abort-changes') erases the > > buffer and insert the original buffer contents, then re-enters > > dired-mode. Positions in `dired-subdir-alist' (that are necessary for > > $) are represented as markers. These just have to be updated. > > Are we sure this is the _only_ thing that needs to be updated? > dired-revert does much more, so we should audit what it does carefully > to determine which parts may need re-doing here. My point is that dired-revert does _too_ much - see below. And I'm relatively sure that what we used to do is not problematic, so this is not necessary (please note that the installed patch introduces a mistake into the code: we now remember the buffer contents when entering wdired in a variable. When aborting, we now erase the buffer, restore those remembered old contents, throw all of that away again, and revert from anew). > If you did that, would you please present the analysis and the > conclusions? > In particular, wdired-abort-changes could be called > after more commands than the original recipe shows, and that could > affect other aspects of the Dired buffer, not just dired-subdir-alist. You only edit a text buffer then whose contents are later completely discarded. Dired commands are completely unavailable and/or not functional. Marks are part of the saved original buffer contents. Local variables are not touched. ATM I can't imagine what else could go wrong. Note that the posted recipe doesn't involve doing anything in between: already entering wdired and immediate aborting causes the problem. What you do in between is irrelevant in this case. > And having said all that, I don't really understand why we should > worry so much about the downsides of the solution: is > wdired-abort-changes something that is used a lot? At least its speed > sounds not important at all, and if the information it loses is indeed > important enough, the way to avoid that is to restore that information > after reverting, perhaps the way wdired-finish-edit does (which, btw, > does call revert). This is not a trivial matter. Reverting, for example, also gets rid of information, like that of `dired-do-kill-lines' calls. Note that `dired-revert' is existing to _undo_ such things, to get to the initial, starting state. That's not what the user normally wants when aborting wdired. This is like aborting `query-replace' would call `revert-buffer'! An example: When I'm in the middle of a complex renaming operation using wdired, I might have done some preparations before entering wdired, like killing some unrelated lines so that it's simpler to use rectangle commands on the rest (to rename a "block" of files). When aborting reverts the buffer (Say, I made a mistake ans lost orientation), I now have to do the preparations again. This is not helpful. Also note that there are things that we can't easily restore at all: markers that are not under the control of dired are lost (also with the original code). For example, when you remember a (file) position in dired using a register (say, you want to rename it later, or copy it into another buffer after doing something else) the position is saved using a marker - after reverting (or wdired-aborting) it is gone. Other packages or features might use markers and overlays for other purposes (highlighting stuff, showing thumbnails, such things). Since wdired has developed in a direction where it only very carefully touches the buffer at all (good! toggling wdired should only toggle the mode of operation and leave the rest as is), the right direction, in my opinion, is to try to avoid not explicitly user requested modifications as much as possible. The current patch goes into the opposite direction :-( I'll have another look whether something else could be broken and everything that is part of the dired buffer's state is restored appropriately, ok? And of course did I not want suggest to experiment with any further things on the release branch. Michael.