bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hi,

a short demonstration for emacs -Q:

1. M-x dired a directory containing some files

2. Go to the first file's line (not "." or ".."), and mark the file
with m

3. M-x wdired-change-to-wdired-mode

4. Edit the name of the first file: e.g. add a "z" to the front of the
   name so that the file will surely appear at another position after
   renaming

5. C-c C-c

You'll see that point is still at the first file's line, although the
renamed file is now displayed near the end of the buffer.  Even worse,
the mark of the file has disappeared.

Background: `wdired-finish-edit' calls `revert-buffer', which will
call `dired-revert'.  Before this is called, everything is ok.  But
`dired-revert' fails to remember the mark and the position of the
renamed file correctly.  I didn't dig any further, but I guess a
specialized revert function is needed for `wdired-finish-edit'.


Thanks,

Michael.


In GNU Emacs 24.1.50.1 (i486-pc-linux-gnu, GTK+ Version 3.4.2)
 of 2012-06-22 on zelenka, modified by Debian
 (emacs-snapshot package, version 2:20120622-1)
Windowing system distributor `The X.Org Foundation', version 11.0.11201902
Configured using:
 `configure '--build' 'i486-linux-gnu' '--host' 'i486-linux-gnu'
 '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib'
 '--localstatedir=/var' '--infodir=/usr/share/info'
 '--mandir=/usr/share/man' '--with-pop=yes'
 '--enable-locallisppath=/etc/emacs-snapshot:/etc/emacs:/usr/local/share/emacs/24.1.50/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/24.1.50/site-lisp:/usr/share/emacs/site-lisp'
 '--without-compress-info' '--with-crt-dir=/usr/lib/i386-linux-gnu/'
 '--with-x=yes' '--with-x-toolkit=gtk3' '--with-imagemagick=yes'
 'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu'
 'CFLAGS=-DDEBIAN -DSITELOAD_PURESIZE_EXTRA=5000 -g -O2' 'LDFLAGS=-g
 -Wl,--as-needed -znocombreloc' 'CPPFLAGS=-D_FORTIFY_SOURCE=2''

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Drew Adams]

Hi Michael,

I haven't looked at the bug report much, but are you aware of user option
`dired-keep-marker-rename'.  Dunno whether it is involve here, but it generally
controls whether a marked file keeps its mark after renaming.

HTH - Drew

> Background: `wdired-finish-edit' calls `revert-buffer', which will
> call `dired-revert'.  Before this is called, everything is ok.  But
> `dired-revert' fails to remember the mark and the position of the
> renamed file correctly.  I didn't dig any further, but I guess a
> specialized revert function is needed for `wdired-finish-edit'.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Christopher Schmidt]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Background: `wdired-finish-edit' calls `revert-buffer', which will
> call `dired-revert'.  Before this is called, everything is ok.  But
> `dired-revert' fails to remember the mark and the position of the
> renamed file correctly.  I didn't dig any further, but I guess a
> specialized revert function is needed for `wdired-finish-edit'.

This patch for wdired.el fixes the problem here (GNU Emacs 24.1.1).

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired-finish-edit-remeber-marks.diff --]
[-- Type: text/x-diff, Size: 1188 bytes --]

diff --git a/wdired.el b/wdired-fixed.el
index fe5b4ef..d241d2e 100644
--- a/wdired.el
+++ b/wdired-fixed.el
@@ -350,7 +350,8 @@ non-nil means return old filename."
   (interactive)
   (wdired-change-to-dired-mode)
   (let ((changes nil)
-	(errors 0)
+	(errors 0)	
+	(marks (dired-remember-marks (point-min) (point-max)))
 	files-deleted
 	files-renamed
 	some-file-names-unchanged
@@ -376,6 +377,9 @@ non-nil means return old filename."
             (setq changes t)
             (if (not file-new)		;empty filename!
                 (push file-old files-deleted)
+	      (push (cons (substitute-in-file-name file-new)
+			  (cdr (assoc file-old marks)))
+		    marks)
               (push (cons file-old (substitute-in-file-name file-new))
                     files-renamed))))
 	(forward-line -1)))
@@ -393,7 +397,8 @@ non-nil means return old filename."
 		     (= (length files-renamed) 1))
 	    (setq dired-directory (cdr (car files-renamed))))
 	  ;; Re-sort the buffer.
-	  (revert-buffer))
+	  (revert-buffer)
+	  (dired-mark-remembered marks))
       (let ((inhibit-read-only t))
 	(remove-text-properties (point-min) (point-max)
 				'(old-name nil end-name nil old-link nil

[-- Attachment #3: Type: text/plain, Size: 21 bytes --]


        Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Drew Adams]

> > Background: `wdired-finish-edit' calls `revert-buffer', which will
> > call `dired-revert'.  Before this is called, everything is ok.  But
> > `dired-revert' fails to remember the mark and the position of the
> > renamed file correctly.  I didn't dig any further, but I guess a
> > specialized revert function is needed for `wdired-finish-edit'.
> 
> This patch for wdired.el fixes the problem here (GNU Emacs 24.1.1).

But shouldn't Wdired respect the user's setting of option
`dired-keep-marker-rename'?  IOW, shouldn't users be able to control whether
exiting wdired removes marks?

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Christopher Schmidt]

