bug#15329: saveplace restores dired positions to random places

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

Tags: patch

Remembering dired positions is a good feature, but
unfortunately in its current implementation is useless.

The problem is that often the position of point in Dired
is restored to random places because in Dired buffers
it depends on many irrelevant ever-changing factors
such as the total size of all files in the directory
and available space (thus the line with this information
changes its length), sizes of each file above the restored
file in the Dired buffer also accounts to the incorrect position
of point, also added/deleted files in Dired change the restored
position to random places, different sorting order, etc.

An obvious solution to this problem is to save the
current file name in Dired instead of its point.

It seems saving information other than point doesn't contradict
the purpose of saveplace.el that automatically saves places where
visiting them later automatically moves point to the saved position.
Also elements of `save-place-alist' are (FILENAME . POSITION),
not more limited (FILENAME . POINT).  So saving a string to
POSITION would be consistent with the design of saveplace.el.

=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el	2013-06-14 09:32:01 +0000
+++ lisp/saveplace.el	2013-09-10 20:44:01 +0000
@@ -176,14 +178,17 @@ (defun save-place-to-alist ()
                    (not (string-match save-place-ignore-files-regexp
                                       item))))
       (let ((cell (assoc item save-place-alist))
-            (position (if (not (eq major-mode 'hexl-mode))
-                          (point)
-                        (with-no-warnings
-                          (1+ (hexl-current-address))))))
+            (position (cond ((eq major-mode 'hexl-mode)
+			     (with-no-warnings
+			       (1+ (hexl-current-address))))
+			    ((derived-mode-p 'dired-mode)
+			     (or (dired-get-filename nil t) (point)))
+			    (t (point)))))
         (if cell
             (setq save-place-alist (delq cell save-place-alist)))
         (if (and save-place
-                 (not (= position 1)))  ;; Optimize out the degenerate case.
+                 (not (and (integerp position)
+			   (= position 1)))) ;; Optimize out the degenerate case.
             (setq save-place-alist
                   (cons (cons item position)
                         save-place-alist)))))))
@@ -298,7 +304,8 @@ (defun save-place-find-file-hook ()
     (if cell
 	(progn
 	  (or revert-buffer-in-progress-p
-	      (goto-char (cdr cell)))
+	      (and (integerp (cdr cell))
+		   (goto-char (cdr cell))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 
@@ -309,7 +318,10 @@ (defun save-place-dired-hook ()
     (if cell
         (progn
           (or revert-buffer-in-progress-p
-              (goto-char (cdr cell)))
+              (cond ((integerp (cdr cell))
+		     (goto-char (cdr cell)))
+		    ((stringp (cdr cell))
+		     (dired-goto-file (cdr cell)))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> Remembering dired positions is a good feature, but
> unfortunately in its current implementation is useless.
>
> The problem is that often the position of point in Dired
> is restored to random places because in Dired buffers
> it depends on many irrelevant ever-changing factors
> such as the total size of all files in the directory
> and available space (thus the line with this information
> changes its length), sizes of each file above the restored
> file in the Dired buffer also accounts to the incorrect position
> of point, also added/deleted files in Dired change the restored
> position to random places, different sorting order, etc.
>
> An obvious solution to this problem is to save the
> current file name in Dired instead of its point.
>
> It seems saving information other than point doesn't contradict
> the purpose of saveplace.el that automatically saves places where
> visiting them later automatically moves point to the saved position.
> Also elements of `save-place-alist' are (FILENAME . POSITION),
> not more limited (FILENAME . POINT).  So saving a string to
> POSITION would be consistent with the design of saveplace.el.

I realized this is not backward-compatible change, i.e.
older Emacs versions won't be able to read saved places
in the new format.  Unfortunately, Dired filename positions
can't be added to the current format that uses an alist
with cons pair cells.  I mean (FILENAME . POINT)
can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
because older Emacs versions will fail to read them.

The only solution that I see is to save two alists to the places file:

