bug#15329: saveplace restores dired positions to random places

[Juri Linkov <juri@jurta.org>]

>> -  (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)))))