GNU bug report logs - #15329
saveplace restores dired positions to random places

Previous Next

Package: emacs;

Reported by: Juri Linkov <juri <at> jurta.org>

Date: Tue, 10 Sep 2013 20:51:02 UTC

Severity: minor

Tags: patch

Done: Juri Linkov <juri <at> jurta.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 15329 in the body.
You can then email your comments to 15329 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#15329; Package emacs. (Tue, 10 Sep 2013 20:51:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Juri Linkov <juri <at> jurta.org>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Tue, 10 Sep 2013 20:51:02 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: bug-gnu-emacs <at> gnu.org
Subject: saveplace restores dired positions to random places
Date: Tue, 10 Sep 2013 23:45:15 +0300
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)))))
 




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Wed, 11 Sep 2013 20:48:02 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: 15329 <at> debbugs.gnu.org
Cc: emacs-devel <at> gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Wed, 11 Sep 2013 23:45:11 +0300
> 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).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Thu, 12 Sep 2013 16:14:01 GMT) Full text and rfc822 format available.

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

From: Karl Fogel <kfogel <at> red-bean.com>
To: emacs-devel <at> gnu.org
Cc: 15329 <at> debbugs.gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Thu, 12 Sep 2013 11:12:58 -0500
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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Thu, 12 Sep 2013 19:14:02 GMT) Full text and rfc822 format available.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Karl Fogel <kfogel <at> red-bean.com>
Cc: 15329 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Thu, 12 Sep 2013 15:13:00 -0400
>   ((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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Thu, 12 Sep 2013 20:54:02 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: Karl Fogel <kfogel <at> red-bean.com>
Cc: 15329 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Thu, 12 Sep 2013 23:52:29 +0300
> 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').




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Thu, 03 Oct 2013 21:38:02 GMT) Full text and rfc822 format available.

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

From: Karl Fogel <kfogel <at> red-bean.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: 15329 <at> debbugs.gnu.org, emacs-devel <at> gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Thu, 03 Oct 2013 16:37:09 -0500
Juri Linkov <juri <at> 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




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Sun, 15 Dec 2013 20:23:02 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: Karl Fogel <kfogel <at> red-bean.com>
Cc: 15329 <at> debbugs.gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Sun, 15 Dec 2013 22:20:04 +0200
> 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)))))
 




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Sun, 15 Dec 2013 21:44:02 GMT) Full text and rfc822 format available.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Juri Linkov <juri <at> jurta.org>, Karl Fogel <kfogel <at> red-bean.com>
Cc: 15329 <at> debbugs.gnu.org
Subject: RE: bug#15329: saveplace restores dired positions to random places
Date: Sun, 15 Dec 2013 13:43:42 -0800 (PST)
> -  (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).




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Mon, 16 Dec 2013 21:00:03 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: Drew Adams <drew.adams <at> oracle.com>
Cc: Karl Fogel <kfogel <at> red-bean.com>, 15329 <at> debbugs.gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Mon, 16 Dec 2013 22:58:41 +0200
>> -  (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)))))
 




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Mon, 16 Dec 2013 21:16:02 GMT) Full text and rfc822 format available.

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

From: Drew Adams <drew.adams <at> oracle.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: Karl Fogel <kfogel <at> red-bean.com>, 15329 <at> debbugs.gnu.org
Subject: RE: bug#15329: saveplace restores dired positions to random places
Date: Mon, 16 Dec 2013 13:15:43 -0800 (PST)
> > 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.




Reply sent to Juri Linkov <juri <at> jurta.org>:
You have taken responsibility. (Fri, 20 Dec 2013 20:23:01 GMT) Full text and rfc822 format available.

Notification sent to Juri Linkov <juri <at> jurta.org>:
bug acknowledged by developer. (Fri, 20 Dec 2013 20:23:02 GMT) Full text and rfc822 format available.

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

From: Juri Linkov <juri <at> jurta.org>
To: Karl Fogel <kfogel <at> red-bean.com>
Cc: 15329-done <at> debbugs.gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Fri, 20 Dec 2013 22:20:57 +0200
> 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.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#15329; Package emacs. (Sat, 21 Dec 2013 02:10:03 GMT) Full text and rfc822 format available.

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

From: Karl Fogel <kfogel <at> red-bean.com>
To: Juri Linkov <juri <at> jurta.org>
Cc: 15329-done <at> debbugs.gnu.org
Subject: Re: bug#15329: saveplace restores dired positions to random places
Date: Fri, 20 Dec 2013 20:09:06 -0600
Juri Linkov <juri <at> 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




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sat, 18 Jan 2014 12:24:03 GMT) Full text and rfc822 format available.

This bug report was last modified 10 years and 123 days ago.

Previous Next


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