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

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

[Liu Hui]

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

Hi,

save-place-mode cannot restore last saved position when
`save-place-abbreviate-file-names' is non-nil:

- emacs -Q
- M-x save-place-mode, and set save-place-abbreviate-file-names to t
- open a file, scroll to some position, and kill buffer
- reopen the file, the position is not restored

The reason is it uses abbreviated file names when saving positions to
`save-place-alist', but uses full file names to find position in
`save-place-alist'.


Best,
Liu Hui

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

From 468b699e3123c008d8a5bd050c1efc3ad926b198 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Fri, 24 Mar 2023 10:08:12 +0800
Subject: [PATCH] Restore positions for abbreviated file names in saveplace.el

* lisp/saveplace.el (save-place-find-file-hook): Use abbreviated file
name when `save-place-abbreviate-file-names' is non-nil.
---
 lisp/saveplace.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 7512fc87c5d..5262f574efc 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -353,8 +353,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* ((item (if (and (stringp buffer-file-name)
+                        save-place-abbreviate-file-names)
+                   (abbreviate-file-name buffer-file-name)
+                 buffer-file-name))
+         (cell (assoc item save-place-alist)))
     (if cell
 	(progn
 	  (or revert-buffer-in-progress-p
--
2.25.1

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Fri, 24 Mar 2023 10:19:07 +0800
> 
> save-place-mode cannot restore last saved position when
> `save-place-abbreviate-file-names' is non-nil:
> 
> - emacs -Q
> - M-x save-place-mode, and set save-place-abbreviate-file-names to t
> - open a file, scroll to some position, and kill buffer
> - reopen the file, the position is not restored
> 
> The reason is it uses abbreviated file names when saving positions to
> `save-place-alist', but uses full file names to find position in
> `save-place-alist'.

I believe you are right with this analysis, thanks.

> --- a/lisp/saveplace.el
> +++ b/lisp/saveplace.el
> @@ -353,8 +353,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* ((item (if (and (stringp buffer-file-name)
> +                        save-place-abbreviate-file-names)
> +                   (abbreviate-file-name buffer-file-name)
> +                 buffer-file-name))
> +         (cell (assoc item save-place-alist)))

Wouldn't it be best to always test for abbreviated file name if the
full file name fails to match?  E.g., it could be that the user turned
on save-place-abbreviate-file-names for a while, then turned it off
(or vice versa), thus causing mixed file names in the saved history.
It would also make the code simpler, I think.

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

[Liu Hui]

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

在 2023/3/25 19:52, Eli Zaretskii 写道:

>> --- a/lisp/saveplace.el
>> +++ b/lisp/saveplace.el
>> @@ -353,8 +353,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* ((item (if (and (stringp buffer-file-name)
>> +                        save-place-abbreviate-file-names)
>> +                   (abbreviate-file-name buffer-file-name)
>> +                 buffer-file-name))
>> +         (cell (assoc item save-place-alist)))
>
> Wouldn't it be best to always test for abbreviated file name if the
> full file name fails to match?  E.g., it could be that the user turned
> on save-place-abbreviate-file-names for a while, then turned it off
> (or vice versa), thus causing mixed file names in the saved history.
> It would also make the code simpler, I think.

I agree that it is better to consider mixed file names. Please see the
updated patch.

[-- Attachment #2: 0001-Restore-positions-for-abbreviated-file-names-in-save.patch --]
[-- Type: application/x-patch, Size: 1427 bytes --]

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Sat, 25 Mar 2023 22:14:02 +0800
> Cc: 62413@debbugs.gnu.org
> 
> > Wouldn't it be best to always test for abbreviated file name if the
> > full file name fails to match?  E.g., it could be that the user turned
> > on save-place-abbreviate-file-names for a while, then turned it off
> > (or vice versa), thus causing mixed file names in the saved history.
> > It would also make the code simpler, I think.
> 
> I agree that it is better to consider mixed file names. Please see the
> updated patch.
> 
> From ad5f2d4f3c878a15c2fd1d2eca09e2f2fbecec15 Mon Sep 17 00:00:00 2001
> From: Liu Hui <liuhui1610@gmail.com>
> Date: Fri, 24 Mar 2023 10:08:12 +0800
> Subject: [PATCH] Restore positions for abbreviated file names in saveplace.el
> 
> * lisp/saveplace.el (save-place-find-file-hook): Use abbreviated file
> name when `save-place-abbreviate-file-names' is non-nil.
> ---
>  lisp/saveplace.el | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/lisp/saveplace.el b/lisp/saveplace.el
> index 7512fc87c5d..6c3ed34f198 100644
> --- a/lisp/saveplace.el
> +++ b/lisp/saveplace.el
> @@ -353,7 +353,14 @@ 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)
> +                   (if save-place-abbreviate-file-names
> +                       (or (assoc (abbreviate-file-name buffer-file-name)
> +                                  save-place-alist)
> +                           (assoc buffer-file-name save-place-alist))
> +                     (or (assoc buffer-file-name save-place-alist)
> +                         (assoc (abbreviate-file-name buffer-file-name)
> +                                save-place-alist))))))
>      (if cell

But now testing save-place-abbreviate-file-names here should be
redundant, right?

Also, I think we should first test buffer-file-name, and only after
that its abbreviated variant.

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

[Liu Hui]

Eli Zaretskii <eliz@gnu.org> 于2023年3月25日周六 22:17写道：

> >  lisp/saveplace.el | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/lisp/saveplace.el b/lisp/saveplace.el
> > index 7512fc87c5d..6c3ed34f198 100644
> > --- a/lisp/saveplace.el
> > +++ b/lisp/saveplace.el
> > @@ -353,7 +353,14 @@ 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)
> > +                   (if save-place-abbreviate-file-names
> > +                       (or (assoc (abbreviate-file-name buffer-file-name)
> > +                                  save-place-alist)
> > +                           (assoc buffer-file-name save-place-alist))
> > +                     (or (assoc buffer-file-name save-place-alist)
> > +                         (assoc (abbreviate-file-name buffer-file-name)
> > +                                save-place-alist))))))
> >      (if cell
>
> But now testing save-place-abbreviate-file-names here should be
> redundant, right?
>
> Also, I think we should first test buffer-file-name, and only after
> that its abbreviated variant.

I don't think so. Consider the following case:

- open file A and then close the buffer:
  (buffer-file-name . position1) is saved in save-place-alist

- then set save-place-abbreviate-file-names to t

- open file A, scroll the buffer and close it:
  (abbreviated-file-name . position2) is saved

- open file A again, and the point will be at position1 if
  buffer-file-name is tested first. But I would expect the point is at
  position2.

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Sun, 26 Mar 2023 09:26:22 +0800
> Cc: 62413@debbugs.gnu.org
> 
> > But now testing save-place-abbreviate-file-names here should be
> > redundant, right?
> >
> > Also, I think we should first test buffer-file-name, and only after
> > that its abbreviated variant.
> 
> I don't think so. Consider the following case:
> 
> - open file A and then close the buffer:
>   (buffer-file-name . position1) is saved in save-place-alist
> 
> - then set save-place-abbreviate-file-names to t
> 
> - open file A, scroll the buffer and close it:
>   (abbreviated-file-name . position2) is saved
> 
> - open file A again, and the point will be at position1 if
>   buffer-file-name is tested first. But I would expect the point is at
>   position2.

Ugh!  This feature was not thought out well enough when it was
introduced: if the user changes the value half-way through a session,
the history will record visited files twice, under 2 different
file-name formats and with different places recorded.  I think
changing the value of save-place-abbreviate-file-names should rewrite
the entire alist in the selected format.

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

[Liu Hui]

Eli Zaretskii <eliz@gnu.org> 于2023年3月26日周日 13:20写道：

> Ugh!  This feature was not thought out well enough when it was
> introduced: if the user changes the value half-way through a session,
> the history will record visited files twice, under 2 different
> file-name formats and with different places recorded.  I think
> changing the value of save-place-abbreviate-file-names should rewrite
> the entire alist in the selected format.

I agree that it is better to avoid mixed file name formats, and then
save-place-find-file-hook can be fixed simply. The difficult part is
how to rewrite save-place-alist automatically. Otherwise users need to
convert the format of file names manually, which, I think, is also
acceptable.

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Tue, 28 Mar 2023 13:56:05 +0800
> Cc: 62413@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> 于2023年3月26日周日 13:20写道：
> 
> > Ugh!  This feature was not thought out well enough when it was
> > introduced: if the user changes the value half-way through a session,
> > the history will record visited files twice, under 2 different
> > file-name formats and with different places recorded.  I think
> > changing the value of save-place-abbreviate-file-names should rewrite
> > the entire alist in the selected format.
> 
> I agree that it is better to avoid mixed file name formats, and then
> save-place-find-file-hook can be fixed simply. The difficult part is
> how to rewrite save-place-alist automatically.

Isn't it just a matter of going through the list and calling
abbreviate-file-name on each file name there?

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

[Liu Hui]

Eli Zaretskii <eliz@gnu.org> 于2023年3月28日周二 20:03写道：
>
> > From: Liu Hui <liuhui1610@gmail.com>
> > Date: Tue, 28 Mar 2023 13:56:05 +0800
> > Cc: 62413@debbugs.gnu.org
> >
> > Eli Zaretskii <eliz@gnu.org> 于2023年3月26日周日 13:20写道：
> >
> > > Ugh!  This feature was not thought out well enough when it was
> > > introduced: if the user changes the value half-way through a session,
> > > the history will record visited files twice, under 2 different
> > > file-name formats and with different places recorded.  I think
> > > changing the value of save-place-abbreviate-file-names should rewrite
> > > the entire alist in the selected format.
> >
> > I agree that it is better to avoid mixed file name formats, and then
> > save-place-find-file-hook can be fixed simply. The difficult part is
> > how to rewrite save-place-alist automatically.
>
> Isn't it just a matter of going through the list and calling
> abbreviate-file-name on each file name there?

The conversion itself is easy. But users can change the value of
save-place-abbreviate-file-names anytime. To make sure the list is
always consistent with save-place-abbreviate-file-names, I think an
internal variable is needed to record the old value. If they are
different when save-place-to-alist is called, we rewrite the list. Is
it OK?

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Thu, 30 Mar 2023 10:49:39 +0800
> Cc: 62413@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> 于2023年3月28日周二 20:03写道：
> >
> > > I agree that it is better to avoid mixed file name formats, and then
> > > save-place-find-file-hook can be fixed simply. The difficult part is
> > > how to rewrite save-place-alist automatically.
> >
> > Isn't it just a matter of going through the list and calling
> > abbreviate-file-name on each file name there?
> 
> The conversion itself is easy. But users can change the value of
> save-place-abbreviate-file-names anytime. To make sure the list is
> always consistent with save-place-abbreviate-file-names, I think an
> internal variable is needed to record the old value. If they are
> different when save-place-to-alist is called, we rewrite the list. Is
> it OK?

I think there's a cleaner way: a defcustom can have a :set function,
which is called each time the variable is customized; this setter
function should be defined for a defcustom when changing its value has
non-trivial effects.  So we can define such a setter function to
rewrite the list, and document in the doc string of the defcustom that
users should not just set the value with setq, but instead use either
setopt or "M-x customize-variable".  WDYT?

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

[Liu Hui]

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

Eli Zaretskii <eliz@gnu.org> 于2023年3月30日周四 13:33写道：
>
> I think there's a cleaner way: a defcustom can have a :set function,
> which is called each time the variable is customized; this setter
> function should be defined for a defcustom when changing its value has
> non-trivial effects.  So we can define such a setter function to
> rewrite the list, and document in the doc string of the defcustom that
> users should not just set the value with setq, but instead use either
> setopt or "M-x customize-variable".  WDYT?

OK, I think it is good. Please see the attached patch.

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

From 89fba3250a41ed956b36414912b7fac72450c5f3 Mon Sep 17 00:00:00 2001
From: Liu Hui <liuhui1610@gmail.com>
Date: Mon, 3 Apr 2023 08:47:11 +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-mode): Make sure `save-place-alist' is loaded.
(save-place-to-alist): Save Abbreviated dired-filename.
(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 | 79 ++++++++++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 22 deletions(-)

diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 7512fc87c5d..543c72ee511 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)

@@ -90,8 +92,32 @@ save-place-forget-unreadable-files
 (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
+first turn on `save-place-mode' to load `save-place-alist', and
+then use either `setopt' or M-x customize-variable to set this
+option."
   :type 'boolean
+  :set (lambda (sym val)
+         (set-default sym val)
+         (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
@@ -153,6 +179,7 @@ save-place-mode
 where it was when you previously visited the same file."
   :global t
   :group 'save-place
+  (or save-place-loaded (save-place-load-alist-from-file))
   (save-place--setup-hooks save-place-mode))

 (make-variable-buffer-local 'save-place-mode)
@@ -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)))
 				 (point))))
 			    (t (point)))))
         (if cell
@@ -353,7 +384,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 +403,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

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

[Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Liu Hui <liuhui1610@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> 于2023年3月30日周四 13:33写道：
>>
>> I think there's a cleaner way: a defcustom can have a :set function,
>> which is called each time the variable is customized; this setter
>> function should be defined for a defcustom when changing its value has
>> non-trivial effects.  So we can define such a setter function to
>> rewrite the list, and document in the doc string of the defcustom that
>> users should not just set the value with setq, but instead use either
>> setopt or "M-x customize-variable".  WDYT?
>
> OK, I think it is good. Please see the attached patch.
>
> [2. text/x-patch; 0001-Restore-positions-reliably-for-abbreviated-file-name.patch]...

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.

> @@ -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?

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?

-- 
Best,


RY

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

[Liu Hui]

[-- 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

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

[Eli Zaretskii]

> From: Liu Hui <liuhui1610@gmail.com>
> Date: Tue, 4 Apr 2023 09:37:26 +0800
> Cc: Eli Zaretskii <eliz@gnu.org>, 62413@debbugs.gnu.org, bug-gnu-emacs@gnu.org
> 
> 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.

Thanks, I installed this on the master branch, and I'm therefore
closing this bug.