Re: [elpa] main 2ec80977e1: * elpa-packages (dired-preview): New package

[Protesilaos Stavrou]

> From: Philip Kaludercic <philipk@posteo.net>
> Date: Mon, 10 Jul 2023 18:29:23 +0000

Hello Philip,

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> --- a/elpa-packages
>>> +++ b/elpa-packages
>>> @@ -211,6 +211,10 @@
>>>    :ignored-files ("LICENSE"))
>>>   (dired-du         :url nil)
>>>   (dired-git-info   :url "https://github.com/clemera/dired-git-info";)
>>> + (dired-preview            :url 
>>> "https://git.sr.ht/~protesilaos/dired-preview";
>>> +  :doc "README.org"
>>> +  :readme "README.md"
>>> +  :ignored-files ("COPYING" "doclicense.texi"))
>
> Also, it would be nice if this list could be maintained in a .elpaignore
> file.

Yes, such a file make sense.

>>>   (disk-usage               :url 
>>> "https://gitlab.com/ambrevar/emacs-disk-usage";)
>>>   (dismal           :url nil)
>>>   (djvu                     :url nil)
>>
>> Isn't it odd to name your manual `README.org`?
>> IOW to use the same name as for the "read me" file.
>>
>> I mean you avoid a name collision only by accident because the two
>> happen to use different source formats and hence different extensions.
>>
>>         Stefan
>
> Was there an announcement message regarding the package?

No, sorry!  I used to do those in the form of a question, not
announcement.  The answer was always the same about going ahead with the
package, given that it conformed with the formalities.  Plus, the
maintainers are already too busy.

> I am a bit surprised to see it being added without any discussion on
> the list, perhaps something like this could have also been added to
> the core?

It is premature to add this in core.  There is more that can be done
with it and areas that can be greatly improved.  Having it as a GNU ELPA
package is easier at this early stage.  Let's give it a minimum of six
months.

> At the very least, it would be useful to gather some feedback
> regarding the code:

This is always welcome!

> diff --git a/dired-preview.el b/dired-preview.el
> index 853131b..b3517bd 100644

> [... 13 lines elided]

Can you please send these as a patch?  That way your contribution is
recorded by Git.

