bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position

[Liu Hui <liuhui1610@gmail.com>]

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

Ruijie Yu <ruijie@netyu.xyz> 于2023年4月3日周一 11:06写道：

> Two minor comments below.
>
> > @@ -90,8 +92,32 @@ save-place-forget-unreadable-files
> >  (defcustom save-place-abbreviate-file-names nil
> > [...]
> > +  :set (lambda (sym val)
> > +         (set-default sym val)
> > +         (let ((fun (if val 'abbreviate-file-name 'expand-file-name)))
>
> I believe function quotes "#'" are preferred over simple quotes "'" when
> dealing with functions.

OK

> > @@ -214,7 +241,11 @@ save-place-to-alist
> >                           ((and (derived-mode-p 'dired-mode) directory)
> >                            (let ((filename (dired-get-filename nil t)))
> >                              (if filename
> > -                                `((dired-filename . ,filename))
> > +                                   (list
> > +                                    (cons 'dired-filename
> > +                                          (if save-place-abbreviate-file-names
> > +                                              (abbreviate-file-name filename)
> > +                                            filename)))
>
> It seems that you rewrote the quote-backquote thing with regular
> list-cons construct -- no comments on that.  I noticed that here, and in
> a few other places, you are reusing the exact `if' construct multiple
> times.  Does that warrant defining a helper function?

I feel such a function is too short.

> Also, while I was about to send the mail, regarding the docstring of
> `save-place-abbreviate-file-names', instead of letting the user enable
> `save-place-mode', would it be better if you directly call facilities in
> saveplace to load `save-place-alist' from file system, within your :set
> function?

Thanks for the suggestion. I have added `save-place-load-alist-from-file'
to the :set function in the new patch.

[-- Attachment #2: 0001-Restore-positions-reliably-for-abbreviated-file-name.patch --]
[-- Type: text/x-patch, Size: 10041 bytes --]

From fb117e1649b3bdde9828816852c612d9b0e14eb5 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Tue, 4 Apr 2023 09:13:32 +0800
Subject: [PATCH] Restore positions reliably for abbreviated file names in
 saveplace.el

* lisp/saveplace.el (save-place-abbreviate-file-names): Add setter
function for rewriting `save-place-alist'.  Update docstring.
(save-place-to-alist): Save Abbreviated dired-filename.
(save-place-load-alist-from-file): Move this function above
`save-place-abbreviate-file-names' since it is used in the :set
function.
(save-place-find-file-hook):
(save-place-dired-hook): Use abbreviated file name when
`save-place-abbreviate-file-names' is non-nil.
---
 lisp/saveplace.el | 163 ++++++++++++++++++++++++++++------------------
 1 file changed, 98 insertions(+), 65 deletions(-)

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 7512fc87c5d..18d296ba2d9 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -35,6 +35,8 @@
 
 ;;; Code:
 
+(require 'cl-lib)
+
 ;; this is what I was using during testing:
 ;; (define-key ctl-x-map "p" 'toggle-save-place-globally)
 
@@ -87,11 +89,77 @@ save-place-forget-unreadable-files
 `save-place-file'."
   :type 'boolean)
 
+(defun save-place-load-alist-from-file ()
+  (if (not save-place-loaded)
+      (progn
+        (setq save-place-loaded t)
+        (let ((file (expand-file-name save-place-file)))
+          ;; make sure that the alist does not get overwritten, and then
+          ;; load it if it exists:
+          (if (file-readable-p file)
+              ;; don't want to use find-file because we have been
+              ;; adding hooks to it.
+              (with-current-buffer (get-buffer-create " *Saved Places*")
+                (delete-region (point-min) (point-max))
+                ;; Make sure our 'coding:' cookie in the save-place
+                ;; file will take effect, in case the caller binds
+                ;; coding-system-for-read.
+                (let (coding-system-for-read)
+                  (insert-file-contents file))
+                (goto-char (point-min))
+                (setq save-place-alist
+                      (with-demoted-errors "Error reading save-place-file: %S"
+                        (car (read-from-string
+                              (buffer-substring (point-min) (point-max))))))
+
+                ;; If there is a limit, and we're over it, then we'll
+                ;; have to truncate the end of the list:
+                (if save-place-limit
+                    (if (<= save-place-limit 0)
+                        ;; Zero gets special cased.  I'm not thrilled
+                        ;; with this, but the loop for >= 1 is tight.
+                        (setq save-place-alist nil)
+                      ;; Else the limit is >= 1, so enforce it by
+                      ;; counting and then `setcdr'ing.
+                      (let ((s save-place-alist)
+                            (count 1))
+                        (while s
+                          (if (>= count save-place-limit)
+                              (setcdr s nil)
+                            (setq count (1+ count)))
+                          (setq s (cdr s))))))
+
+                (kill-buffer (current-buffer))))
+          nil))))
+
 (defcustom save-place-abbreviate-file-names nil
   "If non-nil, abbreviate file names before saving them.
 This can simplify sharing the `save-place-file' file across
-different hosts."
+different hosts.
+
+Changing this option requires rewriting `save-place-alist' with
+corresponding file name format, therefore setting this option
+just using `setq' may cause out-of-sync problems.  You should use
+either `setopt' or M-x customize-variable to set this option."
   :type 'boolean
+  :set (lambda (sym val)
+         (set-default sym val)
+         (or save-place-loaded (save-place-load-alist-from-file))
+         (let ((fun (if val #'abbreviate-file-name #'expand-file-name)))
+           (setq save-place-alist
+                 (cl-delete-duplicates
+                  (cl-loop for (k . v) in save-place-alist
+                           collect
+                           (cons (funcall fun k)
+                                 (if (listp v)
+                                     (cl-loop for (k1 . v1) in v
+                                              collect
+                                              (cons k1 (funcall fun v1)))
+                                   v)))
+                  :key #'car
+                  :from-end t
+                  :test #'equal)))
+         val)
   :version "28.1")
 
 (defcustom save-place-save-skipped t
@@ -214,7 +282,11 @@ save-place-to-alist
 			    ((and (derived-mode-p 'dired-mode) directory)
 			     (let ((filename (dired-get-filename nil t)))
 			       (if filename
-				   `((dired-filename . ,filename))
+                                   (list
+                                    (cons 'dired-filename
+                                          (if save-place-abbreviate-file-names
+                                              (abbreviate-file-name filename)
+                                            filename)))
 				 (point))))
 			    (t (point)))))
         (if cell
@@ -278,49 +350,6 @@ save-place-alist-to-file
 	  (file-error (message "Saving places: can't write %s" file)))
         (kill-buffer (current-buffer))))))
 
-(defun save-place-load-alist-from-file ()
-  (if (not save-place-loaded)
-      (progn
-        (setq save-place-loaded t)
-        (let ((file (expand-file-name save-place-file)))
-          ;; make sure that the alist does not get overwritten, and then
-          ;; load it if it exists:
-          (if (file-readable-p file)
-              ;; don't want to use find-file because we have been
-              ;; adding hooks to it.
-              (with-current-buffer (get-buffer-create " *Saved Places*")
-                (delete-region (point-min) (point-max))
-                ;; Make sure our 'coding:' cookie in the save-place
-                ;; file will take effect, in case the caller binds
-                ;; coding-system-for-read.
-                (let (coding-system-for-read)
-                  (insert-file-contents file))
-                (goto-char (point-min))
-                (setq save-place-alist
-                      (with-demoted-errors "Error reading save-place-file: %S"
-                        (car (read-from-string
-                              (buffer-substring (point-min) (point-max))))))
-
-                ;; If there is a limit, and we're over it, then we'll
-                ;; have to truncate the end of the list:
-                (if save-place-limit
-                    (if (<= save-place-limit 0)
-                        ;; Zero gets special cased.  I'm not thrilled
-                        ;; with this, but the loop for >= 1 is tight.
-                        (setq save-place-alist nil)
-                      ;; Else the limit is >= 1, so enforce it by
-                      ;; counting and then `setcdr'ing.
-                      (let ((s save-place-alist)
-                            (count 1))
-                        (while s
-                          (if (>= count save-place-limit)
-                              (setcdr s nil)
-                            (setq count (1+ count)))
-                          (setq s (cdr s))))))
-
-                (kill-buffer (current-buffer))))
-          nil))))
-
 (defun save-places-to-alist ()
   ;; go through buffer-list, saving places to alist if save-place-mode
   ;; is non-nil, deleting them from alist if it is nil.
@@ -353,7 +382,11 @@ save-place-find-file-hook
   "Function added to `find-file-hook' by `save-place-mode'.
 It runs the hook `save-place-after-find-file-hook'."
   (or save-place-loaded (save-place-load-alist-from-file))
-  (let ((cell (assoc buffer-file-name save-place-alist)))
+  (let ((cell (and (stringp buffer-file-name)
+                   (assoc (if save-place-abbreviate-file-names
+                              (abbreviate-file-name buffer-file-name)
+                            buffer-file-name)
+                          save-place-alist))))
     (if cell
 	(progn
 	  (or revert-buffer-in-progress-p
@@ -368,25 +401,25 @@ save-place-find-file-hook
 (defun save-place-dired-hook ()
   "Position the point in a Dired buffer."
   (or save-place-loaded (save-place-load-alist-from-file))
-  (let* ((directory (and (derived-mode-p 'dired-mode)
-                         (boundp 'dired-subdir-alist)
-			 dired-subdir-alist
-			 (dired-current-directory)))
-	 (cell (assoc (and directory
-			   (expand-file-name (if (consp directory)
-						 (car directory)
-					       directory)))
-		      save-place-alist)))
-    (if cell
-        (progn
-          (or revert-buffer-in-progress-p
-              (cond
-	       ((integerp (cdr cell))
-		(goto-char (cdr cell)))
-	       ((and (listp (cdr cell)) (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-mode t)))))
+  (when-let ((directory (and (derived-mode-p 'dired-mode)
+                             (boundp 'dired-subdir-alist)
+			     dired-subdir-alist
+			     (dired-current-directory)))
+             (item (expand-file-name (if (consp directory)
+					 (car directory)
+				       directory)))
+	     (cell (assoc (if save-place-abbreviate-file-names
+                              (abbreviate-file-name item) item)
+		          save-place-alist)))
+    (or revert-buffer-in-progress-p
+        (cond
+	 ((integerp (cdr cell))
+	  (goto-char (cdr cell)))
+	 ((listp (cdr cell))
+          (when-let ((elt (assq 'dired-filename (cdr cell))))
+            (dired-goto-file (expand-file-name (cdr elt)))))))
+    ;; and make sure it will be saved again for later
+    (setq save-place-mode t)))
 
 (defun save-place-kill-emacs-hook ()
   ;; First update the alist.  This loads the old save-place-file if nec.
-- 
2.25.1