((FILENAME1 . POINT1)
 (FILENAME2 . POINT2)
 ...)
((FILENAME1 DIRED-FILENAME-POSITION1)
 (FILENAME2 DIRED-FILENAME-POSITION2)
 ...)

Then the current code:

  (with-demoted-errors
    (car (read-from-string
      (buffer-substring (point-min) (point-max)))))

will read the first alist without problems,
and additional code in newer versions like

  (with-demoted-errors
    (cadr (read-from-string
      (buffer-substring (point-min) (point-max)))))

will read the second alist with more information
about the context of saved places (like bookmarks).

Re: bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> Remembering dired positions is a good feature, but
> unfortunately in its current implementation is useless.
>
> The problem is that often the position of point in Dired
> is restored to random places because in Dired buffers
> it depends on many irrelevant ever-changing factors
> such as the total size of all files in the directory
> and available space (thus the line with this information
> changes its length), sizes of each file above the restored
> file in the Dired buffer also accounts to the incorrect position
> of point, also added/deleted files in Dired change the restored
> position to random places, different sorting order, etc.
>
> An obvious solution to this problem is to save the
> current file name in Dired instead of its point.
>
> It seems saving information other than point doesn't contradict
> the purpose of saveplace.el that automatically saves places where
> visiting them later automatically moves point to the saved position.
> Also elements of `save-place-alist' are (FILENAME . POSITION),
> not more limited (FILENAME . POINT).  So saving a string to
> POSITION would be consistent with the design of saveplace.el.

I realized this is not backward-compatible change, i.e.
older Emacs versions won't be able to read saved places
in the new format.  Unfortunately, Dired filename positions
can't be added to the current format that uses an alist
with cons pair cells.  I mean (FILENAME . POINT)
can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
because older Emacs versions will fail to read them.

The only solution that I see is to save two alists to the places file:

((FILENAME1 . POINT1)
 (FILENAME2 . POINT2)
 ...)
((FILENAME1 DIRED-FILENAME-POSITION1)
 (FILENAME2 DIRED-FILENAME-POSITION2)
 ...)

Then the current code:

  (with-demoted-errors
    (car (read-from-string
      (buffer-substring (point-min) (point-max)))))

will read the first alist without problems,
and additional code in newer versions like

  (with-demoted-errors
    (cadr (read-from-string
      (buffer-substring (point-min) (point-max)))))

will read the second alist with more information
about the context of saved places (like bookmarks).

bug#15329: saveplace restores dired positions to random places

[Karl Fogel]

Juri Linkov wrote:
>> An obvious solution to this problem is to save the
>> current file name in Dired instead of its point.
>>
>> It seems saving information other than point doesn't contradict
>> the purpose of saveplace.el that automatically saves places where
>> visiting them later automatically moves point to the saved position.
>> Also elements of `save-place-alist' are (FILENAME . POSITION),
>> not more limited (FILENAME . POINT).  So saving a string to
>> POSITION would be consistent with the design of saveplace.el.
>
>I realized this is not backward-compatible change, i.e.
>older Emacs versions won't be able to read saved places
>in the new format.  Unfortunately, Dired filename positions
>can't be added to the current format that uses an alist
>with cons pair cells.  I mean (FILENAME . POINT)
>can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
>because older Emacs versions will fail to read them.
>
>The only solution that I see is to save two alists to the places file:
>
>((FILENAME1 . POINT1)
> (FILENAME2 . POINT2)
> ...)
>((FILENAME1 DIRED-FILENAME-POSITION1)
> (FILENAME2 DIRED-FILENAME-POSITION2)
> ...)
>
>Then the current code:
>
>  (with-demoted-errors
>    (car (read-from-string
>      (buffer-substring (point-min) (point-max)))))
>
>will read the first alist without problems,
>and additional code in newer versions like
>
>  (with-demoted-errors
>    (cadr (read-from-string
>      (buffer-substring (point-min) (point-max)))))
>
>will read the second alist with more information
>about the context of saved places (like bookmarks).