[-- Attachment #1: Type: text/plain, Size: 298 bytes --]

"Drew Adams" <drew.adams@oracle.com> writes:

>> This patch for wdired.el fixes the problem here (GNU Emacs 24.1.1).
>
> But shouldn't Wdired respect the user's setting of option
> `dired-keep-marker-rename'?  IOW, shouldn't users be able to control
> whether exiting wdired removes marks?

Right.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired-finish-edit-remember-marks.diff --]
[-- Type: text/x-diff, Size: 1314 bytes --]

diff --git a/wdired.el b/wdired-fixed.el
index fe5b4ef..c5956c5 100644
--- a/wdired.el
+++ b/wdired-fixed.el
@@ -351,6 +351,7 @@ non-nil means return old filename."
   (wdired-change-to-dired-mode)
   (let ((changes nil)
 	(errors 0)
+	(marks (dired-remember-marks (point-min) (point-max)))
 	files-deleted
 	files-renamed
 	some-file-names-unchanged
@@ -376,6 +377,12 @@ non-nil means return old filename."
             (setq changes t)
             (if (not file-new)		;empty filename!
                 (push file-old files-deleted)
+	      (when dired-keep-marker-rename
+		(push (cons (substitute-in-file-name file-new)
+			    (if (integerp dired-keep-marker-rename)
+				dired-keep-marker-rename
+			      (cdr (assoc file-old marks))))
+		      marks))
               (push (cons file-old (substitute-in-file-name file-new))
                     files-renamed))))
 	(forward-line -1)))
@@ -393,7 +400,9 @@ non-nil means return old filename."
 		     (= (length files-renamed) 1))
 	    (setq dired-directory (cdr (car files-renamed))))
 	  ;; Re-sort the buffer.
-	  (revert-buffer))
+	  (revert-buffer)
+	  (let ((inhibit-read-only t))
+	    (dired-mark-remembered marks)))
       (let ((inhibit-read-only t))
 	(remove-text-properties (point-min) (point-max)
 				'(old-name nil end-name nil old-link nil

[-- Attachment #3: Type: text/plain, Size: 21 bytes --]


        Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Drew Adams]

> >> This patch for wdired.el fixes the problem here (GNU Emacs 24.1.1).
> >
> > But shouldn't Wdired respect the user's setting of option
> > `dired-keep-marker-rename'?  IOW, shouldn't users be able to control
> > whether exiting wdired removes marks?
> 
> Right.

Looks OK to me.

A question is whether we should bother to invoke `dired-remember-marks' if
`dired-keep-marker-rename' is nil.

This patch is local to `wdired-finish-edit', which I think is intended to be
more or less atomic (a transaction), so I guess the answer is no (a user cannot
change the option value during `wdired-finish-edit').

Not calling `dired-remember-marks' if `dired-keep-marker-rename' is nil would
save a little time (not important), and I don't think we would lose anything
doing that.  But it's a question.

BTW, I wonder if there isn't some part of `wdired-finish-edit' that should be in
an `unwind-protect'.  Not thinking of anything in particular - just a feeling...

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hi Christopher,

the patch indeed fixes the problem, thanks!

But there is a problem: if I now rename a file that isn't marked, I get the
following error:

Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  dired-mark-remembered((("/home/micha/Treasure/Trash/bug")))
  (let ((inhibit-read-only t)) (dired-mark-remembered marks))
  (progn (when (and (stringp dired-directory) (not (file-directory-p dired-directory)) (null some-file-names-unchanged) (= (length files-renamed) 1)) (setq dired-directory (cdr (car files-renamed)))) (revert-buffer) (let ((inhibit-read-only t)) (dired-mark-remembered marks)))
  (if changes (progn (when (and (stringp dired-directory) (not (file-directory-p dired-directory)) (null some-file-names-unchanged) (= (length files-renamed) 1)) (setq dired-directory (cdr (car files-renamed)))) (revert-buffer) (let ((inhibit-read-only t)) (dired-mark-remembered marks))) (let ((inhibit-read-only t)) (remove-text-properties (point-min) (point-max) (quote (old-name nil end-name nil old-link nil end-link nil end-perm nil old-perm nil perm-changed nil))) (message "(No changes to be performed)")))
  (let ((changes nil) (errors 0) (marks (dired-remember-marks (point-min) (point-max))) files-deleted files-renamed some-file-names-unchanged file-old file-new tmp-value) (save-excursion (when (and wdired-allow-to-redirect-links (fboundp (quote make-symbolic-link))) (setq tmp-value (wdired-do-symlink-changes)) (setq errors (cdr tmp-value)) (setq changes (car tmp-value))) (when (and wdired-allow-to-change-permissions (boundp (quote wdired-col-perm))) (setq tmp-value (wdired-do-perm-changes)) (setq errors (+ errors (cdr tmp-value))) (setq changes (or changes (car tmp-value)))) (goto-char (point-max)) (while (not (bobp)) (setq file-old (wdired-get-filename nil t)) (when file-old (setq file-new (wdired-get-filename)) (if (equal file-new file-old) (setq some-file-names-unchanged t) (setq changes t) (if (not file-new) (push file-old files-deleted) (when dired-keep-marker-rename (push ... marks)) (push (cons file-old ...) files-renamed)))) (forward-line -1))) (when files-renamed (setq errors (+ errors (wdired-do-renames files-renamed)))) (if changes (progn (when (and (stringp dired-directory) (not (file-directory-p dired-directory)) (null some-file-names-unchanged) (= (length files-renamed) 1)) (setq dired-directory (cdr (car files-renamed)))) (revert-buffer) (let ((inhibit-read-only t)) (dired-mark-remembered marks))) (let ((inhibit-read-only t)) (remove-text-properties (point-min) (point-max) (quote (old-name nil end-name nil old-link nil end-link nil end-perm nil old-perm nil perm-changed nil))) (message "(No changes to be performed)"))) (when files-deleted (wdired-flag-for-deletion files-deleted)) (when (> errors 0) (dired-log-summary (format "%d rename actions failed" errors) nil)))
  wdired-finish-edit()
  call-interactively(wdired-finish-edit nil nil)

`dired-mark-remembered' does not accept files that are not marked, see
the docstring.  I guess this can easily be fixed by just removing those
from the alist before calling `dired-mark-remembered'.


Regards,

Michael.


> diff --git a/wdired.el b/wdired-fixed.el
> index fe5b4ef..c5956c5 100644
> --- a/wdired.el
> +++ b/wdired-fixed.el
> @@ -351,6 +351,7 @@ non-nil means return old filename."
>    (wdired-change-to-dired-mode)
>    (let ((changes nil)
>  	(errors 0)
> +	(marks (dired-remember-marks (point-min) (point-max)))
>  	files-deleted
>  	files-renamed
>  	some-file-names-unchanged
> @@ -376,6 +377,12 @@ non-nil means return old filename."
>              (setq changes t)
>              (if (not file-new)		;empty filename!
>                  (push file-old files-deleted)
> +	      (when dired-keep-marker-rename
> +		(push (cons (substitute-in-file-name file-new)
> +			    (if (integerp dired-keep-marker-rename)
> +				dired-keep-marker-rename
> +			      (cdr (assoc file-old marks))))
> +		      marks))
>                (push (cons file-old (substitute-in-file-name file-new))
>                      files-renamed))))
>  	(forward-line -1)))
> @@ -393,7 +400,9 @@ non-nil means return old filename."
>  		     (= (length files-renamed) 1))
>  	    (setq dired-directory (cdr (car files-renamed))))
>  	  ;; Re-sort the buffer.
> -	  (revert-buffer))
> +	  (revert-buffer)
> +	  (let ((inhibit-read-only t))
> +	    (dired-mark-remembered marks)))
>        (let ((inhibit-read-only t))
>  	(remove-text-properties (point-min) (point-max)
>  				'(old-name nil end-name nil old-link nil
>
>
>         Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Christopher Schmidt]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> But there is a problem: if I now rename a file that isn't marked, I
> get the following error:
[...]
> `dired-mark-remembered' does not accept files that are not marked, see
> the docstring.  I guess this can easily be fixed by just removing
> those from the alist before calling `dired-mark-remembered'.

Here is a fix for the fix.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired-finish-edit-remember-marks-fixed.diff --]
[-- Type: text/x-diff, Size: 1353 bytes --]

diff --git a/wdired.el b/wdired-fixed-fixed.el
index fe5b4ef..045fe91 100644
--- a/wdired.el
+++ b/wdired-fixed-fixed.el
@@ -351,6 +351,7 @@ non-nil means return old filename."
   (wdired-change-to-dired-mode)
   (let ((changes nil)
 	(errors 0)
+	(marks (dired-remember-marks (point-min) (point-max)))
 	files-deleted
 	files-renamed
 	some-file-names-unchanged
@@ -376,6 +377,12 @@ non-nil means return old filename."
             (setq changes t)
             (if (not file-new)		;empty filename!
                 (push file-old files-deleted)
+	      (when dired-keep-marker-rename
+		(let ((mark (if (integerp dired-keep-marker-rename)
+				dired-keep-marker-rename
+			      (cdr (assoc file-old marks)))))
+		  (when mark
+		    (push (cons (substitute-in-file-name file-new) mark) marks))))
               (push (cons file-old (substitute-in-file-name file-new))
                     files-renamed))))
 	(forward-line -1)))
@@ -393,7 +400,9 @@ non-nil means return old filename."
 		     (= (length files-renamed) 1))
 	    (setq dired-directory (cdr (car files-renamed))))
 	  ;; Re-sort the buffer.
-	  (revert-buffer))
+	  (revert-buffer)
+	  (let ((inhibit-read-only t))
+	    (dired-mark-remembered marks)))
       (let ((inhibit-read-only t))
 	(remove-text-properties (point-min) (point-max)
 				'(old-name nil end-name nil old-link nil

[-- Attachment #3: Type: text/plain, Size: 21 bytes --]


        Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hi Christopher,

thanks for working on this.

Your patch looks and works quite well so far - but one more detail:

-	  (revert-buffer))
+	  (revert-buffer)
+	  (let ((inhibit-read-only t))
+	    (dired-mark-remembered marks)))

The call to `dired-mark-remembered' should be wrapped in a
`save-excursion', else it may move point to another position.


Regards,

Michael

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

[-- Attachment #1: Type: text/plain, Size: 170 bytes --]

Hello again,

> The call to `dired-mark-remembered' should be wrapped in a
> `save-excursion', else it may move point to another position.

So, this is the fixed patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired-finish-edit-remember-marks-fixed.diff --]
[-- Type: text/x-diff, Size: 1904 bytes --]

diff -c /home/micha/today/wdired-stuff/wdired.el /home/micha/today/wdired-stuff/wdired-orig.el
*** /home/micha/today/wdired-stuff/wdired.el	2012-07-01 04:32:46.000000000 +0200
--- /home/micha/today/wdired-stuff/wdired-orig.el	2012-07-01 04:48:03.000000000 +0200
***************
*** 351,357 ****
    (wdired-change-to-dired-mode)
    (let ((changes nil)
  	(errors 0)
- 	(marks (dired-remember-marks (point-min) (point-max)))
  	files-deleted
  	files-renamed
  	some-file-names-unchanged
--- 351,356 ----
***************
*** 377,390 ****
              (setq changes t)
              (if (not file-new)		;empty filename!
                  (push file-old files-deleted)
- 	      (when dired-keep-marker-rename
- 		(let ((mark (if (integerp dired-keep-marker-rename)
- 				dired-keep-marker-rename
- 			      (cdr (assoc file-old marks)))))
- 		  (when mark
- 		    (push (cons (expand-file-name
-                                  (substitute-in-file-name file-new)) mark)
-                           marks))))
                (push (cons file-old (substitute-in-file-name file-new))
                      files-renamed))))
  	(forward-line -1)))
--- 376,381 ----
***************
*** 402,410 ****
  		     (= (length files-renamed) 1))
  	    (setq dired-directory (cdr (car files-renamed))))
  	  ;; Re-sort the buffer.
! 	  (revert-buffer)
! 	  (let ((inhibit-read-only t))
! 	    (save-excursion (dired-mark-remembered marks))))
        (let ((inhibit-read-only t))
  	(remove-text-properties (point-min) (point-max)
  				'(old-name nil end-name nil old-link nil
--- 393,399 ----
  		     (= (length files-renamed) 1))
  	    (setq dired-directory (cdr (car files-renamed))))
  	  ;; Re-sort the buffer.
! 	  (revert-buffer))
        (let ((inhibit-read-only t))
  	(remove-text-properties (point-min) (point-max)
  				'(old-name nil end-name nil old-link nil

Diff finished.  Tue Jul  3 02:02:01 2012

[-- Attachment #3: Type: text/plain, Size: 131 bytes --]


I made a lot of tests, and it seems to work well now.  Please, somebody
have a look, and apply it if it's ok.


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

 >> The call to `dired-mark-remembered' should be wrapped in a
 >> `save-excursion', else it may move point to another position.

Can someone explain why `dired-mark-remembered' cannot do:

(defun dired-mark-remembered (alist)
   "Mark all files remembered in ALIST.
Each element of ALIST looks like (FILE . MARKERCHAR)."
   (let (elt fil chr)
     (save-excursion
       (while alist
	(setq elt (car alist)
	      alist (cdr alist)
	      fil (car elt)
	      chr (cdr elt))
	(when (dired-goto-file fil)
	  (beginning-of-line)
	  (delete-char 1)
	  (insert chr))))))

martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Stefan Monnier]

