* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position @ 2023-03-24 2:19 Liu Hui 2023-03-25 11:52 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-03-24 2:19 UTC (permalink / raw) To: 62413 [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-24 2:19 bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position Liu Hui @ 2023-03-25 11:52 ` Eli Zaretskii 2023-03-25 14:14 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2023-03-25 11:52 UTC (permalink / raw) To: Liu Hui; +Cc: 62413 > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-25 11:52 ` Eli Zaretskii @ 2023-03-25 14:14 ` Liu Hui 2023-03-25 14:17 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-03-25 14:14 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 62413 [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-25 14:14 ` Liu Hui @ 2023-03-25 14:17 ` Eli Zaretskii 2023-03-26 1:26 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2023-03-25 14:17 UTC (permalink / raw) To: Liu Hui; +Cc: 62413 > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-25 14:17 ` Eli Zaretskii @ 2023-03-26 1:26 ` Liu Hui 2023-03-26 5:20 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-03-26 1:26 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 62413 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-26 1:26 ` Liu Hui @ 2023-03-26 5:20 ` Eli Zaretskii 2023-03-28 5:56 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2023-03-26 5:20 UTC (permalink / raw) To: Liu Hui; +Cc: 62413 > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-26 5:20 ` Eli Zaretskii @ 2023-03-28 5:56 ` Liu Hui 2023-03-28 12:03 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-03-28 5:56 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 62413 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-28 5:56 ` Liu Hui @ 2023-03-28 12:03 ` Eli Zaretskii 2023-03-30 2:49 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2023-03-28 12:03 UTC (permalink / raw) To: Liu Hui; +Cc: 62413 > 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-28 12:03 ` Eli Zaretskii @ 2023-03-30 2:49 ` Liu Hui 2023-03-30 5:34 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-03-30 2:49 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 62413 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-30 2:49 ` Liu Hui @ 2023-03-30 5:34 ` Eli Zaretskii 2023-04-03 1:02 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Eli Zaretskii @ 2023-03-30 5:34 UTC (permalink / raw) To: Liu Hui; +Cc: 62413 > 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-03-30 5:34 ` Eli Zaretskii @ 2023-04-03 1:02 ` Liu Hui 2023-04-03 2:45 ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-04-03 1:02 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 62413 [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-04-03 1:02 ` Liu Hui @ 2023-04-03 2:45 ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-04-04 1:37 ` Liu Hui 0 siblings, 1 reply; 14+ messages in thread From: Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-04-03 2:45 UTC (permalink / raw) To: Liu Hui; +Cc: eliz, 62413 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-04-03 2:45 ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2023-04-04 1:37 ` Liu Hui 2023-04-06 10:27 ` Eli Zaretskii 0 siblings, 1 reply; 14+ messages in thread From: Liu Hui @ 2023-04-04 1:37 UTC (permalink / raw) To: Ruijie Yu; +Cc: eliz, 62413 [-- 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position 2023-04-04 1:37 ` Liu Hui @ 2023-04-06 10:27 ` Eli Zaretskii 0 siblings, 0 replies; 14+ messages in thread From: Eli Zaretskii @ 2023-04-06 10:27 UTC (permalink / raw) To: Liu Hui; +Cc: ruijie, 62413-done > 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-04-06 10:27 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-24 2:19 bug#62413: 29.0.60; [PATCH] save-place-mode cannot restore saved position Liu Hui 2023-03-25 11:52 ` Eli Zaretskii 2023-03-25 14:14 ` Liu Hui 2023-03-25 14:17 ` Eli Zaretskii 2023-03-26 1:26 ` Liu Hui 2023-03-26 5:20 ` Eli Zaretskii 2023-03-28 5:56 ` Liu Hui 2023-03-28 12:03 ` Eli Zaretskii 2023-03-30 2:49 ` Liu Hui 2023-03-30 5:34 ` Eli Zaretskii 2023-04-03 1:02 ` Liu Hui 2023-04-03 2:45 ` Ruijie Yu via Bug reports for GNU Emacs, the Swiss army knife of text editors 2023-04-04 1:37 ` Liu Hui 2023-04-06 10:27 ` Eli Zaretskii
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).