* bug#15329: saveplace restores dired positions to random places
@ 2013-09-10 20:45 Juri Linkov
2013-09-11 20:45 ` Juri Linkov
2013-09-11 20:45 ` Juri Linkov
0 siblings, 2 replies; 17+ messages in thread
From: Juri Linkov @ 2013-09-10 20:45 UTC (permalink / raw)
To: 15329
Tags: patch
Remembering dired positions is a good feature, but
unfortunately in its current implementation is useless.
The problem is that often the position of point in Dired
is restored to random places because in Dired buffers
it depends on many irrelevant ever-changing factors
such as the total size of all files in the directory
and available space (thus the line with this information
changes its length), sizes of each file above the restored
file in the Dired buffer also accounts to the incorrect position
of point, also added/deleted files in Dired change the restored
position to random places, different sorting order, etc.
An obvious solution to this problem is to save the
current file name in Dired instead of its point.
It seems saving information other than point doesn't contradict
the purpose of saveplace.el that automatically saves places where
visiting them later automatically moves point to the saved position.
Also elements of `save-place-alist' are (FILENAME . POSITION),
not more limited (FILENAME . POINT). So saving a string to
POSITION would be consistent with the design of saveplace.el.
=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el 2013-06-14 09:32:01 +0000
+++ lisp/saveplace.el 2013-09-10 20:44:01 +0000
@@ -176,14 +178,17 @@ (defun save-place-to-alist ()
(not (string-match save-place-ignore-files-regexp
item))))
(let ((cell (assoc item save-place-alist))
- (position (if (not (eq major-mode 'hexl-mode))
- (point)
- (with-no-warnings
- (1+ (hexl-current-address))))))
+ (position (cond ((eq major-mode 'hexl-mode)
+ (with-no-warnings
+ (1+ (hexl-current-address))))
+ ((derived-mode-p 'dired-mode)
+ (or (dired-get-filename nil t) (point)))
+ (t (point)))))
(if cell
(setq save-place-alist (delq cell save-place-alist)))
(if (and save-place
- (not (= position 1))) ;; Optimize out the degenerate case.
+ (not (and (integerp position)
+ (= position 1)))) ;; Optimize out the degenerate case.
(setq save-place-alist
(cons (cons item position)
save-place-alist)))))))
@@ -298,7 +304,8 @@ (defun save-place-find-file-hook ()
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (and (integerp (cdr cell))
+ (goto-char (cdr cell))))
;; and make sure it will be saved again for later
(setq save-place t)))))
@@ -309,7 +318,10 @@ (defun save-place-dired-hook ()
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (cond ((integerp (cdr cell))
+ (goto-char (cdr cell)))
+ ((stringp (cdr cell))
+ (dired-goto-file (cdr cell)))))
;; and make sure it will be saved again for later
(setq save-place t)))))
^ permalink raw reply [flat|nested] 17+ messages in thread* bug#15329: saveplace restores dired positions to random places
2013-09-10 20:45 bug#15329: saveplace restores dired positions to random places Juri Linkov
@ 2013-09-11 20:45 ` Juri Linkov
2013-09-11 20:45 ` Juri Linkov
1 sibling, 0 replies; 17+ messages in thread
From: Juri Linkov @ 2013-09-11 20:45 UTC (permalink / raw)
To: 15329; +Cc: emacs-devel
> Remembering dired positions is a good feature, but
> unfortunately in its current implementation is useless.
>
> The problem is that often the position of point in Dired
> is restored to random places because in Dired buffers
> it depends on many irrelevant ever-changing factors
> such as the total size of all files in the directory
> and available space (thus the line with this information
> changes its length), sizes of each file above the restored
> file in the Dired buffer also accounts to the incorrect position
> of point, also added/deleted files in Dired change the restored
> position to random places, different sorting order, etc.
>
> An obvious solution to this problem is to save the
> current file name in Dired instead of its point.
>
> It seems saving information other than point doesn't contradict
> the purpose of saveplace.el that automatically saves places where
> visiting them later automatically moves point to the saved position.
> Also elements of `save-place-alist' are (FILENAME . POSITION),
> not more limited (FILENAME . POINT). So saving a string to
> POSITION would be consistent with the design of saveplace.el.
I realized this is not backward-compatible change, i.e.
older Emacs versions won't be able to read saved places
in the new format. Unfortunately, Dired filename positions
can't be added to the current format that uses an alist
with cons pair cells. I mean (FILENAME . POINT)
can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
because older Emacs versions will fail to read them.
The only solution that I see is to save two alists to the places file:
((FILENAME1 . POINT1)
(FILENAME2 . POINT2)
...)
((FILENAME1 DIRED-FILENAME-POSITION1)
(FILENAME2 DIRED-FILENAME-POSITION2)
...)
Then the current code:
(with-demoted-errors
(car (read-from-string
(buffer-substring (point-min) (point-max)))))
will read the first alist without problems,
and additional code in newer versions like
(with-demoted-errors
(cadr (read-from-string
(buffer-substring (point-min) (point-max)))))
will read the second alist with more information
about the context of saved places (like bookmarks).
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: bug#15329: saveplace restores dired positions to random places
2013-09-10 20:45 bug#15329: saveplace restores dired positions to random places Juri Linkov
2013-09-11 20:45 ` Juri Linkov
@ 2013-09-11 20:45 ` Juri Linkov
2013-09-12 16:12 ` Karl Fogel
2013-09-12 16:12 ` Karl Fogel
1 sibling, 2 replies; 17+ messages in thread
From: Juri Linkov @ 2013-09-11 20:45 UTC (permalink / raw)
To: 15329; +Cc: emacs-devel
> Remembering dired positions is a good feature, but
> unfortunately in its current implementation is useless.
>
> The problem is that often the position of point in Dired
> is restored to random places because in Dired buffers
> it depends on many irrelevant ever-changing factors
> such as the total size of all files in the directory
> and available space (thus the line with this information
> changes its length), sizes of each file above the restored
> file in the Dired buffer also accounts to the incorrect position
> of point, also added/deleted files in Dired change the restored
> position to random places, different sorting order, etc.
>
> An obvious solution to this problem is to save the
> current file name in Dired instead of its point.
>
> It seems saving information other than point doesn't contradict
> the purpose of saveplace.el that automatically saves places where
> visiting them later automatically moves point to the saved position.
> Also elements of `save-place-alist' are (FILENAME . POSITION),
> not more limited (FILENAME . POINT). So saving a string to
> POSITION would be consistent with the design of saveplace.el.
I realized this is not backward-compatible change, i.e.
older Emacs versions won't be able to read saved places
in the new format. Unfortunately, Dired filename positions
can't be added to the current format that uses an alist
with cons pair cells. I mean (FILENAME . POINT)
can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
because older Emacs versions will fail to read them.
The only solution that I see is to save two alists to the places file:
((FILENAME1 . POINT1)
(FILENAME2 . POINT2)
...)
((FILENAME1 DIRED-FILENAME-POSITION1)
(FILENAME2 DIRED-FILENAME-POSITION2)
...)
Then the current code:
(with-demoted-errors
(car (read-from-string
(buffer-substring (point-min) (point-max)))))
will read the first alist without problems,
and additional code in newer versions like
(with-demoted-errors
(cadr (read-from-string
(buffer-substring (point-min) (point-max)))))
will read the second alist with more information
about the context of saved places (like bookmarks).
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: bug#15329: saveplace restores dired positions to random places
2013-09-11 20:45 ` Juri Linkov
@ 2013-09-12 16:12 ` Karl Fogel
2013-09-12 19:13 ` Stefan Monnier
` (3 more replies)
2013-09-12 16:12 ` Karl Fogel
1 sibling, 4 replies; 17+ messages in thread
From: Karl Fogel @ 2013-09-12 16:12 UTC (permalink / raw)
To: emacs-devel; +Cc: 15329
Juri Linkov wrote:
>> An obvious solution to this problem is to save the
>> current file name in Dired instead of its point.
>>
>> It seems saving information other than point doesn't contradict
>> the purpose of saveplace.el that automatically saves places where
>> visiting them later automatically moves point to the saved position.
>> Also elements of `save-place-alist' are (FILENAME . POSITION),
>> not more limited (FILENAME . POINT). So saving a string to
>> POSITION would be consistent with the design of saveplace.el.
>
>I realized this is not backward-compatible change, i.e.
>older Emacs versions won't be able to read saved places
>in the new format. Unfortunately, Dired filename positions
>can't be added to the current format that uses an alist
>with cons pair cells. I mean (FILENAME . POINT)
>can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
>because older Emacs versions will fail to read them.
>
>The only solution that I see is to save two alists to the places file:
>
>((FILENAME1 . POINT1)
> (FILENAME2 . POINT2)
> ...)
>((FILENAME1 DIRED-FILENAME-POSITION1)
> (FILENAME2 DIRED-FILENAME-POSITION2)
> ...)
>
>Then the current code:
>
> (with-demoted-errors
> (car (read-from-string
> (buffer-substring (point-min) (point-max)))))
>
>will read the first alist without problems,
>and additional code in newer versions like
>
> (with-demoted-errors
> (cadr (read-from-string
> (buffer-substring (point-min) (point-max)))))
>
>will read the second alist with more information
>about the context of saved places (like bookmarks).
Well, rather, we could use that as an upgrade strategy for the saveplace
format as a whole. In other words, starting now and for a few versions
of Emacs into the future, write the old format to the first part of the
file, and then a more flexible new format to the second part. The
modern format would have a more extensible structure, similarly to how
bookmark.el does it. Say, a sublist whose first element is the type of
the record, and the rest of which is the data for that record. Like:
((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
(FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
...)
etc.
Thoughts?
-Karl
^ permalink raw reply [flat|nested] 17+ messages in thread* bug#15329: saveplace restores dired positions to random places
2013-09-12 16:12 ` Karl Fogel
@ 2013-09-12 19:13 ` Stefan Monnier
2013-09-12 19:13 ` Stefan Monnier
` (2 subsequent siblings)
3 siblings, 0 replies; 17+ messages in thread
From: Stefan Monnier @ 2013-09-12 19:13 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329, emacs-devel
> ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
> (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
> ...)
Re-using as much as possible of bookmarks's format would make sense.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: bug#15329: saveplace restores dired positions to random places
2013-09-12 16:12 ` Karl Fogel
2013-09-12 19:13 ` Stefan Monnier
@ 2013-09-12 19:13 ` Stefan Monnier
2013-09-12 20:52 ` Juri Linkov
2013-09-12 20:52 ` Juri Linkov
3 siblings, 0 replies; 17+ messages in thread
From: Stefan Monnier @ 2013-09-12 19:13 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329, emacs-devel
> ((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
> (FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
> ...)
Re-using as much as possible of bookmarks's format would make sense.
Stefan
^ permalink raw reply [flat|nested] 17+ messages in thread* bug#15329: saveplace restores dired positions to random places
2013-09-12 16:12 ` Karl Fogel
2013-09-12 19:13 ` Stefan Monnier
2013-09-12 19:13 ` Stefan Monnier
@ 2013-09-12 20:52 ` Juri Linkov
2013-09-12 20:52 ` Juri Linkov
3 siblings, 0 replies; 17+ messages in thread
From: Juri Linkov @ 2013-09-12 20:52 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329, emacs-devel
> The modern format would have a more extensible structure, similarly to how
> bookmark.el does it. Say, a sublist whose first element is the type of
> the record, and the rest of which is the data for that record.
Do you think it would be possible to use the existing
infrastructure of bookmark.el, so for instance, to save
a place in an Info manual, saveplace.el could call
`Info-bookmark-make-record' and to restore it with
`Info-bookmark-jump'. This would be better than adding
a third hook for saveplace (the second existing hook
is desktop-specific like `Info-desktop-buffer-misc-data'
and `Info-restore-desktop-buffer').
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: bug#15329: saveplace restores dired positions to random places
2013-09-12 16:12 ` Karl Fogel
` (2 preceding siblings ...)
2013-09-12 20:52 ` Juri Linkov
@ 2013-09-12 20:52 ` Juri Linkov
2013-10-03 21:37 ` Karl Fogel
2013-10-03 21:37 ` Karl Fogel
3 siblings, 2 replies; 17+ messages in thread
From: Juri Linkov @ 2013-09-12 20:52 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329, emacs-devel
> The modern format would have a more extensible structure, similarly to how
> bookmark.el does it. Say, a sublist whose first element is the type of
> the record, and the rest of which is the data for that record.
Do you think it would be possible to use the existing
infrastructure of bookmark.el, so for instance, to save
a place in an Info manual, saveplace.el could call
`Info-bookmark-make-record' and to restore it with
`Info-bookmark-jump'. This would be better than adding
a third hook for saveplace (the second existing hook
is desktop-specific like `Info-desktop-buffer-misc-data'
and `Info-restore-desktop-buffer').
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: bug#15329: saveplace restores dired positions to random places
2013-09-12 20:52 ` Juri Linkov
@ 2013-10-03 21:37 ` Karl Fogel
2013-12-15 20:20 ` Juri Linkov
2013-10-03 21:37 ` Karl Fogel
1 sibling, 1 reply; 17+ messages in thread
From: Karl Fogel @ 2013-10-03 21:37 UTC (permalink / raw)
To: Juri Linkov; +Cc: 15329, emacs-devel
Juri Linkov <juri@jurta.org> writes:
>> The modern format would have a more extensible structure, similarly to how
>> bookmark.el does it. Say, a sublist whose first element is the type of
>> the record, and the rest of which is the data for that record.
>
>Do you think it would be possible to use the existing
>infrastructure of bookmark.el, so for instance, to save
>a place in an Info manual, saveplace.el could call
>`Info-bookmark-make-record' and to restore it with
>`Info-bookmark-jump'. This would be better than adding
>a third hook for saveplace (the second existing hook
>is desktop-specific like `Info-desktop-buffer-misc-data'
>and `Info-restore-desktop-buffer').
Yes; since both saveplace and bookmark are in the standard Emacs dist,
it's fine for them to share code, and would improve maintainability.
I'm not sure when I'll get a chance to work on this, though. The
original bug here is about saving/restoring position in dired buffers.
While the current rather random behavior in dired is obviously a bug,
and fixing it would be a Good Thing, I'm not sure it rises to the level
where I drop other things to work on it :-). However, if someone were
to write a patch (along the lines described in this bug report), I'd
certainly commit to reviewing it.
Best,
-Karl
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-10-03 21:37 ` Karl Fogel
@ 2013-12-15 20:20 ` Juri Linkov
2013-12-15 21:43 ` Drew Adams
2013-12-20 20:20 ` Juri Linkov
0 siblings, 2 replies; 17+ messages in thread
From: Juri Linkov @ 2013-12-15 20:20 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329
> While the current rather random behavior in dired is obviously a bug,
> and fixing it would be a Good Thing, I'm not sure it rises to the level
> where I drop other things to work on it :-). However, if someone were
> to write a patch (along the lines described in this bug report), I'd
> certainly commit to reviewing it.
I realized now that the current format is completely backward-compatible.
When `find-file-hook' restores the saved position in a file, it doesn't see
the dired entries (that end with a directory separator) in the new format.
As an additional benefit, the users can add a regexp like "/$" to
`save-place-ignore-files-regexp' to not save dired positions.
More packages can do the same later, e.g. Info could save the entries like
"(emacs) Top" with own additional information that only Info will restore, etc.
So now dired positions should be fixed by this patch:
=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el 2013-09-12 05:32:57 +0000
+++ lisp/saveplace.el 2013-12-15 20:12:27 +0000
@@ -152,8 +152,8 @@ (defun toggle-save-place (&optional parg
\(setq-default save-place t\)"
(interactive "P")
- (if (not buffer-file-name)
- (message "Buffer `%s' not visiting a file" (buffer-name))
+ (if (not (or buffer-file-name dired-directory))
+ (message "Buffer `%s' not visiting a file or directory" (buffer-name))
(if (and save-place (or (not parg) (<= parg 0)))
(progn
(message "No place will be saved in this file")
@@ -161,6 +161,8 @@ (defun toggle-save-place (&optional parg
(message "Place will be saved")
(setq save-place t))))
+(declare-function dired-get-filename "dired" (&optional localp no-error-if-not-filep))
+
(defun save-place-to-alist ()
;; put filename and point in a cons box and then cons that onto the
;; front of the save-place-alist, if save-place is non-nil.
@@ -176,14 +178,20 @@ (defun save-place-to-alist ()
(not (string-match save-place-ignore-files-regexp
item))))
(let ((cell (assoc item save-place-alist))
- (position (if (not (eq major-mode 'hexl-mode))
- (point)
- (with-no-warnings
- (1+ (hexl-current-address))))))
+ (position (cond ((eq major-mode 'hexl-mode)
+ (with-no-warnings
+ (1+ (hexl-current-address))))
+ ((derived-mode-p 'dired-mode)
+ (let ((filename (dired-get-filename nil t)))
+ (if filename
+ `((dired-filename . ,filename))
+ (point))))
+ (t (point)))))
(if cell
(setq save-place-alist (delq cell save-place-alist)))
(if (and save-place
- (not (= position 1))) ;; Optimize out the degenerate case.
+ (not (and (integerp position)
+ (= position 1)))) ;; Optimize out the degenerate case.
(setq save-place-alist
(cons (cons item position)
save-place-alist)))))))
@@ -290,7 +298,8 @@ (defun save-places-to-alist ()
(with-current-buffer (car buf-list)
;; save-place checks buffer-file-name too, but we can avoid
;; overhead of function call by checking here too.
- (and buffer-file-name (save-place-to-alist))
+ (and (or buffer-file-name dired-directory)
+ (save-place-to-alist))
(setq buf-list (cdr buf-list))))))
(defun save-place-find-file-hook ()
@@ -299,10 +308,13 @@ (defun save-place-find-file-hook ()
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (and (integerp (cdr cell))
+ (goto-char (cdr cell))))
;; and make sure it will be saved again for later
(setq save-place t)))))
+(declare-function dired-goto-file "dired" (file))
+
(defun save-place-dired-hook ()
"Position the point in a dired buffer."
(or save-place-loaded (load-save-place-alist-from-file))
@@ -310,7 +322,10 @@ (defun save-place-dired-hook ()
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (if (integerp (cdr cell))
+ (goto-char (cdr cell))
+ (and (assq 'dired-filename (cdr cell))
+ (dired-goto-file (cdr (assq 'dired-filename (cdr cell)))))))
;; and make sure it will be saved again for later
(setq save-place t)))))
^ permalink raw reply [flat|nested] 17+ messages in thread* bug#15329: saveplace restores dired positions to random places
2013-12-15 20:20 ` Juri Linkov
@ 2013-12-15 21:43 ` Drew Adams
2013-12-16 20:58 ` Juri Linkov
2013-12-20 20:20 ` Juri Linkov
1 sibling, 1 reply; 17+ messages in thread
From: Drew Adams @ 2013-12-15 21:43 UTC (permalink / raw)
To: Juri Linkov, Karl Fogel; +Cc: 15329
> - (if (not buffer-file-name)
> - (message "Buffer `%s' not visiting a file" (buffer-name))
> + (if (not (or buffer-file-name dired-directory))
> + (message "Buffer `%s' not visiting a file or directory"
> + (buffer-name))
Is `dired-directory' really the right test here? I am used to seeing
(derived-mode-p 'dired-mode) for that purpose (assuming I understand the
purpose here).
There is, BTW, nothing in the doc string of `dired-directory' that says
what a nil value means. Should code now instead be using
`dired-directory' to test whether the mode is (derived from) Dired?
If so, then at the very least the doc string of that variable should
describe such a Boolean meaning: nil means not in Dired mode or a mode
derived from it (or whatever the completely correct interpretation is).
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-12-15 21:43 ` Drew Adams
@ 2013-12-16 20:58 ` Juri Linkov
2013-12-16 21:15 ` Drew Adams
0 siblings, 1 reply; 17+ messages in thread
From: Juri Linkov @ 2013-12-16 20:58 UTC (permalink / raw)
To: Drew Adams; +Cc: Karl Fogel, 15329
>> - (if (not buffer-file-name)
>> - (message "Buffer `%s' not visiting a file" (buffer-name))
>> + (if (not (or buffer-file-name dired-directory))
>> + (message "Buffer `%s' not visiting a file or directory"
>> + (buffer-name))
>
> Is `dired-directory' really the right test here? I am used to seeing
> (derived-mode-p 'dired-mode) for that purpose (assuming I understand the
> purpose here).
Yes, `dired-directory' is the right test, because `dired-directory'
is used as a key in `save-place-alist'.
> There is, BTW, nothing in the doc string of `dired-directory' that says
> what a nil value means.
Thanks for pointing to the doc string of `dired-directory' where I noticed:
May be a list, in which case the car is the
directory name and the cdr is the list of files to mention.
This case should be handled properly that I added to the following patch.
> Should code now instead be using `dired-directory' to test whether the
> mode is (derived from) Dired?
Yes, code should be using `dired-directory' now.
> If so, then at the very least the doc string of that variable should
> describe such a Boolean meaning: nil means not in Dired mode or a mode
> derived from it (or whatever the completely correct interpretation is).
Are you sure that nil means not in Dired mode or a mode derived from it?
Anyway, this is a new version:
=== modified file 'lisp/saveplace.el'
--- lisp/saveplace.el 2013-09-12 05:32:57 +0000
+++ lisp/saveplace.el 2013-12-16 20:56:14 +0000
@@ -152,8 +152,8 @@ (defun toggle-save-place (&optional parg
\(setq-default save-place t\)"
(interactive "P")
- (if (not buffer-file-name)
- (message "Buffer `%s' not visiting a file" (buffer-name))
+ (if (not (or buffer-file-name dired-directory))
+ (message "Buffer `%s' not visiting a file or directory" (buffer-name))
(if (and save-place (or (not parg) (<= parg 0)))
(progn
(message "No place will be saved in this file")
@@ -161,6 +161,8 @@ (defun toggle-save-place (&optional parg
(message "Place will be saved")
(setq save-place t))))
+(declare-function dired-get-filename "dired" (&optional localp no-error-if-not-filep))
+
(defun save-place-to-alist ()
;; put filename and point in a cons box and then cons that onto the
;; front of the save-place-alist, if save-place is non-nil.
@@ -170,20 +172,29 @@ (defun save-place-to-alist ()
;; will be saved again when Emacs is killed.
(or save-place-loaded (load-save-place-alist-from-file))
(let ((item (or buffer-file-name
- (and dired-directory (expand-file-name dired-directory)))))
+ (and dired-directory
+ (if (consp dired-directory)
+ (expand-file-name (car dired-directory))
+ (expand-file-name dired-directory))))))
(when (and item
(or (not save-place-ignore-files-regexp)
(not (string-match save-place-ignore-files-regexp
item))))
(let ((cell (assoc item save-place-alist))
- (position (if (not (eq major-mode 'hexl-mode))
- (point)
- (with-no-warnings
- (1+ (hexl-current-address))))))
+ (position (cond ((eq major-mode 'hexl-mode)
+ (with-no-warnings
+ (1+ (hexl-current-address))))
+ (dired-directory
+ (let ((filename (dired-get-filename nil t)))
+ (if filename
+ `((dired-filename . ,filename))
+ (point))))
+ (t (point)))))
(if cell
(setq save-place-alist (delq cell save-place-alist)))
(if (and save-place
- (not (= position 1))) ;; Optimize out the degenerate case.
+ (not (and (integerp position)
+ (= position 1)))) ;; Optimize out the degenerate case.
(setq save-place-alist
(cons (cons item position)
save-place-alist)))))))
@@ -290,7 +301,8 @@ (defun save-places-to-alist ()
(with-current-buffer (car buf-list)
;; save-place checks buffer-file-name too, but we can avoid
;; overhead of function call by checking here too.
- (and buffer-file-name (save-place-to-alist))
+ (and (or buffer-file-name dired-directory)
+ (save-place-to-alist))
(setq buf-list (cdr buf-list))))))
(defun save-place-find-file-hook ()
@@ -299,18 +311,27 @@ (defun save-place-find-file-hook ()
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (and (integerp (cdr cell))
+ (goto-char (cdr cell))))
;; and make sure it will be saved again for later
(setq save-place t)))))
+(declare-function dired-goto-file "dired" (file))
+
(defun save-place-dired-hook ()
"Position the point in a dired buffer."
(or save-place-loaded (load-save-place-alist-from-file))
- (let ((cell (assoc (expand-file-name dired-directory) save-place-alist)))
+ (let ((cell (assoc (if (consp dired-directory)
+ (expand-file-name (car dired-directory))
+ (expand-file-name dired-directory))
+ save-place-alist)))
(if cell
(progn
(or revert-buffer-in-progress-p
- (goto-char (cdr cell)))
+ (if (integerp (cdr cell))
+ (goto-char (cdr cell))
+ (and (assq 'dired-filename (cdr cell))
+ (dired-goto-file (cdr (assq 'dired-filename (cdr cell)))))))
;; and make sure it will be saved again for later
(setq save-place t)))))
^ permalink raw reply [flat|nested] 17+ messages in thread* bug#15329: saveplace restores dired positions to random places
2013-12-16 20:58 ` Juri Linkov
@ 2013-12-16 21:15 ` Drew Adams
0 siblings, 0 replies; 17+ messages in thread
From: Drew Adams @ 2013-12-16 21:15 UTC (permalink / raw)
To: Juri Linkov; +Cc: Karl Fogel, 15329
> > Is `dired-directory' really the right test here? I am used to seeing
> > (derived-mode-p 'dired-mode) for that purpose (assuming I understand the
> > purpose here).
>
> Yes, `dired-directory' is the right test, because `dired-directory'
> is used as a key in `save-place-alist'.
OK.
> > There is, BTW, nothing in the doc string of `dired-directory' that says
> > what a nil value means.
>
> Thanks for pointing to the doc string of `dired-directory' where I noticed:
>
> May be a list, in which case the car is the
> directory name and the cdr is the list of files to mention.
Yes, I use that use case quite a lot.
> This case should be handled properly that I added to the following patch.
Great.
> > Should code now instead be using `dired-directory' to test whether the
> > mode is (derived from) Dired?
>
> Yes, code should be using `dired-directory' now.
OK. Do you happen to know whether that is only the case now or has been the
case all along?
`dired-directory' is old, but I have always seen `derived-mode-p' used for
the test - and I see it still, e.g., for `dired-toggle-read-only'. Or does
it perhaps not matter at all which we use? Looking for what is best to use
with multiple Emacs versions.
> > If so, then at the very least the doc string of that variable should
> > describe such a Boolean meaning: nil means not in Dired mode or a mode
> > derived from it (or whatever the completely correct interpretation is).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> Are you sure that nil means not in Dired mode or a mode derived from it?
I posed a question; I didn't make a statement. Let's please describe what
nil means here, whatever that might be.
> Anyway, this is a new version:
Thanks for taking the cons case into account.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-12-15 20:20 ` Juri Linkov
2013-12-15 21:43 ` Drew Adams
@ 2013-12-20 20:20 ` Juri Linkov
2013-12-21 2:09 ` Karl Fogel
1 sibling, 1 reply; 17+ messages in thread
From: Juri Linkov @ 2013-12-20 20:20 UTC (permalink / raw)
To: Karl Fogel; +Cc: 15329-done
> I realized now that the current format is completely backward-compatible.
> When `find-file-hook' restores the saved position in a file, it doesn't see
> the dired entries (that end with a directory separator) in the new format.
Karl, due to the imminent feature freeze I had to commit the patch now.
Please revert it if you don't agree with this implementation.
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-12-20 20:20 ` Juri Linkov
@ 2013-12-21 2:09 ` Karl Fogel
0 siblings, 0 replies; 17+ messages in thread
From: Karl Fogel @ 2013-12-21 2:09 UTC (permalink / raw)
To: Juri Linkov; +Cc: 15329-done
Juri Linkov <juri@jurta.org> writes:
>> I realized now that the current format is completely backward-compatible.
>> When `find-file-hook' restores the saved position in a file, it doesn't see
>> the dired entries (that end with a directory separator) in the new format.
>
>Karl, due to the imminent feature freeze I had to commit the patch now.
>Please revert it if you don't agree with this implementation.
Understood, and thank you.
Unlike the rest of the Western world, my schedule is such that I don't
have much time for Emacs hacking during the holiday season, but should
be able to pay some attention to Emacs stuff afterwards. So I'll review
this then. It's unfortunate that that review has to wait until possibly
after the release :-(, but I try to think about these things with a long
term perspective...
Best,
-K
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-09-12 20:52 ` Juri Linkov
2013-10-03 21:37 ` Karl Fogel
@ 2013-10-03 21:37 ` Karl Fogel
1 sibling, 0 replies; 17+ messages in thread
From: Karl Fogel @ 2013-10-03 21:37 UTC (permalink / raw)
To: Juri Linkov; +Cc: 15329, emacs-devel
Juri Linkov <juri@jurta.org> writes:
>> The modern format would have a more extensible structure, similarly to how
>> bookmark.el does it. Say, a sublist whose first element is the type of
>> the record, and the rest of which is the data for that record.
>
>Do you think it would be possible to use the existing
>infrastructure of bookmark.el, so for instance, to save
>a place in an Info manual, saveplace.el could call
>`Info-bookmark-make-record' and to restore it with
>`Info-bookmark-jump'. This would be better than adding
>a third hook for saveplace (the second existing hook
>is desktop-specific like `Info-desktop-buffer-misc-data'
>and `Info-restore-desktop-buffer').
Yes; since both saveplace and bookmark are in the standard Emacs dist,
it's fine for them to share code, and would improve maintainability.
I'm not sure when I'll get a chance to work on this, though. The
original bug here is about saving/restoring position in dired buffers.
While the current rather random behavior in dired is obviously a bug,
and fixing it would be a Good Thing, I'm not sure it rises to the level
where I drop other things to work on it :-). However, if someone were
to write a patch (along the lines described in this bug report), I'd
certainly commit to reviewing it.
Best,
-Karl
^ permalink raw reply [flat|nested] 17+ messages in thread
* bug#15329: saveplace restores dired positions to random places
2013-09-11 20:45 ` Juri Linkov
2013-09-12 16:12 ` Karl Fogel
@ 2013-09-12 16:12 ` Karl Fogel
1 sibling, 0 replies; 17+ messages in thread
From: Karl Fogel @ 2013-09-12 16:12 UTC (permalink / raw)
To: emacs-devel; +Cc: 15329
Juri Linkov wrote:
>> An obvious solution to this problem is to save the
>> current file name in Dired instead of its point.
>>
>> It seems saving information other than point doesn't contradict
>> the purpose of saveplace.el that automatically saves places where
>> visiting them later automatically moves point to the saved position.
>> Also elements of `save-place-alist' are (FILENAME . POSITION),
>> not more limited (FILENAME . POINT). So saving a string to
>> POSITION would be consistent with the design of saveplace.el.
>
>I realized this is not backward-compatible change, i.e.
>older Emacs versions won't be able to read saved places
>in the new format. Unfortunately, Dired filename positions
>can't be added to the current format that uses an alist
>with cons pair cells. I mean (FILENAME . POINT)
>can't be changed to (FILENAME POINT DIRED-FILENAME-POSITION)
>because older Emacs versions will fail to read them.
>
>The only solution that I see is to save two alists to the places file:
>
>((FILENAME1 . POINT1)
> (FILENAME2 . POINT2)
> ...)
>((FILENAME1 DIRED-FILENAME-POSITION1)
> (FILENAME2 DIRED-FILENAME-POSITION2)
> ...)
>
>Then the current code:
>
> (with-demoted-errors
> (car (read-from-string
> (buffer-substring (point-min) (point-max)))))
>
>will read the first alist without problems,
>and additional code in newer versions like
>
> (with-demoted-errors
> (cadr (read-from-string
> (buffer-substring (point-min) (point-max)))))
>
>will read the second alist with more information
>about the context of saved places (like bookmarks).
Well, rather, we could use that as an upgrade strategy for the saveplace
format as a whole. In other words, starting now and for a few versions
of Emacs into the future, write the old format to the first part of the
file, and then a more flexible new format to the second part. The
modern format would have a more extensible structure, similarly to how
bookmark.el does it. Say, a sublist whose first element is the type of
the record, and the rest of which is the data for that record. Like:
((FILE_OR_DIR_NAME_1 ('position (position information goes here)))
(FILE_OR_DIR_NAME_2 ('dired-position (different kind of information)))
...)
etc.
Thoughts?
-Karl
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-12-21 2:09 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-10 20:45 bug#15329: saveplace restores dired positions to random places Juri Linkov
2013-09-11 20:45 ` Juri Linkov
2013-09-11 20:45 ` Juri Linkov
2013-09-12 16:12 ` Karl Fogel
2013-09-12 19:13 ` Stefan Monnier
2013-09-12 19:13 ` Stefan Monnier
2013-09-12 20:52 ` Juri Linkov
2013-09-12 20:52 ` Juri Linkov
2013-10-03 21:37 ` Karl Fogel
2013-12-15 20:20 ` Juri Linkov
2013-12-15 21:43 ` Drew Adams
2013-12-16 20:58 ` Juri Linkov
2013-12-16 21:15 ` Drew Adams
2013-12-20 20:20 ` Juri Linkov
2013-12-21 2:09 ` Karl Fogel
2013-10-03 21:37 ` Karl Fogel
2013-09-12 16:12 ` Karl Fogel
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.