>>> The call to `dired-mark-remembered' should be wrapped in a
>>> `save-excursion', else it may move point to another position.
> Can someone explain why `dired-mark-remembered' cannot do:

> (defun dired-mark-remembered (alist)
>   "Mark all files remembered in ALIST.
> Each element of ALIST looks like (FILE . MARKERCHAR)."
>   (let (elt fil chr)
>     (save-excursion
>       (while alist
> 	(setq elt (car alist)
> 	      alist (cdr alist)
> 	      fil (car elt)
> 	      chr (cdr elt))
> 	(when (dired-goto-file fil)
> 	  (beginning-of-line)
> 	  (delete-char 1)
> 	  (insert chr))))))

I have no idea, but you could rewrite the above as:

  (require 'pcase)
  (defun dired-mark-remembered (alist)
    "Mark all files remembered in ALIST.
  Each element of ALIST looks like (FILE . MARKERCHAR)."
    (save-excursion
      (pcase-dolist (`(,fil . ,chr) alist)
        (when (dired-goto-file fil)
          (beginning-of-line)
          (delete-char 1)
          (insert chr)))))


-- Stefan "official pcase promoter"

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

martin rudalics <rudalics@gmx.at> writes:

> >> The call to `dired-mark-remembered' should be wrapped in a
> >> `save-excursion', else it may move point to another position.
>
> Can someone explain why `dired-mark-remembered' cannot do:
>
> (defun dired-mark-remembered (alist)
>   "Mark all files remembered in ALIST.
> Each element of ALIST looks like (FILE . MARKERCHAR)."
>   (let (elt fil chr)
>     (save-excursion
>       (while alist
> 	(setq elt (car alist)
> 	      alist (cdr alist)
> 	      fil (car elt)
> 	      chr (cdr elt))
> 	(when (dired-goto-file fil)
> 	  (beginning-of-line)
> 	  (delete-char 1)
> 	  (insert chr))))))
>
> martin

I guess nobody can explain it because your version simply is better and
more correct.

So, it would be cool if the patch could be applied.


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

 > So, it would be cool if the patch could be applied.

I hopefully applied the change now.  Please check whether it DTRT.

Thanks, martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Christopher Schmidt]

martin rudalics <rudalics@gmx.at> writes:
> I hopefully applied the change now.  Please check whether it DTRT.

It looks like the first two hunks of Michael's patch were not applied.
(The patch looks fishy - it actually removes the new code rather than
adding it.)

        Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Christopher Schmidt <christopher@ch.ristopher.com> writes:

> martin rudalics <rudalics@gmx.at> writes:
> > I hopefully applied the change now.  Please check whether it DTRT.
>
> It looks like the first two hunks of Michael's patch were not applied.
> (The patch looks fishy - it actually removes the new code rather than
> adding it.)

Actually, nothing was applied at all to wdired.el.

Martin, please wait a moment.  I'll prepare a patch that DTRT.


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

> Actually, nothing was applied at all to wdired.el.
> 
> Martin, please wait a moment.  I'll prepare a patch that DTRT.

Probably my bad.  Take all the time you need to DTRT.

martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

> Actually, nothing was applied at all to wdired.el.
> 
> Martin, please wait a moment.  I'll prepare a patch that DTRT.

Any chances for a fix to this in the next days?

martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

martin rudalics <rudalics@gmx.at> writes:

> > Martin, please wait a moment.  I'll prepare a patch that DTRT.
>
> Any chances for a fix to this in the next days?

Martin, I would rather like to discuss first what we do with bug#12394.
Can you please have a look?  It would be good to know what we want to
change for that bug before applying a patch for this.


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

 > Martin, I would rather like to discuss first what we do with bug#12394.
 > Can you please have a look?  It would be good to know what we want to
 > change for that bug before applying a patch for this.

IIRC `wdired' is full of such niceties.  Here the problem is that
`dired-move-to-filename' moves to the start of the old filename,
skipping the prepended string.  Maybe it's sufficient to remove that
call as in the patch below since the subsequent

(search-forward (wdired-get-filename t) nil t)

should move to the right position anyway.  If not, we'll have to rewrite
this function from scratch.

martin

*** lisp/wdired.el	2012-08-05 04:19:21 +0000
--- lisp/wdired.el	2012-10-05 06:36:43 +0000
***************
*** 506,512 ****
               (progn
                 (setq done t)
                 (let ((inhibit-read-only t))
!                 (dired-move-to-filename)
                   (search-forward (wdired-get-filename t) nil t)
                   (replace-match (file-name-nondirectory filename-ori) t t))
                 (dired-do-create-files-regexp
--- 506,512 ----
               (progn
                 (setq done t)
                 (let ((inhibit-read-only t))
! ;;                 (dired-move-to-filename)
                   (search-forward (wdired-get-filename t) nil t)
                   (replace-match (file-name-nondirectory filename-ori) t t))
                 (dired-do-create-files-regexp

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Hello Martin,

> Any chances for a fix to this in the next days?

Sorry for the delay.

The patch we had so far for this had several problems:

- It sometimes failed to collect file marks because it did that when the
  buffer had already been modified.

- It failed to restore the marks of files that couldn't be renamed
  (e.g. in read-only dirs)

- It didn't propagate the name changes to other dired buffers

I created a new fix that should take care of these problems.  I wanted
to make some more sanity tests today, unfortunately I'm ill and don't
feel well.  I'll try to do this tomorrow.

So, this is the new patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired.patch --]
[-- Type: text/x-diff, Size: 6394 bytes --]

*** /home/micha/today/wdired.el	2012-10-09 02:27:03.638708806 +0200
--- /home/micha/today/wdired-new.el	2012-10-09 02:24:13.498706008 +0200
***************
*** 180,186 ****
  (defvar wdired-col-perm) ;; Column where the permission bits start
  (defvar wdired-old-content)
  (defvar wdired-old-point)
! 
  
  (defun wdired-mode ()
    "Writable Dired (WDired) mode.
--- 180,186 ----
  (defvar wdired-col-perm) ;; Column where the permission bits start
  (defvar wdired-old-content)
  (defvar wdired-old-point)
! (defvar wdired-old-marks)
  
  (defun wdired-mode ()
    "Writable Dired (WDired) mode.
***************
*** 221,226 ****
--- 221,228 ----
      (error "Not a Dired buffer"))
    (set (make-local-variable 'wdired-old-content)
         (buffer-substring (point-min) (point-max)))
+   (set (make-local-variable 'wdired-old-marks)
+        (dired-remember-marks (point-min) (point-max)))
    (set (make-local-variable 'wdired-old-point) (point))
    (set (make-local-variable 'query-replace-skip-read-only) t)
    (set (make-local-variable 'isearch-filter-predicate)
***************
*** 430,477 ****
        (let* ((rename (pop renames))
               (file-new (cdr rename)))
          (cond
!          ((rassoc file-new renames)
!           (error "Trying to rename 2 files to the same name"))
!          ((assoc file-new renames)
!           ;; Renaming to a file name that already exists but will itself be
!           ;; renamed as well.  Let's wait until that one gets renamed.
!           (push rename residue))
!          ((and (assoc file-new residue)
!                ;; Make sure the file really exists: if it doesn't it's
!                ;; not really a conflict.  It might be a temp-file generated
!                ;; specifically to break a circular renaming.
!                (file-exists-p file-new))
!           ;; Renaming to a file name that already exists, needed to be renamed,
!           ;; but whose renaming could not be performed right away.
!           (if (or progress renames)
!               ;; There's still a chance the conflict will be resolved.
!               (push rename residue)
!             ;; We have not made any progress and we've reached the end of
!             ;; the renames, so we really have a circular conflict, and we
!             ;; have to forcefully break the cycle.
!             (message "Circular renaming: using temporary file name")
!             (let ((tmp (make-temp-name file-new)))
!               (push (cons (car rename) tmp) renames)
!               (push (cons tmp file-new) residue))))
!          (t
!           (setq progress t)
!           (let ((file-ori (car rename)))
!             (if wdired-use-interactive-rename
!                 (wdired-search-and-rename file-ori file-new)
!               ;; If dired-rename-file autoloads dired-aux while
!               ;; dired-backup-overwrite is locally bound,
!               ;; dired-backup-overwrite won't be initialized.
!               ;; So we must ensure dired-aux is loaded.
!               (require 'dired-aux)
!               (condition-case err
!                   (let ((dired-backup-overwrite nil))
!                     (dired-rename-file file-ori file-new
!                                        overwrite))
!                 (error
!                  (setq errors (1+ errors))
!                  (dired-log (concat "Rename `" file-ori "' to `"
!                                     file-new "' failed:\n%s\n")
!                             err)))))))))
      errors))
  
  
