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