> @@ -57,24 +58,23 @@
>  ;;; Code:
>  
>  (require 'dired)
> +(require 'seq)
>  
>  (defgroup dired-preview nil
>    "Automatically preview file at point in Dired."
>    :group 'dired)
>  
>  (defcustom dired-preview-ignored-extensions-regexp
> -  (concat "\\."
> +  (concat "\\."                              ;perhaps use `rx'?

I saw 'rx' a couple of times.  I find it harder to use than the strings.
Though this is not a strong argument against it, I know.

>            "\\(mkv\\|webm\\|mp4\\|mp3\\|ogg\\|m4a"
>            "\\|gz\\|zst\\|tar\\|xz\\|rar\\|zip"
>            "\\|iso\\|epub\\|pdf\\)")
>    "Regular expression of file type extensions to not preview."
> -  :group 'dired-preview
>    :type 'string)

I know the group is implicit, but I prefer to specify it.  If we have a
second group, we can then more easily differentiate it.

> [... 21 lines elided]

>  (defvar dired-preview--buffers nil
>    "List with buffers of previewed files.")
> @@ -99,9 +97,8 @@ details."
>    "Return buffers that show previews."
>    (seq-filter
>     (lambda (buffer)
> -     (when (and (bufferp buffer)
> -                (buffer-live-p buffer))
> -       buffer))
> +     (and (bufferp buffer)
> +       (buffer-live-p buffer)))
>     dired-preview--buffers))

This is technically correct but more difficult to express intent.

>  (defun dired-preview--window-parameter-p (window)
> @@ -120,26 +117,22 @@ until it drops below this number.")
>  (defun dired-preview--get-buffer-cumulative-size ()
>    "Return cumulative buffer size of `dired-preview--get-buffers'."
>    (let ((size 0))
> -    (mapc
> -     (lambda (buffer)
> -       (setq size (+ (buffer-size buffer) size)))
> -     (dired-preview--get-buffers))
> +    (dolist (buffer (dired-preview--get-buffers))
> +      (setq size (+ (buffer-size buffer) size)))
>      size))

Since we are here, what is the technical advantage of dolist over mapc?
I have searched the docs for a clarification, but have not found one.  I
get that it looks tidier, but is there a performance boost or something?

> [... 42 lines elided]

> @@ -167,8 +158,8 @@ See user option 
> `dired-preview-ignored-extensions-regexp'."
>  
>  (defun dired-preview--file-displayed-p (file)
>    "Return non-nil if FILE is already displayed in a window."
> -  (when-let* ((buffer (get-file-buffer file))
> -              (window (get-buffer-window buffer)))
> +  (when-let ((buffer (get-file-buffer file))
> +          (window (get-buffer-window buffer)))
>      (window-live-p window)))

Why remove the asterisk?  I understand that it works, but this makes it
more difficult to reason about the intent.

>  (defun dired-preview--set-window-parameters (window value)
> @@ -183,9 +174,9 @@ See user option 
> `dired-preview-ignored-extensions-regexp'."
>    (cond
>     ((window-parameter (selected-window) 'dired-preview-window)
>      (dired-preview--delete-windows))
> -   ((and delay-mode-hooks (current-buffer))
> +   ((and delay-mode-hooks (current-buffer)) ;can `current-buffer' be nil

This is probably the remnant of an earlier debugging session.  If I
recall the scenario, the file could be visited in another buffer, which
would trigger the delayed-mode-hooks.  It does not look good, I know.

>      (dired-preview--set-window-parameters (selected-window) nil)
> -    (apply #'run-hooks (delete-dups delayed-mode-hooks))
> +    (apply #'run-hooks (delete-dups delayed-mode-hooks)) ;why `delete-dups', 
> which is destructive

Again, the product of an earlier session.  I had cases where a function
was added twice and it would slow things down.  I don't know how it was
duplicated in the first place though.

>      (kill-local-variable 'delayed-mode-hooks)
>      (remove-hook 'post-command-hook #'dired-preview--run-mode-hooks 
> :local))))
>  
> @@ -206,19 +197,19 @@ See user option 
> `dired-preview-ignored-extensions-regexp'."
>    "Add FILE to `dired-preview--buffers', if not already in a buffer.
>  Always return FILE buffer."
>    (let ((buffer (find-buffer-visiting file)))
> -    (if (buffer-live-p buffer)
> -        buffer
> +    (unless (buffer-live-p buffer)

Technically correct, but more difficult to reason about.  You can
already see this more verbose pattern of mine.

>        (setq buffer (dired-preview--find-file-no-select file)))
>      (with-current-buffer buffer
>        (add-hook 'post-command-hook #'dired-preview--run-mode-hooks nil 
> :local))
> -    (add-to-list 'dired-preview--buffers buffer)
> +    (unless (memq buffer dired-preview--buffers) ;or `cl-pushnew'
> +      (push buffer dired-preview--buffers))

The change or cl-pushnew is fine.

>      buffer))
>  
>  (defun dired-preview--get-preview-buffer (file)
>    "Return buffer to preview FILE in."
>    (dired-preview--add-to-previews file))
>  
> -(defvar dired-preview-buffer-name "*dired-preview*"
> +(defconst dired-preview-buffer-name "*dired-preview*" ;unused?
>    "Name of preview buffer.")

Yes, this should be deleted.  It was used in earlier versions when the
mode line would have a custom format.

>  (defun dired-preview-get-window-size (dimension)
> @@ -235,9 +226,9 @@ checked against `split-width-threshold' or
>  
>  (defun dired-preview-display-action-side ()
>    "Pick a side window that is appropriate for the given frame."
> -  (if-let* ((width (window-body-width))
> -            ((>= width (window-body-height)))
> -            ((>= width split-width-threshold)))
> +  (if-let ((width (window-body-width))
> +        ((>= width (window-body-height)))
> +        ((>= width split-width-threshold)))
>        `(:side right :dimension window-width :size 
> ,(dired-preview-get-window-size :width))
>      `(:side bottom :dimension window-height :size 
> ,(dired-preview-get-window-size :height))))

Same point as above about the asterisk.

> [... 18 lines elided]

> @@ -341,7 +332,7 @@ the preview with `dired-preview-delay' of idleness."
>  
>  (defun dired-preview--on ()
>    "Enable `dired-preview-mode' in Dired."
> -  (when (eq major-mode 'dired-mode)
> +  (when (derived-mode-p 'dired-mode)
>      (dired-preview-mode 1)))
>  
>  ;;;###autoload

I generally use this, though I wanted to be explicit in this case to
avoid potential conflicts elsewhere.  Where?  I don't know.  Just being
cautious.

All the best,
Protesilaos (or simply "Prot")

-- 
Protesilaos Stavrou
https://protesilaos.com