--- 432,485 ----
        (let* ((rename (pop renames))
               (file-new (cdr rename)))
          (cond
!           ((rassoc file-new renames)
!            (error "Trying to rename 2 files to the same name"))
!           ((assoc file-new renames)
!            ;; Renaming to a file name that already exists but will itself be
!            ;; renamed as well.  Let's wait until that one gets renamed.
!            (push rename residue))
!           ((and (assoc file-new residue)
!                 ;; Make sure the file really exists: if it doesn't it's
!                 ;; not really a conflict.  It might be a temp-file generated
!                 ;; specifically to break a circular renaming.
!                 (file-exists-p file-new))
!            ;; Renaming to a file name that already exists, needed to be renamed,
!            ;; but whose renaming could not be performed right away.
!            (if (or progress renames)
!                ;; There's still a chance the conflict will be resolved.
!                (push rename residue)
!              ;; We have not made any progress and we've reached the end of
!              ;; the renames, so we really have a circular conflict, and we
!              ;; have to forcefully break the cycle.
!              (message "Circular renaming: using temporary file name")
!              (let ((tmp (make-temp-name file-new)))
!                (push (cons (car rename) tmp) renames)
!                (push (cons tmp file-new) residue))))
!           (t
!            (setq progress t)
!            (let* ((file-ori (car rename))
!                   (old-mark (cdr (assoc file-ori wdired-old-marks))))
!              (if wdired-use-interactive-rename
!                  (wdired-search-and-rename file-ori file-new)
!                ;; If dired-rename-file autoloads dired-aux while
!                ;; dired-backup-overwrite is locally bound,
!                ;; dired-backup-overwrite won't be initialized.
!                ;; So we must ensure dired-aux is loaded.
!                (require 'dired-aux)
!                (condition-case err
!                    (let ((dired-backup-overwrite nil))
!                      (dired-rename-file file-ori file-new
!                                         overwrite)
!                      (dired-remove-file file-ori)
!                      (dired-add-file file-new (if (integerp dired-keep-marker-rename)
!                                                   dired-keep-marker-rename
!                                                 old-mark)))
!                  (error
!                   (setq errors (1+ errors))
!                   (dired-log (concat "Rename `" file-ori "' to `"
!                                      file-new "' failed:\n%s\n")
!                              err)
!                   (dired-add-entry file-ori old-mark)))))))))
      errors))
  
  

