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

Previous Next

Package: emacs;

Reported by: Liu Hui <liuhui1610 <at> gmail.com>

Date: Fri, 24 Mar 2023 04:10:02 UTC

Severity: normal

Tags: patch

Found in version 29.0.60

Done: Eli Zaretskii <eliz <at> gnu.org>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62413 in the body.
You can then email your comments to 62413 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Fri, 24 Mar 2023 04:10:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Liu Hui <liuhui1610 <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Fri, 24 Mar 2023 04:10:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.60; [PATCH] save-place-mode cannot restore saved position
Date: Fri, 24 Mar 2023 10:19:07 +0800
[Message part 1 (text/plain, inline)]
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
[0001-Restore-positions-for-abbreviated-file-names.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Sat, 25 Mar 2023 11:53:01 GMT) Full text and rfc822 format available.

Message #8 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Sat, 25 Mar 2023 14:52:17 +0300
> From: Liu Hui <liuhui1610 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Sat, 25 Mar 2023 14:18:01 GMT) Full text and rfc822 format available.

Message #11 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Sat, 25 Mar 2023 17:17:11 +0300
> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Sat, 25 Mar 2023 22:14:02 +0800
> Cc: 62413 <at> 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 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Sat, 25 Mar 2023 14:23:02 GMT) Full text and rfc822 format available.

Message #14 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Sat, 25 Mar 2023 22:14:02 +0800
[Message part 1 (text/plain, inline)]
在 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.
[0001-Restore-positions-for-abbreviated-file-names-in-save.patch (application/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Sun, 26 Mar 2023 01:27:01 GMT) Full text and rfc822 format available.

Message #17 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Sun, 26 Mar 2023 09:26:22 +0800
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Sun, 26 Mar 2023 05:21:02 GMT) Full text and rfc822 format available.

Message #20 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Sun, 26 Mar 2023 08:20:48 +0300
> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Sun, 26 Mar 2023 09:26:22 +0800
> Cc: 62413 <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Tue, 28 Mar 2023 05:57:01 GMT) Full text and rfc822 format available.

Message #23 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Tue, 28 Mar 2023 13:56:05 +0800
Eli Zaretskii <eliz <at> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Tue, 28 Mar 2023 12:04:01 GMT) Full text and rfc822 format available.

Message #26 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Tue, 28 Mar 2023 15:03:20 +0300
> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Tue, 28 Mar 2023 13:56:05 +0800
> Cc: 62413 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Thu, 30 Mar 2023 02:50:02 GMT) Full text and rfc822 format available.

Message #29 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Thu, 30 Mar 2023 10:49:39 +0800
Eli Zaretskii <eliz <at> gnu.org> 于2023年3月28日周二 20:03写道:
>
> > From: Liu Hui <liuhui1610 <at> gmail.com>
> > Date: Tue, 28 Mar 2023 13:56:05 +0800
> > Cc: 62413 <at> debbugs.gnu.org
> >
> > Eli Zaretskii <eliz <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Thu, 30 Mar 2023 05:35:02 GMT) Full text and rfc822 format available.

Message #32 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Thu, 30 Mar 2023 08:34:03 +0300
> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Thu, 30 Mar 2023 10:49:39 +0800
> Cc: 62413 <at> debbugs.gnu.org
> 
> Eli Zaretskii <eliz <at> 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?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Mon, 03 Apr 2023 01:03:01 GMT) Full text and rfc822 format available.

Message #35 received at 62413 <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62413 <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Mon, 3 Apr 2023 09:02:37 +0800
[Message part 1 (text/plain, inline)]
Eli Zaretskii <eliz <at> 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.
[0001-Restore-positions-reliably-for-abbreviated-file-name.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Mon, 03 Apr 2023 03:07:01 GMT) Full text and rfc822 format available.

Message #38 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ruijie Yu <ruijie <at> netyu.xyz>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 62413 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
Subject: Re: bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore
 saved position
Date: Mon, 03 Apr 2023 10:45:34 +0800
Liu Hui <liuhui1610 <at> gmail.com> writes:

> Eli Zaretskii <eliz <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Mon, 03 Apr 2023 03:07:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Tue, 04 Apr 2023 01:38:02 GMT) Full text and rfc822 format available.

Message #44 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Liu Hui <liuhui1610 <at> gmail.com>
To: Ruijie Yu <ruijie <at> netyu.xyz>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 62413 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Tue, 4 Apr 2023 09:37:26 +0800
[Message part 1 (text/plain, inline)]
Ruijie Yu <ruijie <at> 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.
[0001-Restore-positions-reliably-for-abbreviated-file-name.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#62413; Package emacs. (Tue, 04 Apr 2023 01:38:02 GMT) Full text and rfc822 format available.

Reply sent to Eli Zaretskii <eliz <at> gnu.org>:
You have taken responsibility. (Thu, 06 Apr 2023 10:28:02 GMT) Full text and rfc822 format available.

Notification sent to Liu Hui <liuhui1610 <at> gmail.com>:
bug acknowledged by developer. (Thu, 06 Apr 2023 10:28:02 GMT) Full text and rfc822 format available.

Message #52 received at 62413-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Liu Hui <liuhui1610 <at> gmail.com>
Cc: ruijie <at> netyu.xyz, 62413-done <at> debbugs.gnu.org
Subject: Re: bug#62413: 29.0.60;
 [PATCH] save-place-mode cannot restore saved position
Date: Thu, 06 Apr 2023 13:27:55 +0300
> From: Liu Hui <liuhui1610 <at> gmail.com>
> Date: Tue, 4 Apr 2023 09:37:26 +0800
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 62413 <at> debbugs.gnu.org, bug-gnu-emacs <at> gnu.org
> 
> Ruijie Yu <ruijie <at> 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.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Thu, 04 May 2023 11:24:05 GMT) Full text and rfc822 format available.

This bug report was last modified 1 year and 4 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.