Well, rather, we could use that as an upgrade strategy for the saveplace
format as a whole.  In other words, starting now and for a few versions
of Emacs into the future, write the old format to the first part of the
file, and then a more flexible new format to the second part.  The
modern format would have a more extensible structure, similarly to how
bookmark.el does it.  Say, a sublist whose first element is the type of
the record, and the rest of which is the data for that record.  Like:

  ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
   (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
   ...)

etc.

Thoughts?

-Karl

Re: bug#15329: saveplace restores dired positions to random places

[Karl Fogel]

Juri Linkov wrote:
>> An obvious solution to this problem is to save the
>> current file name in Dired instead of its point.
>>
>> It seems saving information other than point doesn't contradict
>> the purpose of saveplace.el that automatically saves places where
>> visiting them later automatically moves point to the saved position.
>> Also elements of `save-place-alist' are (FILENAME . POSITION),
>> not more limited (FILENAME . POINT).  So saving a string to
>> POSITION would be consistent with the design of saveplace.el.
>
>I realized this is not backward-compatible change, i.e.
>older Emacs versions won't be able to read saved places
>in the new format.  Unfortunately, Dired filename positions
>can't be added to the current format that uses an alist
>with cons pair cells.  I mean (FILENAME . POINT)
>can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
>because older Emacs versions will fail to read them.
>
>The only solution that I see is to save two alists to the places file:
>
>((FILENAME1 . POINT1)
> (FILENAME2 . POINT2)
> ...)
>((FILENAME1 DIRED-FILENAME-POSITION1)
> (FILENAME2 DIRED-FILENAME-POSITION2)
> ...)
>
>Then the current code:
>
>  (with-demoted-errors
>    (car (read-from-string
>      (buffer-substring (point-min) (point-max)))))
>
>will read the first alist without problems,
>and additional code in newer versions like
>
>  (with-demoted-errors
>    (cadr (read-from-string
>      (buffer-substring (point-min) (point-max)))))
>
>will read the second alist with more information
>about the context of saved places (like bookmarks).

Well, rather, we could use that as an upgrade strategy for the saveplace
format as a whole.  In other words, starting now and for a few versions
of Emacs into the future, write the old format to the first part of the
file, and then a more flexible new format to the second part.  The
modern format would have a more extensible structure, similarly to how
bookmark.el does it.  Say, a sublist whose first element is the type of
the record, and the rest of which is the data for that record.  Like:

  ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
   (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
   ...)

etc.

Thoughts?

-Karl

bug#15329: saveplace restores dired positions to random places

[Stefan Monnier]

>   ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
>    (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
>    ...)

Re-using as much as possible of bookmarks's format would make sense.


        Stefan

Re: bug#15329: saveplace restores dired positions to random places

[Stefan Monnier]

>   ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
>    (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
>    ...)

Re-using as much as possible of bookmarks's format would make sense.


        Stefan

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> The modern format would have a more extensible structure, similarly to how
> bookmark.el does it.  Say, a sublist whose first element is the type of
> the record, and the rest of which is the data for that record.

Do you think it would be possible to use the existing
infrastructure of bookmark.el, so for instance, to save
a place in an Info manual, saveplace.el could call
`Info-bookmark-make-record' and to restore it with
`Info-bookmark-jump'.  This would be better than adding
a third hook for saveplace (the second existing hook
is desktop-specific like `Info-desktop-buffer-misc-data'
and `Info-restore-desktop-buffer').

Re: bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> The modern format would have a more extensible structure, similarly to how
> bookmark.el does it.  Say, a sublist whose first element is the type of
> the record, and the rest of which is the data for that record.

Do you think it would be possible to use the existing
infrastructure of bookmark.el, so for instance, to save
a place in an Info manual, saveplace.el could call
`Info-bookmark-make-record' and to restore it with
`Info-bookmark-jump'.  This would be better than adding
a third hook for saveplace (the second existing hook
is desktop-specific like `Info-desktop-buffer-misc-data'
and `Info-restore-desktop-buffer').

bug#15329: saveplace restores dired positions to random places

[Karl Fogel]

Juri Linkov <juri@jurta.org> writes:
>> The modern format would have a more extensible structure, similarly to how
>> bookmark.el does it.  Say, a sublist whose first element is the type of
>> the record, and the rest of which is the data for that record.
>
>Do you think it would be possible to use the existing
>infrastructure of bookmark.el, so for instance, to save
>a place in an Info manual, saveplace.el could call
>`Info-bookmark-make-record' and to restore it with
>`Info-bookmark-jump'.  This would be better than adding
>a third hook for saveplace (the second existing hook
>is desktop-specific like `Info-desktop-buffer-misc-data'
>and `Info-restore-desktop-buffer').

Yes; since both saveplace and bookmark are in the standard Emacs dist,
it's fine for them to share code, and would improve maintainability.

I'm not sure when I'll get a chance to work on this, though.  The
original bug here is about saving/restoring position in dired buffers.
While the current rather random behavior in dired is obviously a bug,
and fixing it would be a Good Thing, I'm not sure it rises to the level
where I drop other things to work on it :-).  However, if someone were
to write a patch (along the lines described in this bug report), I'd
certainly commit to reviewing it.

Best,
-Karl

Re: bug#15329: saveplace restores dired positions to random places

[Karl Fogel]

Juri Linkov <juri@jurta.org> writes:
>> The modern format would have a more extensible structure, similarly to how
>> bookmark.el does it.  Say, a sublist whose first element is the type of
>> the record, and the rest of which is the data for that record.
>
>Do you think it would be possible to use the existing
>infrastructure of bookmark.el, so for instance, to save
>a place in an Info manual, saveplace.el could call
>`Info-bookmark-make-record' and to restore it with
>`Info-bookmark-jump'.  This would be better than adding
>a third hook for saveplace (the second existing hook
>is desktop-specific like `Info-desktop-buffer-misc-data'
>and `Info-restore-desktop-buffer').

Yes; since both saveplace and bookmark are in the standard Emacs dist,
it's fine for them to share code, and would improve maintainability.

I'm not sure when I'll get a chance to work on this, though.  The
original bug here is about saving/restoring position in dired buffers.
While the current rather random behavior in dired is obviously a bug,
and fixing it would be a Good Thing, I'm not sure it rises to the level
where I drop other things to work on it :-).  However, if someone were
to write a patch (along the lines described in this bug report), I'd
certainly commit to reviewing it.

Best,
-Karl

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> While the current rather random behavior in dired is obviously a bug,
> and fixing it would be a Good Thing, I'm not sure it rises to the level
> where I drop other things to work on it :-).  However, if someone were
> to write a patch (along the lines described in this bug report), I'd
> certainly commit to reviewing it.

I realized now that the current format is completely backward-compatible.
When `find-file-hook' restores the saved position in a file, it doesn't see
the dired entries (that end with a directory separator) in the new format.