[-- Attachment #3: Type: text/plain, Size: 21 bytes --]



Regards,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

[-- Attachment #1: Type: text/plain, Size: 188 bytes --]

martin rudalics <rudalics@gmx.at> writes:

> Any chances for a fix to this in the next days?

After performing different tests, my patch looks sane so far.  IMHO it
can be installed now.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: wdired.patch --]
[-- Type: text/x-diff, Size: 6394 bytes --]

*** /home/micha/today/wdired.el	2012-10-09 02:27:03.638708806 +0200
--- /home/micha/today/wdired-new.el	2012-10-09 02:24:13.498706008 +0200
***************
*** 180,186 ****
  (defvar wdired-col-perm) ;; Column where the permission bits start
  (defvar wdired-old-content)
  (defvar wdired-old-point)
! 
  
  (defun wdired-mode ()
    "Writable Dired (WDired) mode.
--- 180,186 ----
  (defvar wdired-col-perm) ;; Column where the permission bits start
  (defvar wdired-old-content)
  (defvar wdired-old-point)
! (defvar wdired-old-marks)
  
  (defun wdired-mode ()
    "Writable Dired (WDired) mode.
***************
*** 221,226 ****
--- 221,228 ----
      (error "Not a Dired buffer"))
    (set (make-local-variable 'wdired-old-content)
         (buffer-substring (point-min) (point-max)))
+   (set (make-local-variable 'wdired-old-marks)
+        (dired-remember-marks (point-min) (point-max)))
    (set (make-local-variable 'wdired-old-point) (point))
    (set (make-local-variable 'query-replace-skip-read-only) t)
    (set (make-local-variable 'isearch-filter-predicate)
***************
*** 430,477 ****
        (let* ((rename (pop renames))
               (file-new (cdr rename)))
          (cond
!          ((rassoc file-new renames)
!           (error "Trying to rename 2 files to the same name"))
!          ((assoc file-new renames)
!           ;; Renaming to a file name that already exists but will itself be
!           ;; renamed as well.  Let's wait until that one gets renamed.
!           (push rename residue))
!          ((and (assoc file-new residue)
!                ;; Make sure the file really exists: if it doesn't it's
!                ;; not really a conflict.  It might be a temp-file generated
!                ;; specifically to break a circular renaming.
!                (file-exists-p file-new))
!           ;; Renaming to a file name that already exists, needed to be renamed,
!           ;; but whose renaming could not be performed right away.
!           (if (or progress renames)
!               ;; There's still a chance the conflict will be resolved.
!               (push rename residue)
!             ;; We have not made any progress and we've reached the end of
!             ;; the renames, so we really have a circular conflict, and we
!             ;; have to forcefully break the cycle.
!             (message "Circular renaming: using temporary file name")
!             (let ((tmp (make-temp-name file-new)))
!               (push (cons (car rename) tmp) renames)
!               (push (cons tmp file-new) residue))))
!          (t
!           (setq progress t)
!           (let ((file-ori (car rename)))
!             (if wdired-use-interactive-rename
!                 (wdired-search-and-rename file-ori file-new)
!               ;; If dired-rename-file autoloads dired-aux while
!               ;; dired-backup-overwrite is locally bound,
!               ;; dired-backup-overwrite won't be initialized.
!               ;; So we must ensure dired-aux is loaded.
!               (require 'dired-aux)
!               (condition-case err
!                   (let ((dired-backup-overwrite nil))
!                     (dired-rename-file file-ori file-new
!                                        overwrite))
!                 (error
!                  (setq errors (1+ errors))
!                  (dired-log (concat "Rename `" file-ori "' to `"
!                                     file-new "' failed:\n%s\n")
!                             err)))))))))
      errors))
  
  
--- 432,485 ----
        (let* ((rename (pop renames))
               (file-new (cdr rename)))
          (cond
!           ((rassoc file-new renames)
!            (error "Trying to rename 2 files to the same name"))
!           ((assoc file-new renames)
!            ;; Renaming to a file name that already exists but will itself be
!            ;; renamed as well.  Let's wait until that one gets renamed.
!            (push rename residue))
!           ((and (assoc file-new residue)
!                 ;; Make sure the file really exists: if it doesn't it's
!                 ;; not really a conflict.  It might be a temp-file generated
!                 ;; specifically to break a circular renaming.
!                 (file-exists-p file-new))
!            ;; Renaming to a file name that already exists, needed to be renamed,
!            ;; but whose renaming could not be performed right away.
!            (if (or progress renames)
!                ;; There's still a chance the conflict will be resolved.
!                (push rename residue)
!              ;; We have not made any progress and we've reached the end of
!              ;; the renames, so we really have a circular conflict, and we
!              ;; have to forcefully break the cycle.
!              (message "Circular renaming: using temporary file name")
!              (let ((tmp (make-temp-name file-new)))
!                (push (cons (car rename) tmp) renames)
!                (push (cons tmp file-new) residue))))
!           (t
!            (setq progress t)
!            (let* ((file-ori (car rename))
!                   (old-mark (cdr (assoc file-ori wdired-old-marks))))
!              (if wdired-use-interactive-rename
!                  (wdired-search-and-rename file-ori file-new)
!                ;; If dired-rename-file autoloads dired-aux while
!                ;; dired-backup-overwrite is locally bound,
!                ;; dired-backup-overwrite won't be initialized.
!                ;; So we must ensure dired-aux is loaded.
!                (require 'dired-aux)
!                (condition-case err
!                    (let ((dired-backup-overwrite nil))
!                      (dired-rename-file file-ori file-new
!                                         overwrite)
!                      (dired-remove-file file-ori)
!                      (dired-add-file file-new (if (integerp dired-keep-marker-rename)
!                                                   dired-keep-marker-rename
!                                                 old-mark)))
!                  (error
!                   (setq errors (1+ errors))
!                   (dired-log (concat "Rename `" file-ori "' to `"
!                                      file-new "' failed:\n%s\n")
!                              err)
!                   (dired-add-entry file-ori old-mark)))))))))
      errors))
  
  

[-- Attachment #3: Type: text/plain, Size: 11 bytes --]



Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

>> Any chances for a fix to this in the next days?
> 
> After performing different tests, my patch looks sane so far.  IMHO it
> can be installed now.

Christopher can you check Michael's patch?  Pretty please.

Thanks, martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

> After performing different tests, my patch looks sane so far.  IMHO it
> can be installed now.

Installed as revision 110564 on trunk.  Bug closed.

Thanks, martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Christopher Schmidt]

martin rudalics <rudalics@gmx.at> writes:
> Christopher can you check Michael's patch?  Pretty please.
>
> Thanks, martin

martin rudalics <rudalics@gmx.at> writes:
>> After performing different tests, my patch looks sane so far.  IMHO
>> it can be installed now.
>
> Installed as revision 110564 on trunk.  Bug closed.

FWIW I did some very basic testing yesterday.  Everything looked fine to
me.

Michael, Martin, thanks for your work on this issue.

        Christopher

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[martin rudalics]

> FWIW I did some very basic testing yesterday.  Everything looked fine to
> me.
> 
> Michael, Martin, thanks for your work on this issue.

Thanks to you for testing.

martin

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> But shouldn't Wdired respect the user's setting of option
> `dired-keep-marker-rename'?  IOW, shouldn't users be able to control whether
> exiting wdired removes marks?

When `dired-keep-marker-rename' is customized to a character like ?R or ?M,
now `wdired-finish-edit' adds a new marker on the files renamed by wdired.
This is new behavior.  Strictly speaking, it is consistent with the meaning
of `dired-keep-marker-rename', so I'm not sure if a new option is necessary
to control this.  But at least when someone wants to put markers only on the
files moved by `dired-do-rename', but not on the files renamed by wdired,
this is still possible to do by adding to ~/.emacs something like:

  (defadvice wdired-finish-edit (around my-wdired-finish-edit act)
    (let ((dired-keep-marker-rename t))
      ad-do-it))

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Juri Linkov <juri@jurta.org> writes:

> > But shouldn't Wdired respect the user's setting of option
> > `dired-keep-marker-rename'?  IOW, shouldn't users be able to control
> > whether
> > exiting wdired removes marks?
>
> When `dired-keep-marker-rename' is customized to a character like ?R or ?M,
> now `wdired-finish-edit' adds a new marker on the files renamed by wdired.
> This is new behavior.  Strictly speaking, it is consistent with the meaning
> of `dired-keep-marker-rename', so I'm not sure if a new option is necessary
> to control this.  But at least when someone wants to put markers only on the
> files moved by `dired-do-rename', but not on the files renamed by wdired,
> this is still possible to do by adding to ~/.emacs something like:
>
>   (defadvice wdired-finish-edit (around my-wdired-finish-edit act)
>     (let ((dired-keep-marker-rename t))
>       ad-do-it))

I think you're right, some people may want to have a behavior different
than that defined by `dired-keep-marker-rename'.  And we should avoid
the necessity for users to use something nontrivial like `defadvice' to
control this.

So, introducing an new option `wdired-keep-marker-rename' would IMHO make
sense.  The default value could be t, at least that's what I think would
be most intuitive.


Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> So, introducing an new option `wdired-keep-marker-rename' would IMHO make
> sense.  The default value could be t, at least that's what I think would
> be most intuitive.

Yes, I agree that introducing `wdired-keep-marker-rename' would be
the best way to fix this.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> So, introducing an new option `wdired-keep-marker-rename' would IMHO make
> sense.  The default value could be t, at least that's what I think would
> be most intuitive.

I fixed this regression by adding `wdired-keep-marker-rename'.
Thanks for the suggestion.

When testing it, I noticed a random behavior of `wdired-do-renames' when
the marker is customized to a character.  The decision what character to put
depends on the position of point.  When point is at the top of the Dired
buffer, it uses a customized marker character.  When point is at the bottom,
it restores an old marker character.

The problem is that `dired-remove-file' doesn't remove file entries
(and neither `dired-rename-file') because files already have new renamed names
in the Dired buffer.  Whereas `dired-add-entry' adds duplicated file entries.

Fortunately, `revert-buffer' (called at the end of `wdired-finish-edit')
sorts out this mess caused by `dired-add-entry'.  But in `dired-revert'
it uses `dired-remember-marks' and `dired-restore-positions' to restore
markers.  So when duplicated file entries were added by `dired-add-entry'
at the top of the Dired buffer (in front of file entries edited by WDired),
it uses a customized marker character.  When duplicated file entries
were added at the bottom, it restores an old marker character because
`dired-restore-positions' in `dired-revert' gets markers from the
first duplicated file entry.

Do you have an idea how to fix this problem?  One way would be
instead of using `dired-add-file' to change markers in the remembered
variable `wdired-old-marks' and just call `dired-mark-remembered'
(before `revert-buffer' in `wdired-finish-edit') with the modified
list of markers for renamed files.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hello Juri,

> > So, introducing an new option `wdired-keep-marker-rename' would IMHO make
> > sense.  The default value could be t, at least that's what I think would
> > be most intuitive.
>
> I fixed this regression by adding `wdired-keep-marker-rename'.
> Thanks for the suggestion.

Good - thanks!

> When testing it, I noticed a random behavior of `wdired-do-renames'
> when the marker is customized to a character.  The decision what
> character to put depends on the position of point.  When point is at
> the top of the Dired buffer, it uses a customized marker character.
> When point is at the bottom, it restores an old marker character.
>
> The problem is that `dired-remove-file' doesn't remove file entries
> (and neither `dired-rename-file') because files already have new
> renamed names
> in the Dired buffer.  Whereas `dired-add-entry' adds duplicated file
> entries.
>
> Fortunately, `revert-buffer' (called at the end of
> `wdired-finish-edit') sorts out this mess caused by `dired-add-entry'.
> But in `dired-revert' it uses `dired-remember-marks' and
> `dired-restore-positions' to restore markers.  So when duplicated file
> entries were added by `dired-add-entry' at the top of the Dired buffer
> (in front of file entries edited by WDired), it uses a customized
> marker character.  When duplicated file entries were added at the
> bottom, it restores an old marker character because
> `dired-restore-positions' in `dired-revert' gets markers from the
> first duplicated file entry.

Thanks for finding this, and for your analysis.

> Do you have an idea how to fix this problem?  One way would be instead
> of using `dired-add-file' to change markers in the remembered variable
> `wdired-old-marks' and just call `dired-mark-remembered' (before
> `revert-buffer' in `wdired-finish-edit') with the modified list of
> markers for renamed files.

I chose to use `dired-add-file' in my patch so that name changes are
propagated also to other dired buffers where these files may also be
shown.  This is consistent with `dired-do-rename'.

WRT your suggestion: I see two problems:

1. Calling `dired-mark-remembered' before `revert-buffer' is not
reliable, because it may fail to get the right names from places where
changes were made.  I.e., it my not work for the same reason as
`dired-remove-file' doesn't work.

2. Even if this worked well, the remaining old lines of renamed files
are still lying around.  They can still cause trouble.  When
`revert-buffer' is called, these lines are still included for
remembering marks.  This is problematic: there is a chance that the user
has renamed another file in that buffer to that collected name.

My suggestion would be to just delete the according lines so that they
cannot cause trouble, simply with `delete-region'.  That should not do
any harm.

What do you think?


Regards,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> My suggestion would be to just delete the according lines so that they
> cannot cause trouble, simply with `delete-region'.  That should not do
> any harm.

I'm afraid `delete-region' will do harm especially at this stage closer
to the release because it needs heavy testing for possible failures.

Wdired worked correctly for a long time without `dired-add-file', so
it's better to avoid using `dired-add-file' and low-level `delete-region'.

`revert-buffer' already does its work well and propagates name changes
to other dired buffers where these files may also be shown.

The original request was about keeping markers of renamed files,
so what is needed is to use just `dired-remember-marks' and
`dired-mark-remembered' (and also another pair of functions
`dired-save-positions' and `dired-restore-positions').

There is no problem of getting the right names from places where
changes were made.  This should be implemented in the same place
in `wdired-do-renames' that currently has information about old and new
file names and their markers.  The same place in `wdired-do-renames'
can modify the remembered list of markers accordingly.
IOW, keep the same logic in `wdired-do-renames' as it has now,
but modify the `wdired-old-marks' variable instead of operating
on the dired buffer.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

reopen 11795
stop

This bug is a regression against Emacs 23.1+.

Actually I found out that Christopher already proposed a patch
that fixes this bug.  I see no reason not to use it.  I propose
to do this on the Emacs-24 release branch before the pretest:
to revert the ill-behaved part of revno:110564 and install
Christopher's patch that works correctly in my tests and
has no bad effects caused by unnecessary direct operations
on the dired buffer.

Below is a complete patch adapted for the Emacs-24 release branch:

=== modified file 'lisp/wdired.el'
--- lisp/wdired.el	2012-10-27 09:17:14 +0000
+++ lisp/wdired.el	2012-11-22 09:09:30 +0000
@@ -399,6 +397,15 @@ (defun wdired-finish-edit ()
             (setq changes t)
             (if (not file-new)		;empty filename!
                 (push file-old files-deleted)
+	      (when wdired-keep-marker-rename
+		(let ((mark (cond ((integerp wdired-keep-marker-rename)
+				   wdired-keep-marker-rename)
+				  (wdired-keep-marker-rename
+				   (cdr (assoc file-old wdired-old-marks)))
+				  (t nil))))
+		  (when mark
+		    (push (cons (substitute-in-file-name file-new) mark)
+			  wdired-old-marks))))
               (push (cons file-old (substitute-in-file-name file-new))
                     files-renamed))))
 	(forward-line -1)))
@@ -416,7 +423,9 @@ (defun wdired-finish-edit ()
 		     (= (length files-renamed) 1))
 	    (setq dired-directory (cdr (car files-renamed))))
 	  ;; Re-sort the buffer.
-	  (revert-buffer))
+	  (revert-buffer)
+	  (let ((inhibit-read-only t))
+	    (dired-mark-remembered wdired-old-marks)))
       (let ((inhibit-read-only t))
 	(remove-text-properties (point-min) (point-max)
 				'(old-name nil end-name nil old-link nil
@@ -430,8 +439,6 @@ (defun wdired-finish-edit ()
   (set-buffer-modified-p nil)
   (setq buffer-undo-list nil))
 
-(declare-function dired-add-entry "dired-aux" (filename &optional marker-char relative))
-
 (defun wdired-do-renames (renames)
   "Perform RENAMES in parallel."
   (let ((residue ())
@@ -473,8 +480,7 @@ (defun wdired-do-renames (renames)
               (push (cons tmp file-new) residue))))
          (t
           (setq progress t)
-          (let* ((file-ori (car rename))
-                 (old-mark (cdr (assoc file-ori wdired-old-marks))))
+          (let* ((file-ori (car rename)))
             (if wdired-use-interactive-rename
                 (wdired-search-and-rename file-ori file-new)
               ;; If dired-rename-file autoloads dired-aux while
@@ -485,20 +491,12 @@ (defun wdired-do-renames (renames)
               (condition-case err
                   (let ((dired-backup-overwrite nil))
                     (dired-rename-file file-ori file-new
-                                       overwrite)
-                    (dired-remove-file file-ori)
-                    (dired-add-file
-		     file-new
-		     (cond ((integerp wdired-keep-marker-rename)
-			    wdired-keep-marker-rename)
-			   (wdired-keep-marker-rename old-mark)
-			   (t nil))))
+                                       overwrite))
                 (error
                  (setq errors (1+ errors))
                  (dired-log (concat "Rename `" file-ori "' to `"
                                     file-new "' failed:\n%s\n")
-                            err)
-                 (dired-add-entry file-ori old-mark)))))))))
+                            err)))))))))
     errors))
 
 

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Stefan Monnier]

