bug#63676: cancelling editable dired causes UI problems with dired

[Michael Heerdegen <michael_heerdegen@web.de>]

Eli Zaretskii <eliz@gnu.org> 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.