Re: code review request for saveplace with dired buffer

[Karl Fogel]

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

> 
> ;; 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