code review request for saveplace with dired buffer

code review request for saveplace with dired buffer

[Ivan Kanis]

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

I have attached the patch for saveplace with dired. Let me know if it's
good to be pushed.

I had to modify dired.el a bit. I thought I could move the cursor using
one of dired hook. Unfortunately it moves the cursor after it called its
hooks.
-- 
Recursivity. Call back if it happens again.
    -- BOFH excuse #36

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: saveplace-dired.patch --]
[-- Type: text/x-diff, Size: 4082 bytes --]

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index bec5a55..1cdec70 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,10 @@
+2013-06-99  Ivan Kanis <ivan@kanis.fr>
+	Add support for dired in saveplace.
+	* dired.el (dired-initial-position): Call cursor motion in
+	saveplace.
+	* saveplace.el (save-place-to-alist): Add dired position
+	(save-place-position-dired-cursor): New function
+
 2013-06-05  Leo Liu  <sdl.web@gmail.com>
 
 	Re-implement smie matching block highlight using
diff --git a/lisp/dired.el b/lisp/dired.el
index 5b6a787..84f2be7 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -2755,6 +2755,9 @@ as returned by `dired-get-filename'.  LIMIT is the search limit."
 
 (defvar dired-find-subdir)
 
+;; saveplace support
+(eval-when-compile (require 'saveplace))
+
 ;; FIXME document whatever dired-x is doing.
 (defun dired-initial-position (dirname)
   "Where point should go in a new listing of DIRNAME.
@@ -2762,7 +2765,8 @@ Point assumed at beginning of new subdir line."
   (end-of-line)
   (and (featurep 'dired-x) dired-find-subdir
        (dired-goto-subdir dirname))
-  (if dired-trivial-filenames (dired-goto-next-nontrivial-file)))
+  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
+  (if (featurep 'saveplace) (save-place-position-dired-cursor)))
 \f
 ;; These are hooks which make tree dired work.
 ;; They are in this file because other parts of dired need to call them.
diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 1b7efce..d4af2bc 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -169,22 +169,25 @@ file:
   ;; file.  If not, do so, then feel free to modify the alist.  It
   ;; will be saved again when Emacs is killed.
   (or save-place-loaded (load-save-place-alist-from-file))
-  (when (and buffer-file-name
-	     (or (not save-place-ignore-files-regexp)
-		 (not (string-match save-place-ignore-files-regexp
-				    buffer-file-name))))
-    (let ((cell (assoc buffer-file-name save-place-alist))
-	  (position (if (not (eq major-mode 'hexl-mode))
-			(point)
-		      (with-no-warnings
-			(1+ (hexl-current-address))))))
-      (if cell
-	  (setq save-place-alist (delq cell save-place-alist)))
-      (if (and save-place
-	       (not (= position 1)))  ;; Optimize out the degenerate case.
-	  (setq save-place-alist
-		(cons (cons buffer-file-name position)
-		      save-place-alist))))))
+  (let ((item (or buffer-file-name
+                  (when dired-directory (expand-file-name dired-directory))))
+        cell position)
+    (when (and item
+               (or (not save-place-ignore-files-regexp)
+                   (not (string-match save-place-ignore-files-regexp
+                                      item))))
+      (setq cell (assoc item save-place-alist)
+            position (if (not (eq major-mode 'hexl-mode))
+                         (point)
+                       (with-no-warnings
+                         (1+ (hexl-current-address)))))
+      (when cell
+        (setq save-place-alist (delq cell save-place-alist)))
+      (when (and save-place
+                 (not (= position 1)))  ;; Optimize out the degenerate case.
+        (setq save-place-alist
+              (cons (cons item position)
+                    save-place-alist))))))
 
 (defun save-place-forget-unreadable-files ()
   "Remove unreadable files from `save-place-alist'.