> Below is a complete patch adapted for the Emacs-24 release branch:

Please install it,


        Stefan

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hello Juri,

> reopen 11795
> stop
>
> This bug is a regression against Emacs 23.1+.
>
> Actually I found out that Christopher already proposed a patch
> that fixes this bug.  I see no reason not to use it.  I propose
> to do this on the Emacs-24 release branch before the pretest:
> to revert the ill-behaved part of revno:110564 and install
> Christopher's patch that works correctly in my tests and
> has no bad effects caused by unnecessary direct operations
> on the dired buffer.

Thanks for your work.

But I see some problems in your patch that I already had fixed in my
version.

1.  When I prepend a letter z to the first file in a buffer, so that it
will be under the last files after reverting, and I hit C-c C-c, then
point is at another file afterwards.  This was an issue of the original
report.

2.  If renamed files are also visible in another dired buffer, they
just disappear there.  That makes no sense.


Regards,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

>> Below is a complete patch adapted for the Emacs-24 release branch:
>
> Please install it,

Installed in the emacs-24 branch.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> 1.  When I prepend a letter z to the first file in a buffer, so that it
> will be under the last files after reverting, and I hit C-c C-c, then
> point is at another file afterwards.  This was an issue of the original
> report.

This is not a regression, so it could be fixed in the trunk
in the same way as for `wdired-old-marks', i.e. to store the original
value returned from `dired-save-positions', changing original filenames
to renamed filenames in the returned value, and calling
`dired-restore-positions' with the new value after `revert-buffer'.
A simple patch below does this.