As an additional benefit, the users can add a regexp like "/$" to
`save-place-ignore-files-regexp' to not save dired positions.

More packages can do the same later, e.g. Info could save the entries like
"(emacs) Top" with own additional information that only Info will restore, etc.

So now dired positions should be fixed by this patch:

=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el	2013-09-12 05:32:57 +0000
+++ lisp/saveplace.el	2013-12-15 20:12:27 +0000
@@ -152,8 +152,8 @@ (defun toggle-save-place (&optional parg
 
 \(setq-default save-place t\)"
   (interactive "P")
-  (if (not buffer-file-name)
-      (message "Buffer `%s' not visiting a file" (buffer-name))
+  (if (not (or buffer-file-name dired-directory))
+      (message "Buffer `%s' not visiting a file or directory" (buffer-name))
     (if (and save-place (or (not parg) (<= parg 0)))
 	(progn
 	  (message "No place will be saved in this file")
@@ -161,6 +161,8 @@ (defun toggle-save-place (&optional parg
       (message "Place will be saved")
       (setq save-place t))))
 
+(declare-function dired-get-filename "dired" (&optional localp no-error-if-not-filep))
+
 (defun save-place-to-alist ()
   ;; put filename and point in a cons box and then cons that onto the
   ;; front of the save-place-alist, if save-place is non-nil.
@@ -176,14 +178,20 @@ (defun save-place-to-alist ()
                    (not (string-match save-place-ignore-files-regexp
                                       item))))
       (let ((cell (assoc item save-place-alist))
-            (position (if (not (eq major-mode 'hexl-mode))
-                          (point)
-                        (with-no-warnings
-                          (1+ (hexl-current-address))))))
+            (position (cond ((eq major-mode 'hexl-mode)
+			     (with-no-warnings
+			       (1+ (hexl-current-address))))
+			    ((derived-mode-p 'dired-mode)
+			     (let ((filename (dired-get-filename nil t)))
+			       (if filename
+				   `((dired-filename . ,filename))
+				 (point))))
+			    (t (point)))))
         (if cell
             (setq save-place-alist (delq cell save-place-alist)))
         (if (and save-place
-                 (not (= position 1)))  ;; Optimize out the degenerate case.
+                 (not (and (integerp position)
+			   (= position 1)))) ;; Optimize out the degenerate case.
             (setq save-place-alist
                   (cons (cons item position)
                         save-place-alist)))))))
@@ -290,7 +298,8 @@ (defun save-places-to-alist ()
       (with-current-buffer (car buf-list)
 	;; save-place checks buffer-file-name too, but we can avoid
 	;; overhead of function call by checking here too.
-	(and buffer-file-name (save-place-to-alist))
+	(and (or buffer-file-name dired-directory)
+	     (save-place-to-alist))
 	(setq buf-list (cdr buf-list))))))
 
 (defun save-place-find-file-hook ()
@@ -299,10 +308,13 @@ (defun save-place-find-file-hook ()
     (if cell
 	(progn
 	  (or revert-buffer-in-progress-p
-	      (goto-char (cdr cell)))
+	      (and (integerp (cdr cell))
+		   (goto-char (cdr cell))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 
+(declare-function dired-goto-file "dired" (file))
+
 (defun save-place-dired-hook ()
   "Position the point in a dired buffer."
   (or save-place-loaded (load-save-place-alist-from-file))
@@ -310,7 +322,10 @@ (defun save-place-dired-hook ()
     (if cell
         (progn
           (or revert-buffer-in-progress-p
-              (goto-char (cdr cell)))
+              (if (integerp (cdr cell))
+		  (goto-char (cdr cell))
+		(and (assq 'dired-filename (cdr cell))
+		     (dired-goto-file (cdr (assq 'dired-filename (cdr cell)))))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 

bug#15329: saveplace restores dired positions to random places

[Drew Adams]

> -  (if (not buffer-file-name)
> -      (message "Buffer `%s' not visiting a file" (buffer-name))
> +  (if (not (or buffer-file-name dired-directory))
> +      (message "Buffer `%s' not visiting a file or directory"
> +               (buffer-name))

Is `dired-directory' really the right test here?  I am used to seeing
(derived-mode-p 'dired-mode) for that purpose (assuming I understand the
purpose here).

There is, BTW, nothing in the doc string of `dired-directory' that says
what a nil value means.  Should code now instead be using
`dired-directory' to test whether the mode is (derived from) Dired?

If so, then at the very least the doc string of that variable should
describe such a Boolean meaning: nil means not in Dired mode or a mode
derived from it (or whatever the completely correct interpretation is).

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

>> -  (if (not buffer-file-name)
>> -      (message "Buffer `%s' not visiting a file" (buffer-name))
>> +  (if (not (or buffer-file-name dired-directory))
>> +      (message "Buffer `%s' not visiting a file or directory"
>> +               (buffer-name))
>
> Is `dired-directory' really the right test here?  I am used to seeing
> (derived-mode-p 'dired-mode) for that purpose (assuming I understand the
> purpose here).

Yes, `dired-directory' is the right test, because `dired-directory'
is used as a key in `save-place-alist'.

> There is, BTW, nothing in the doc string of `dired-directory' that says
> what a nil value means.

Thanks for pointing to the doc string of `dired-directory' where I noticed:

  May be a list, in which case the car is the
  directory name and the cdr is the list of files to mention.

This case should be handled properly that I added to the following patch.

> Should code now instead be using `dired-directory' to test whether the
> mode is (derived from) Dired?

Yes, code should be using `dired-directory' now.

> If so, then at the very least the doc string of that variable should
> describe such a Boolean meaning: nil means not in Dired mode or a mode
> derived from it (or whatever the completely correct interpretation is).

Are you sure that nil means not in Dired mode or a mode derived from it?

Anyway, this is a new version:

=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el	2013-09-12 05:32:57 +0000
+++ lisp/saveplace.el	2013-12-16 20:56:14 +0000
@@ -152,8 +152,8 @@ (defun toggle-save-place (&optional parg
 
 \(setq-default save-place t\)"
   (interactive "P")
-  (if (not buffer-file-name)
-      (message "Buffer `%s' not visiting a file" (buffer-name))
+  (if (not (or buffer-file-name dired-directory))
+      (message "Buffer `%s' not visiting a file or directory" (buffer-name))
     (if (and save-place (or (not parg) (<= parg 0)))
 	(progn
 	  (message "No place will be saved in this file")
@@ -161,6 +161,8 @@ (defun toggle-save-place (&optional parg
       (message "Place will be saved")
       (setq save-place t))))
 
+(declare-function dired-get-filename "dired" (&optional localp no-error-if-not-filep))
+
 (defun save-place-to-alist ()
   ;; put filename and point in a cons box and then cons that onto the
   ;; front of the save-place-alist, if save-place is non-nil.
@@ -170,20 +172,29 @@ (defun save-place-to-alist ()
   ;; will be saved again when Emacs is killed.
   (or save-place-loaded (load-save-place-alist-from-file))
   (let ((item (or buffer-file-name
-                  (and dired-directory (expand-file-name dired-directory)))))
+                  (and dired-directory
+		       (if (consp dired-directory)
+			   (expand-file-name (car dired-directory))
+			 (expand-file-name dired-directory))))))
     (when (and item
                (or (not save-place-ignore-files-regexp)
                    (not (string-match save-place-ignore-files-regexp
                                       item))))
       (let ((cell (assoc item save-place-alist))
-            (position (if (not (eq major-mode 'hexl-mode))
-                          (point)
-                        (with-no-warnings
-                          (1+ (hexl-current-address))))))
+            (position (cond ((eq major-mode 'hexl-mode)
+			     (with-no-warnings
+			       (1+ (hexl-current-address))))
+			    (dired-directory
+			     (let ((filename (dired-get-filename nil t)))
+			       (if filename
+				   `((dired-filename . ,filename))
+				 (point))))
+			    (t (point)))))
         (if cell
             (setq save-place-alist (delq cell save-place-alist)))
         (if (and save-place
-                 (not (= position 1)))  ;; Optimize out the degenerate case.
+                 (not (and (integerp position)
+			   (= position 1)))) ;; Optimize out the degenerate case.
             (setq save-place-alist
                   (cons (cons item position)
                         save-place-alist)))))))
@@ -290,7 +301,8 @@ (defun save-places-to-alist ()
       (with-current-buffer (car buf-list)
 	;; save-place checks buffer-file-name too, but we can avoid
 	;; overhead of function call by checking here too.
-	(and buffer-file-name (save-place-to-alist))
+	(and (or buffer-file-name dired-directory)
+	     (save-place-to-alist))
 	(setq buf-list (cdr buf-list))))))
 
 (defun save-place-find-file-hook ()
@@ -299,18 +311,27 @@ (defun save-place-find-file-hook ()
     (if cell
 	(progn
 	  (or revert-buffer-in-progress-p
-	      (goto-char (cdr cell)))
+	      (and (integerp (cdr cell))
+		   (goto-char (cdr cell))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 
+(declare-function dired-goto-file "dired" (file))
+
 (defun save-place-dired-hook ()
   "Position the point in a dired buffer."
   (or save-place-loaded (load-save-place-alist-from-file))
-  (let ((cell (assoc (expand-file-name dired-directory) save-place-alist)))
+  (let ((cell (assoc (if (consp dired-directory)
+			 (expand-file-name (car dired-directory))
+		       (expand-file-name dired-directory))
+		     save-place-alist)))
     (if cell
         (progn
           (or revert-buffer-in-progress-p
-              (goto-char (cdr cell)))
+              (if (integerp (cdr cell))
+		  (goto-char (cdr cell))
+		(and (assq 'dired-filename (cdr cell))
+		     (dired-goto-file (cdr (assq 'dired-filename (cdr cell)))))))
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 

bug#15329: saveplace restores dired positions to random places

[Drew Adams]

> > Is `dired-directory' really the right test here?  I am used to seeing
> > (derived-mode-p 'dired-mode) for that purpose (assuming I understand the
> > purpose here).
> 
> Yes, `dired-directory' is the right test, because `dired-directory'
> is used as a key in `save-place-alist'.

OK.

> > There is, BTW, nothing in the doc string of `dired-directory' that says
> > what a nil value means.
> 
> Thanks for pointing to the doc string of `dired-directory' where I noticed:
> 
>   May be a list, in which case the car is the
>   directory name and the cdr is the list of files to mention.

Yes, I use that use case quite a lot.

> This case should be handled properly that I added to the following patch.

Great.

> > Should code now instead be using `dired-directory' to test whether the
> > mode is (derived from) Dired?
> 
> Yes, code should be using `dired-directory' now.

OK.  Do you happen to know whether that is only the case now or has been the
case all along?

`dired-directory' is old, but I have always seen `derived-mode-p' used for
the test - and I see it still, e.g., for `dired-toggle-read-only'.  Or does
it perhaps not matter at all which we use?  Looking for what is best to use
with multiple Emacs versions.

> > If so, then at the very least the doc string of that variable should
> > describe such a Boolean meaning: nil means not in Dired mode or a mode
> > derived from it (or whatever the completely correct interpretation is).
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Are you sure that nil means not in Dired mode or a mode derived from it?

I posed a question; I didn't make a statement.  Let's please describe what
nil means here, whatever that might be.

> Anyway, this is a new version:

Thanks for taking the cons case into account.

bug#15329: saveplace restores dired positions to random places

[Juri Linkov]

> I realized now that the current format is completely backward-compatible.
> When `find-file-hook' restores the saved position in a file, it doesn't see
> the dired entries (that end with a directory separator) in the new format.

Karl, due to the imminent feature freeze I had to commit the patch now.
Please revert it if you don't agree with this implementation.

bug#15329: saveplace restores dired positions to random places

[Karl Fogel]

Juri Linkov <juri@jurta.org> writes:
>> I realized now that the current format is completely backward-compatible.
>> When `find-file-hook' restores the saved position in a file, it doesn't see
>> the dired entries (that end with a directory separator) in the new format.
>
>Karl, due to the imminent feature freeze I had to commit the patch now.
>Please revert it if you don't agree with this implementation.

Understood, and thank you.

Unlike the rest of the Western world, my schedule is such that I don't
have much time for Emacs hacking during the holiday season, but should
be able to pay some attention to Emacs stuff afterwards.  So I'll review
this then.  It's unfortunate that that review has to wait until possibly
after the release :-(, but I try to think about these things with a long
term perspective...

Best,
-K