@@ -300,6 +303,17 @@ may have changed\) back to `save-place-alist'."
           ;; and make sure it will be saved again for later
           (setq save-place t)))))
 
+(defun save-place-position-dired-cursor ()
+  "Position the cursor 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)))
+    (if cell
+	(progn
+	  (or revert-buffer-in-progress-p
+	      (goto-char (cdr cell)))
+          ;; and make sure it will be saved again for later
+          (setq save-place t)))))
+
 (defun save-place-kill-emacs-hook ()
   ;; First update the alist.  This loads the old save-place-file if nec.
   (save-places-to-alist)

Re: code review request for saveplace with dired buffer

[Karl Fogel]

Ivan Kanis <ivan@kanis.fr> writes:
>I have attached the patch for saveplace with dired. Let me know if it's
>good to be pushed.
>
>I had to modify dired.el a bit. I thought I could move the cursor using
>one of dired hook. Unfortunately it moves the cursor after it called its
>hooks.

Some review below.  I'm not experienced in the dired code, so please
don't take my review of that portion as very authoritative.

>diff --git a/lisp/ChangeLog b/lisp/ChangeLog
>index bec5a55..1cdec70 100644
>--- a/lisp/ChangeLog
>+++ b/lisp/ChangeLog
>@@ -1,3 +1,10 @@
>+2013-06-99  Ivan Kanis <ivan@kanis.fr>
>+	Add support for dired in saveplace.
>+	* dired.el (dired-initial-position): Call cursor motion in
>+	saveplace.
>+	* saveplace.el (save-place-to-alist): Add dired position
>+	(save-place-position-dired-cursor): New function
>+
> 2013-06-05  Leo Liu  <sdl.web@gmail.com>
> 
> 	Re-implement smie matching block highlight using
>diff --git a/lisp/dired.el b/lisp/dired.el
>index 5b6a787..84f2be7 100644
>--- a/lisp/dired.el
>+++ b/lisp/dired.el
>@@ -2755,6 +2755,9 @@ as returned by `dired-get-filename'.  LIMIT is the search limit."
> 
> (defvar dired-find-subdir)
> 
>+;; saveplace support
>+(eval-when-compile (require 'saveplace))
>+
> ;; FIXME document whatever dired-x is doing.
> (defun dired-initial-position (dirname)
>   "Where point should go in a new listing of DIRNAME.
>@@ -2762,7 +2765,8 @@ Point assumed at beginning of new subdir line."
>   (end-of-line)
>   (and (featurep 'dired-x) dired-find-subdir
>        (dired-goto-subdir dirname))
>-  (if dired-trivial-filenames (dired-goto-next-nontrivial-file)))
>+  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
>+  (if (featurep 'saveplace) (save-place-position-dired-cursor)))

There might be a convention of using `when' instead of `if', when
there's no possibility of an `else' clause.  (I'm not sure about that,
though, so if someone else is I hope they'll speak up.)

> \f
> ;; These are hooks which make tree dired work.
> ;; They are in this file because other parts of dired need to call them.
>diff --git a/lisp/saveplace.el b/lisp/saveplace.el
>index 1b7efce..d4af2bc 100644
>--- a/lisp/saveplace.el
>+++ b/lisp/saveplace.el
>@@ -169,22 +169,25 @@ file:
>   ;; file.  If not, do so, then feel free to modify the alist.  It
>   ;; will be saved again when Emacs is killed.
>   (or save-place-loaded (load-save-place-alist-from-file))

(This is in `save-place-to-alist'.)  Do you want to update the doc
comment to indicate that it handles directories too now?

>-  (when (and buffer-file-name
>-	     (or (not save-place-ignore-files-regexp)
>-		 (not (string-match save-place-ignore-files-regexp
>-				    buffer-file-name))))
>-    (let ((cell (assoc buffer-file-name save-place-alist))
>-	  (position (if (not (eq major-mode 'hexl-mode))
>-			(point)
>-		      (with-no-warnings
>-			(1+ (hexl-current-address))))))
>-      (if cell
>-	  (setq save-place-alist (delq cell save-place-alist)))
>-      (if (and save-place
>-	       (not (= position 1)))  ;; Optimize out the degenerate case.
>-	  (setq save-place-alist
>-		(cons (cons buffer-file-name position)
>-		      save-place-alist))))))
>+  (let ((item (or buffer-file-name
>+                  (when dired-directory (expand-file-name dired-directory))))
>+        cell position)

Personally, if I'm reading too quickly, I see that as

  "bind `cell' to the value of `position'"

even though of course that's not what it means.  For readability, lIt
might be better to be explicit:

   (cell nil)
   (position nil)

>+    (when (and item
>+               (or (not save-place-ignore-files-regexp)
>+                   (not (string-match save-place-ignore-files-regexp
>+                                      item))))
>+      (setq cell (assoc item save-place-alist)
>+            position (if (not (eq major-mode 'hexl-mode))
>+                         (point)
>+                       (with-no-warnings
>+                         (1+ (hexl-current-address)))))
>+      (when cell
>+        (setq save-place-alist (delq cell save-place-alist)))

Heh, here you fixed the old code to use `when' instead of `if' :-).

>+      (when (and save-place
>+                 (not (= position 1)))  ;; Optimize out the degenerate case.
>+        (setq save-place-alist
>+              (cons (cons item position)
>+                    save-place-alist))))))
> 
> (defun save-place-forget-unreadable-files ()
>   "Remove unreadable files from `save-place-alist'.
>@@ -300,6 +303,17 @@ may have changed\) back to `save-place-alist'."
>           ;; and make sure it will be saved again for later
>           (setq save-place t)))))
> 
>+(defun save-place-position-dired-cursor ()
>+  "Position the cursor 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)))
>+    (if cell
>+	(progn
>+	  (or revert-buffer-in-progress-p
>+	      (goto-char (cdr cell)))
>+          ;; and make sure it will be saved again for later
>+          (setq save-place t)))))

One interesting result of this code is that if a file is replaced with a
director of the same name, or vice versa, the code will simply restore
the saved "place" by point position regardless of the fact that the
contents are completely unrelated.

I guess that's fine -- i.e., we don't need to store the type of the
record -- because after all the same is true if a file remains a file
but its contents completely change, too.

This looks pretty right to me.  I have only read it, not loaded and
tested it.  But if you've tested it and it works, +1 to push.

Thanks for doing this!

-Karl

Re: code review request for saveplace with dired buffer

[Glenn Morris]

Karl Fogel wrote:

>>+  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
>>+  (if (featurep 'saveplace) (save-place-position-dired-cursor)))
>
> There might be a convention of using `when' instead of `if', when
> there's no possibility of an `else' clause.  (I'm not sure about that,
> though, so if someone else is I hope they'll speak up.)

I'm not aware of a convention. Personally I prefer (if foo) rather than
(when foo) if `foo' is a single item, but since (when foo) ==
(if (progn foo)), and the compiler optimizes away the useless progn, it
makes no difference in practice.

RE: code review request for saveplace with dired buffer

[Drew Adams]

> > There might be a convention of using `when' instead of `if', when
> > there's no possibility of an `else' clause.  (I'm not sure about that,
> > though, so if someone else is I hope they'll speak up.)
> 
> I'm not aware of a convention. Personally I prefer (if foo) rather than
> (when foo) if `foo' is a single item, but since (when foo) ==
> (if (progn foo)), and the compiler optimizes away the useless progn, it
> makes no difference in practice.

A Common Lisp convention is to use `when' and `unless' when the return value is not important.  It communicates to a (human) reader that the body is for side effects only.

I, for one, use this same convention with Emacs Lisp.  I consider code that depends on the return value of `when' or `unless' to be bad style (misleading).  Some will disagree, no doubt.

Re: code review request for saveplace with dired buffer

[Karl Fogel]

Drew Adams <drew.adams@oracle.com> writes:
>A Common Lisp convention is to use `when' and `unless' when the return
>value is not important.  It communicates to a (human) reader that the
>body is for side effects only.
>
>I, for one, use this same convention with Emacs Lisp.  I consider code
>that depends on the return value of `when' or `unless' to be bad style
>(misleading).  Some will disagree, no doubt.

Thanks -- that's a nice guideline, and I'll try to remember it too.

RE: code review request for saveplace with dired buffer

[Drew Adams]

I mentioned:

> A Common Lisp convention is to use `when' and `unless' when the
> return value is not important.  It communicates to a (human) reader
> that the body is for side effects only.
> 
> I, for one, use this same convention with Emacs Lisp.  I consider
> code that depends on the return value of `when' or `unless' to be
> bad style (misleading).  Some will disagree, no doubt.

I was pretty sure that this style guidance was even called out in the
Common Lisp spec/manual, but I hadn't bothered to look it up again
(and it's been decades since I read it).

And so it is.  Here is what "Common Lisp The Language 2nd Ed." says:

    "As a matter of style, `when' is normally used to conditionally
   produce some side effects, and the value of the `when' form is
   normally not used. If the value is relevant, then it may be
   stylistically more appropriate to use `and' or `if'."

IMO, it's too bad that Emacs Dev does not appreciate & adopt such
a convention, as it helps users by informally communicating
programmer intent.

Yes, of course any such informal communication can mislead if
applied incorrectly, just as can an incorrect comment.  Still,
it makes sense.


To add to this, I'll mention that if the return value is important
then I also tend to use `and' instead of `if' with no else clause.
E.g.:

(use-value (and some-conditon  return-value))

vs
(use-value (if some-condition return-value))

I don't claim this second style guideline is a Common Lisp convention.

But as a consequence of these two rules, if you see an `if' in my
code then you can expect one or more else clauses.  If you find an
`if' with no else clause then it probably does not reflect what I
intended - what I thought I coded (likely a bug somewhere).

A third rule I use: I generally avoid using `if' with a literal `nil'
as result.  The condition can be negated to get rid of the `nil',
which would, by the second rule, be replaced by `and'.  E.g.:

(use-value (and condition  return-val))

vs
(use-value (if condition return-val))           ; 2nd rule
or
(use-value (if condition return-val nil)        ; 3rd rule
or
(use-value (if (not condition) nil return-val)) ; 3rd rule


An exception to the 3rd rule can be when `nil' is used as the empty
list.  But in that case I (try to remember to) write it as `()',
not `nil'.  E.g.: (use-list (if condition () other-list))

I don't always do that, though - sometimes even for a list I use
(and (not condition)  other-list).  Depends how much attention I
want to draw to the empty-list case, I guess.

I sometimes use another exception to the 3rd rule, to highlight
`nil' as a return value, especially if there are many side-effect
else clauses or some of them are complex.  E.g.:

(defun foo ()
  (if condition
      nil        ; Return nil
    (do-aaaa)
    (do-bbbb)
    (do-cccc)
    ...
    (do-zzzz)
    result))

That can sometimes be clearer than, say:

(defun foo ()
  (and condition
       (progn (do-aaaa)
              (do-bbbb)
              (do-cccc)
              ...
              (do-zzzz)
              result)))

(I tend to avoid `progn' too.)

Re: code review request for saveplace with dired buffer

[Stefan Monnier]

Thanks Ivan.  See some stylistic comments below, but otherwise, feel
free to install it.


        Stefan


> +(eval-when-compile (require 'saveplace))

I don't see any need for it.  Such a compile-time require is only needed
if you use macros from that package (or if your macros use functions
from that package).

>  ;; FIXME document whatever dired-x is doing.
>  (defun dired-initial-position (dirname)
>    "Where point should go in a new listing of DIRNAME.
> @@ -2762,7 +2765,8 @@ Point assumed at beginning of new subdir line."
>    (end-of-line)
>    (and (featurep 'dired-x) dired-find-subdir
>         (dired-goto-subdir dirname))
> -  (if dired-trivial-filenames (dired-goto-next-nontrivial-file)))
> +  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
> +  (if (featurep 'saveplace) (save-place-position-dired-cursor)))

Maybe a better alternative is to add a dired-initial-position-hook.
This way dired doesn't need any saveplace-specific code and instead
saveplace can simply add `save-place-position-dired-cursor' to
`dired-initial-position-hook'.

>    ;; file.  If not, do so, then feel free to modify the alist.  It
>    ;; will be saved again when Emacs is killed.
>    (or save-place-loaded (load-save-place-alist-from-file))
> -  (when (and buffer-file-name
> -	     (or (not save-place-ignore-files-regexp)
> -		 (not (string-match save-place-ignore-files-regexp
> -				    buffer-file-name))))
> -    (let ((cell (assoc buffer-file-name save-place-alist))
> -	  (position (if (not (eq major-mode 'hexl-mode))
> -			(point)
> -		      (with-no-warnings
> -			(1+ (hexl-current-address))))))
> -      (if cell
> -	  (setq save-place-alist (delq cell save-place-alist)))
> -      (if (and save-place
> -	       (not (= position 1)))  ;; Optimize out the degenerate case.
> -	  (setq save-place-alist
> -		(cons (cons buffer-file-name position)
> -		      save-place-alist))))))
> +  (let ((item (or buffer-file-name
> +                  (when dired-directory (expand-file-name dired-directory))))
> +        cell position)
> +    (when (and item
> +               (or (not save-place-ignore-files-regexp)
> +                   (not (string-match save-place-ignore-files-regexp
> +                                      item))))
> +      (setq cell (assoc item save-place-alist)
> +            position (if (not (eq major-mode 'hexl-mode))
> +                         (point)
> +                       (with-no-warnings
> +                         (1+ (hexl-current-address)))))

I think it's preferable to keep the cell and position binding in
a separate let.  Two "let"s are more efficient than such a "single let
plus subsequent setqs".  Also they're cleaner since they avoid `setq'.

> There might be a convention of using `when' instead of `if', when

What Glenn said.


        Stefan

Re: code review request for saveplace with dired buffer

[Ivan Kanis]

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

June, 07 at 15:12 Stefan wrote:

> Thanks Ivan.  See some stylistic comments below, but otherwise, feel
> free to install it.

Thanks for your feedback. I am attaching a second patch that takes in
account all of your comments. Once I have figured out bzr I will push
it.

Ivan


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: saveplace-dired2.patch --]
[-- Type: text/x-diff, Size: 4351 bytes --]

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index bec5a55..7529699 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,11 @@
+2013-06-99 Ivan Kanis <ivan@kanis.fr>
+	Add support for dired in saveplace.
+	* dired.el (dired-initial-position-hook): New variable.
+	(dired-initial-position): Call hook to place cursor position.
+	* saveplace.el (save-place-to-alist): Add dired position.
+	(save-place-position-dired-cursor): Move cursor in dired from
+	previous position.
+
 2013-06-05  Leo Liu  <sdl.web@gmail.com>
 
 	Re-implement smie matching block highlight using
diff --git a/lisp/dired.el b/lisp/dired.el
index 5b6a787..e5a7229 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -217,6 +217,11 @@ with the buffer narrowed to the listing."
 ;; Note this can't simply be run inside function `dired-ls' as the hook
 ;; functions probably depend on the dired-subdir-alist to be OK.
 
+(defcustom dired-initial-position-hook nil
+  "This hook is used to position the cursor position."
+  :group 'dired
+  :type 'hook)
+
 (defcustom dired-dnd-protocol-alist
   '(("^file:///" . dired-dnd-handle-local-file)
     ("^file://"  . dired-dnd-handle-file)
@@ -2762,7 +2767,8 @@ Point assumed at beginning of new subdir line."
   (end-of-line)
   (and (featurep 'dired-x) dired-find-subdir
        (dired-goto-subdir dirname))
-  (if dired-trivial-filenames (dired-goto-next-nontrivial-file)))
+  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
+  (run-hooks 'dired-initial-position-hook))
 \f
 ;; These are hooks which make tree dired work.
 ;; They are in this file because other parts of dired need to call them.
diff --git a/lisp/saveplace.el b/lisp/saveplace.el
index 1b7efce..48d7e09 100644
--- a/lisp/saveplace.el
+++ b/lisp/saveplace.el
@@ -169,22 +169,24 @@ file:
   ;; file.  If not, do so, then feel free to modify the alist.  It
   ;; will be saved again when Emacs is killed.
   (or save-place-loaded (load-save-place-alist-from-file))
-  (when (and buffer-file-name
-	     (or (not save-place-ignore-files-regexp)
-		 (not (string-match save-place-ignore-files-regexp
-				    buffer-file-name))))
-    (let ((cell (assoc buffer-file-name save-place-alist))
-	  (position (if (not (eq major-mode 'hexl-mode))
-			(point)
-		      (with-no-warnings
-			(1+ (hexl-current-address))))))
-      (if cell
-	  (setq save-place-alist (delq cell save-place-alist)))
-      (if (and save-place
-	       (not (= position 1)))  ;; Optimize out the degenerate case.
-	  (setq save-place-alist
-		(cons (cons buffer-file-name position)
-		      save-place-alist))))))
+  (let ((item (or buffer-file-name
+                  (and 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))))))
+        (if cell
+            (setq save-place-alist (delq cell save-place-alist)))
+        (if (and save-place
+                 (not (= position 1)))  ;; Optimize out the degenerate case.
+            (setq save-place-alist
+                  (cons (cons item position)
+                        save-place-alist)))))))
 
 (defun save-place-forget-unreadable-files ()
   "Remove unreadable files from `save-place-alist'.
@@ -308,8 +310,20 @@ may have changed\) back to `save-place-alist'."
   (if save-place-loaded
       (save-place-alist-to-file)))
 
+(defun save-place-position-dired-cursor ()
+  "Position the cursor 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)))
+    (if cell
+        (progn
+          (or revert-buffer-in-progress-p
+              (goto-char (cdr cell)))
+          ;; and make sure it will be saved again for later
+          (setq save-place t)))))
+
 (add-hook 'find-file-hook 'save-place-find-file-hook t)
 
+(add-hook 'dired-initial-position-hook 'save-place-position-dired-cursor)
 (unless noninteractive
   (add-hook 'kill-emacs-hook 'save-place-kill-emacs-hook))
 

Re: code review request for saveplace with dired buffer

[Karl Fogel]

Ivan Kanis <ivan@kanis.fr> writes:
>diff --git a/lisp/ChangeLog b/lisp/ChangeLog
>index bec5a55..7529699 100644
>--- a/lisp/ChangeLog
>+++ b/lisp/ChangeLog
>@@ -1,3 +1,11 @@
>+2013-06-99 Ivan Kanis <ivan@kanis.fr>
>+	Add support for dired in saveplace.
>+	* dired.el (dired-initial-position-hook): New variable.
>+	(dired-initial-position): Call hook to place cursor position.
>+	* saveplace.el (save-place-to-alist): Add dired position.
>+	(save-place-position-dired-cursor): Move cursor in dired from
>+	previous position.
>+
> 2013-06-05  Leo Liu  <sdl.web@gmail.com>
> 
> 	Re-implement smie matching block highlight using
>diff --git a/lisp/dired.el b/lisp/dired.el
>index 5b6a787..e5a7229 100644
>--- a/lisp/dired.el
>+++ b/lisp/dired.el
>@@ -217,6 +217,11 @@ with the buffer narrowed to the listing."
> ;; Note this can't simply be run inside function `dired-ls' as the hook
> ;; functions probably depend on the dired-subdir-alist to be OK.
> 
>+(defcustom dired-initial-position-hook nil
>+  "This hook is used to position the cursor position."
>+  :group 'dired
>+  :type 'hook)

Might be better to refer to "cursor position" as "point", which is the
Emacs Lisp convention.

> (defcustom dired-dnd-protocol-alist
>   '(("^file:///" . dired-dnd-handle-local-file)
>     ("^file://"  . dired-dnd-handle-file)
>@@ -2762,7 +2767,8 @@ Point assumed at beginning of new subdir line."
>   (end-of-line)
>   (and (featurep 'dired-x) dired-find-subdir
>        (dired-goto-subdir dirname))
>-  (if dired-trivial-filenames (dired-goto-next-nontrivial-file)))
>+  (if dired-trivial-filenames (dired-goto-next-nontrivial-file))
>+  (run-hooks 'dired-initial-position-hook))
> \f
> ;; These are hooks which make tree dired work.
> ;; They are in this file because other parts of dired need to call them.
>diff --git a/lisp/saveplace.el b/lisp/saveplace.el
>index 1b7efce..48d7e09 100644
>--- a/lisp/saveplace.el
>+++ b/lisp/saveplace.el
>@@ -169,22 +169,24 @@ file:
>   ;; file.  If not, do so, then feel free to modify the alist.  It
>   ;; will be saved again when Emacs is killed.
>   (or save-place-loaded (load-save-place-alist-from-file))
>-  (when (and buffer-file-name
>-	     (or (not save-place-ignore-files-regexp)
>-		 (not (string-match save-place-ignore-files-regexp
>-				    buffer-file-name))))
>-    (let ((cell (assoc buffer-file-name save-place-alist))
>-	  (position (if (not (eq major-mode 'hexl-mode))
>-			(point)
>-		      (with-no-warnings
>-			(1+ (hexl-current-address))))))
>-      (if cell
>-	  (setq save-place-alist (delq cell save-place-alist)))
>-      (if (and save-place
>-	       (not (= position 1)))  ;; Optimize out the degenerate case.
>-	  (setq save-place-alist
>-		(cons (cons buffer-file-name position)
>-		      save-place-alist))))))
>+  (let ((item (or buffer-file-name
>+                  (and 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))))))
>+        (if cell
>+            (setq save-place-alist (delq cell save-place-alist)))
>+        (if (and save-place
>+                 (not (= position 1)))  ;; Optimize out the degenerate case.
>+            (setq save-place-alist
>+                  (cons (cons item position)
>+                        save-place-alist)))))))
> 
> (defun save-place-forget-unreadable-files ()
>   "Remove unreadable files from `save-place-alist'.
>@@ -308,8 +310,20 @@ may have changed\) back to `save-place-alist'."
>   (if save-place-loaded
>       (save-place-alist-to-file)))
> 
>+(defun save-place-position-dired-cursor ()
>+  "Position the cursor in a dired buffer."

Likewise here, in both the doc string and the function name.  E.g.,
`save-place-dired-set-position'.

>+(add-hook 'dired-initial-position-hook 'save-place-position-dired-cursor)

...with the necessary adjustment here.

Best,
-Karl

Re: code review request for saveplace with dired buffer

[Ivan Kanis]

June, 09 at 11:20 Karl Fogel wrote:

>>+(defcustom dired-initial-position-hook nil
>>+  "This hook is used to position the cursor position."
>>+  :group 'dired
>>+  :type 'hook)
>
> Might be better to refer to "cursor position" as "point", which is the
> Emacs Lisp convention.

I have done that. I will push when I get bzr to work...

Re: code review request for saveplace with dired buffer

[Karl Fogel]

Ivan Kanis <ivan@kanis.fr> writes:
>June, 09 at 11:20 Karl Fogel wrote:
>
>>>+(defcustom dired-initial-position-hook nil
>>>+  "This hook is used to position the cursor position."
>>>+  :group 'dired
>>>+  :type 'hook)
>>
>> Might be better to refer to "cursor position" as "point", which is the
>> Emacs Lisp convention.
>
>I have done that. I will push when I get bzr to work...

Thanks.  Sorry about the bzr problems... :-(.

Re: code review request for saveplace with dired buffer

[Glenn Morris]

Ivan Kanis wrote:

> +2013-06-99 Ivan Kanis <ivan@kanis.fr>
> +	Add support for dired in saveplace.
> +	* dired.el (dired-initial-position-hook): New variable.

Add blank line before "Add support".

> + (save-place-position-dired-cursor): Move cursor in dired from previous position.

You can simply say "New function". "New function to move..." if you prefer.
As has been pointed out, "point" is better than "cursor", in function
names as well as doc.

> +(defcustom dired-initial-position-hook nil
> +  "This hook is used to position the cursor position."
> +  :group 'dired
> +  :type 'hook)

Add :version "24.4".

I'd find a comment in the doc "Run by the function `dired-initial-position'."
helpful. And likewise in the doc of `dired-initial-position': "Runs the
hook `dired-initial-position-hook'."