> 2.  If renamed files are also visible in another dired buffer, they
> just disappear there.  That makes no sense.

The problem where other Dired buffers are not updated
in `wdired-finish-edit' is not a regression too.
WDired worked this way from its first version.

If you want, you could try to rewrite WDired so that it correctly
maintains consistency with renamed files without using `revert-buffer'
and by using `dired-add-file' like `dired-do-rename' does.

Your previous patch doesn't do this correctly - it inserts
duplicate file lines, so Dired displays two versions of the same file,
but fortunately `revert-buffer' fixes this mess.

Instead of this, much simpler would be to keep the current design
of WDired that uses `revert-buffer' to maintain consistency with renames.
Then a simpler fix would be to call `revert-buffer' in all other
Dired buffers that display renamed files by using

  (dired-fun-in-all-buffers dired-directory nil (function revert-buffer))

This patch for the trunk demonstrates how these problems could be fixed.
Do you see more problems with this approach?

=== modified file 'lisp/wdired.el'
--- lisp/wdired.el	2012-11-21 10:29:30 +0000
+++ lisp/wdired.el	2012-11-23 07:28:39 +0000
@@ -194,6 +194,7 @@ (defvar wdired-mode-hook nil
 (defvar wdired-col-perm) ;; Column where the permission bits start
 (defvar wdired-old-content)
 (defvar wdired-old-point)
+(defvar wdired-old-positions)
 (defvar wdired-old-marks)
 
 (defun wdired-mode ()
@@ -237,6 +238,8 @@ (defun wdired-change-to-wdired-mode ()
        (buffer-substring (point-min) (point-max)))
   (set (make-local-variable 'wdired-old-marks)
        (dired-remember-marks (point-min) (point-max)))
+  (set (make-local-variable 'wdired-old-positions)
+       (dired-save-positions))
   (set (make-local-variable 'wdired-old-point) (point))
   (set (make-local-variable 'query-replace-skip-read-only) t)
   (add-hook 'isearch-filter-predicates 'wdired-isearch-filter-read-only nil t)
@@ -406,6 +409,8 @@ (defun wdired-finish-edit ()
 		  (when mark
 		    (push (cons (substitute-in-file-name file-new) mark)
 			  wdired-old-marks))))
+	      (when (equal (cadr (car wdired-old-positions)) file-old)
+		(setcar (cdr (car wdired-old-positions)) file-new))
               (push (cons file-old (substitute-in-file-name file-new))
                     files-renamed))))
 	(forward-line -1)))
@@ -423,9 +428,12 @@ (defun wdired-finish-edit ()
 		     (= (length files-renamed) 1))
 	    (setq dired-directory (cdr (car files-renamed))))
 	  ;; Re-sort the buffer.
-	  (revert-buffer)
+	  (if (stringp dired-directory)
+	      (dired-fun-in-all-buffers dired-directory nil (function revert-buffer))
+	    (revert-buffer))
 	  (let ((inhibit-read-only t))
-	    (dired-mark-remembered wdired-old-marks)))
+	    (dired-mark-remembered wdired-old-marks))
+	  (dired-restore-positions wdired-old-positions))
       (let ((inhibit-read-only t))
 	(remove-text-properties (point-min) (point-max)
 				'(old-name nil end-name nil old-link nil

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> Instead of this, much simpler would be to keep the current design
> of WDired that uses `revert-buffer' to maintain consistency with renames.
> Then a simpler fix would be to call `revert-buffer' in all other
> Dired buffers that display renamed files by using
>
>   (dired-fun-in-all-buffers dired-directory nil (function revert-buffer))

After looking more at this, I noticed a problem that no previous patch
was able to fix.

The question is what to do with marks in other Dired buffers?

One option is to copy marks of renamed files from the current Dired buffer
to other Dired buffers that display the same set of renamed files.
But I think that more correct would be to remember marks in all other
Dired buffers, and in `wdired-finish-edit' restore them in other Dired
buffer to their previous marks.

