Re: code review request for saveplace with dired buffer

[Karl Fogel]

Ivan Kanis <address@hidden> 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 <address@hidden>
>+      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  <address@hidden>
> 
>       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))
> 
> ;; 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