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

[Philip Kaludercic <philipk@posteo.net>]

Protesilaos Stavrou <info@protesilaos.com> writes:

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

FWIW I am always glad to comment on packages before they are added, to
resolve low hanging issues before a package is published. 

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

I only bring this up, because I have found that frequently a lot of
features are more easily implemented in-core, where there is less of a
need for administrative boiler-plate code, and more flexibility when it
comes to changing other functions.

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

I don't have the changes anymore (beyond the diff), but I also don't
care about attribution.  Pick and choose what you want to use.

>> @@ -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.

In this case, it would be

--8<---------------cut here---------------start------------->8---
(rx "." (or "mkv" "webm" "mp4" "mp3" "ogg" "m4a"
	    "gz" "zst" "tar" "xz" "rar" "zip"
	    "iso" "epub" "pdf"))
--8<---------------cut here---------------end--------------->8---

which seems fine to me?  Fewer escape-sequences.

>>            "\\(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.

OK.

>> [... 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.

The reason I proposed the change was that returning a value (especially
via `when') was /not/ correctly expressing the intent in my eyes.  The
type of the first seq-filter argument is mentally something like "a ->
boolean", and `when' had a void or unit-type in my head.

But as Stephan said, this can be simplified even further to (seq-filter
#'buffer-live-p ...), which I think is even better.

>>  (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?

You can also compare the disassembled byte-code:

--8<---------------cut here---------------start------------->8---
(disassemble
 (lambda (y f) (mapc (lambda (x) (funcall f x)) y)))

;; byte code:
;;   doc:   ...
;;   args: (arg1 arg2)
;; 0	constant  mapc
;; 1	constant  make-closure
;; 2	constant  <compiled-function>
;;       doc:   ...
;;       args: (arg1)
;;     0	    constant  V0
;;     1	    stack-ref 1
;;     2	    call      1
;;     3	    return    

;; 3	stack-ref 3
;; 4	call	  2
;; 5	stack-ref 3
;; 6	call	  2
;; 7	return	  

(disassemble
 (lambda (y f) (dolist (x y) (funcall f x))))

;; byte code:
;;   doc:   ...
;;   args: (arg1 arg2)
;; 0	stack-ref 1
;; 1:1	dup	  
;; 2	goto-if-nil-else-pop 2
;; 5	dup	  
;; 6	car	  
;; 7	stack-ref 2
;; 8	stack-ref 1
;; 9	call	  1
;; 10	discard	  
;; 11	stack-ref 1
;; 12	cdr	  
;; 13	discardN-preserve-tos 2
;; 15	goto	  1
;; 18:2	return	  

(disassemble
 (lambda (y f) (mapc f y)))

;; byte code:
;;   doc:   ...
;;   args: (arg1 arg2)
;; 0	constant  mapc
;; 1	stack-ref 1
;; 2	stack-ref 3
;; 3	call	  2
;; 4	return	  
--8<---------------cut here---------------end--------------->8---

So unless you can make use of η-reduction, mapc incurs an overhead of
a funcall overhead for each element, but had an advantage in that
iteration happens in-core.

>> [... 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.

Because when-let* is not documented in (elisp) Conditionals, and the
official line is that using it should be avoided.  There is no analogy
between when-let and when-let* as there is between let and let*, which
is perhaps more confusing.

>>  (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.

I proposed this change, because otherwise it would seem like you would
be discarding the result of evaluating the if-expression.

>>        (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.

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.

That would sound like a bug?

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