This means that `wdired-old-marks' should be an associative list
where the key is a Dired buffer and the value is the returned value
from `dired-remember-marks' remembered for each Dired buffer.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

Hello Juri,

sorry, again, for the long delay.

> > 1.  When I prepend a letter z to the first file in a buffer, so that it
> > will be under the last files after reverting, and I hit C-c C-c, then
> > point is at another file afterwards.  This was an issue of the original
> > report.
>
> This is not a regression, so it could be fixed in the trunk
> in the same way as for `wdired-old-marks', i.e. to store the original
> value returned from `dired-save-positions', changing original filenames
> to renamed filenames in the returned value, and calling
> `dired-restore-positions' with the new value after `revert-buffer'.
> A simple patch below does this.

Ok, that seems reasonable to me.

> > 2.  If renamed files are also visible in another dired buffer, they
> > just disappear there.  That makes no sense.
>
> The problem where other Dired buffers are not updated
> in `wdired-finish-edit' is not a regression too.
> WDired worked this way from its first version.
>
> Instead of this, much simpler would be to keep the current design
> of WDired that uses `revert-buffer' to maintain consistency with renames.
> Then a simpler fix would be to call `revert-buffer' in all other
> Dired buffers that display renamed files by using
>
>   (dired-fun-in-all-buffers dired-directory nil (function revert-buffer))
>
> This patch for the trunk demonstrates how these problems could be fixed.
> Do you see more problems with this approach?

I must say that I'm a bit reluctant to force a revert in other buffers.
Because some aspects of the these buffers may be lost that maybe were
important for the user - like killed files (they reappear when
reverting) or any marks (e.g. belonging to positions stored in a
register).  That's why I tried to avoid reverting.

WRT to the marks in other buffers (see your followup
<87sj7yavw7.fsf@mail.jurta.org>) - I agree we should restore (keep) the
marks in those other buffers when renaming files, and not export the
marks form the buffer where wdired was used.


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Juri Linkov]

> I must say that I'm a bit reluctant to force a revert in other buffers.
> Because some aspects of the these buffers may be lost that maybe were
> important for the user - like killed files (they reappear when
> reverting) or any marks (e.g. belonging to positions stored in a
> register).  That's why I tried to avoid reverting.

Not reverting means a lot of trouble.  For example, you have to detect
the situation when a file is in the killed state, so not to add its
renamed file line to the Dired buffer in this case, etc.

A less error-prone approach would be to allow a revert to avoid
all these nuances.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Michael Heerdegen]

> > I must say that I'm a bit reluctant to force a revert in other buffers.
> > Because some aspects of the these buffers may be lost that maybe were
> > important for the user - like killed files (they reappear when
> > reverting) or any marks (e.g. belonging to positions stored in a
> > register).  That's why I tried to avoid reverting.
>
> Not reverting means a lot of trouble.  For example, you have to detect
> the situation when a file is in the killed state, so not to add its
> renamed file line to the Dired buffer in this case, etc.

Agreed.

> A less error-prone approach would be to allow a revert to avoid
> all these nuances.

Ok.  So, do you want to write a patch to implement this?


Thanks,

Michael.

bug#11795: 24.1.50; wdired: C-c C-c loses marks and positions of renamed files

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> a short demonstration for emacs -Q:
>
> 1. M-x dired a directory containing some files
>
> 2. Go to the first file's line (not "." or ".."), and mark the file
> with m
>
> 3. M-x wdired-change-to-wdired-mode
>
> 4. Edit the name of the first file: e.g. add a "z" to the front of the
>    name so that the file will surely appear at another position after
>    renaming
>
> 5. C-c C-c
>
> You'll see that point is still at the first file's line, although the
> renamed file is now displayed near the end of the buffer.  Even worse,
> the mark of the file has disappeared.

A long discussion then followed, which I've only skimmed.  The bug
itself was fixed at the time, though, so I'm closing this bug report.
If there's anything further to be done in this area, a new bug report
could perhaps be opened.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no