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

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

[Stefan Monnier]

> --- 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"))
>   (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

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

[Protesilaos Stavrou]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Sat,  8 Jul 2023 17:47:44 -0400

Hello Stefan,

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

I had thought about calling it <package-name>.org, like I do with the
modus-themes, but that does not immediately call attention the way
"README" does.

The README.md is there (i) for Git forges that do not parse Org files
properly and (ii) to provide basic information about the package.

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

-- 
Protesilaos Stavrou
https://protesilaos.com

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

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

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.

>>   (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?  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?  At
the very least, it would be useful to gather some feedback regarding the
code:


[-- Attachment #2: Type: text/plain, Size: 8125 bytes --]

diff --git a/dired-preview.el b/dired-preview.el
index 853131b..b3517bd 100644
--- a/dired-preview.el
+++ b/dired-preview.el
@@ -3,8 +3,9 @@
 ;; Copyright (C) 2023  Free Software Foundation, Inc.
 
 ;; Author: Protesilaos Stavrou <info@protesilaos.com>
+;; Maintainer: Protesilaos Stavrou <~protesilaos/general-issues@lists.sr.ht>
 ;; URL: https://git.sr.ht/~protesilaos/dired-preview
-;; Mailing-List: https://lists.sr.ht/~protesilaos/general-issues
+;; Mailing-List: https://lists.sr.ht/~protesilaos/general-issues 
 ;; Version: 0.1.1
 ;; Package-Requires: ((emacs "27.1"))
 ;; Keywords: files, convenience
@@ -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'?
           "\\(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)
 
 (defcustom dired-preview-max-size (expt 2 20)
   "Files larger than this byte limit are not previewed."
-  :group 'dired-preview
-  :type 'integer)
+  :type 'natnum)
 
 (defcustom dired-preview-display-action-alist-function
   #'dired-preview-display-action-alist-dwim
@@ -84,13 +84,11 @@ Read Info node `(elisp) Displaying Buffers'.  As such, it is
 meant for experienced users.  See the reference function
 `dired-preview-display-action-alist-dwim' for the implementation
 details."
-  :group 'dired-preview
   :type 'function)
 
 (defcustom dired-preview-delay 0.7
   "Time in seconds to wait before previewing."
-  :group 'dired-preview
-  :type 'float)
+  :type 'number)
 
 (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))
 
 (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))
 
 (defun dired-preview--kill-buffers ()
   "Kill preview buffers up to `dired-preview--buffers-threshold'."
   (let ((buffers (nreverse (dired-preview--get-buffers))))
     (catch 'stop
-      (mapc
-       (lambda (buffer)
-         (if (and (>= (dired-preview--get-buffer-cumulative-size)
-                      dired-preview--buffers-threshold))
-             (when (and (buffer-local-value 'delayed-mode-hooks buffer)
-                        (not (eq buffer (current-buffer))))
-               (ignore-errors (kill-buffer-if-not-modified buffer))
-               (setq buffers (delq buffer buffers)))
-           (throw 'stop t)))
-       buffers))
+      (dolist (buffer buffers)
+	(if (and (>= (dired-preview--get-buffer-cumulative-size)
+		     dired-preview--buffers-threshold))
+	    (when (and (buffer-local-value 'delayed-mode-hooks buffer)
+		       (not (eq buffer (current-buffer))))
+	      (ignore-errors (kill-buffer-if-not-modified buffer))
+	      (setq buffers (delq buffer buffers)))
+	  (throw 'stop t))))
     (setq dired-preview--buffers (delq nil (nreverse buffers)))))
 
 (defun dired-preview--get-windows ()
@@ -148,12 +141,10 @@ until it drops below this number.")
 
 (defun dired-preview--delete-windows ()
   "Delete preview windows."
-  (mapc
-   (lambda (window)
-     (unless (or (one-window-p)
-                 (eq window (minibuffer-window)))
-       (delete-window window)))
-   (dired-preview--get-windows)))
+  (dolist (window (dired-preview--get-windows))
+    (unless (or (one-window-p)
+		(eq window (minibuffer-window)))
+      (delete-window window))))
 
 (defun dired-preview--file-ignored-p (file)
   "Return non-nil if FILE extension is among the ignored extensions.
@@ -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)))
 
 (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
     (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
     (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)
       (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))
     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.")
 
 (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))))
 
@@ -320,14 +311,14 @@ the preview with `dired-preview-delay' of idleness."
 (defun dired-preview-disable-preview ()
   "Disable Dired preview."
   (unless (eq major-mode 'dired-mode)
-    (error "Can only use `dired-preview' in Dired"))
+    (user-error "Can only use `dired-preview' in Dired"))
   (remove-hook 'post-command-hook #'dired-preview-trigger :local)
   (dired-preview--close-previews))
 
 (defun dired-preview-enable-preview ()
   "Enable Dired preview."
   (unless (eq major-mode 'dired-mode)
-    (error "Can only use `dired-preview' in Dired"))
+    (user-error "Can only use `dired-preview' in Dired"))
   (add-hook 'post-command-hook #'dired-preview-trigger nil :local)
   (dired-preview-trigger :no-delay))
 
@@ -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

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

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

[Stefan Monnier]

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

BTW, `buffer-live-p` also accepts non-buffer arguments, so you can skip
the `bufferp` test.

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

It's like an inlined mapc, which makes it a bit more efficient, yes.


        Stefan

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

[Philip Kaludercic]

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")

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

[Stefan Monnier]

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

BTW, you can most of the benefit here by using `regexp-opt` instead:

    (concat "\\."
            (regexp-opt "mkv" "webm" "mp4" "mp3" "ogg" "m4a"
                        "gz" "zst" "tar" "xz" "rar" "zip"
                        "iso" "epub" "pdf"))

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

For the specific case at hand, there's also the cost of allocating the
closure and converting the free variable into a cons-cell (because it's
modified) which slows down accesses to it.  IOW

    (let ((size 0))
      (mapc
       (lambda (buffer)
         (setq size (+ (buffer-size buffer) size)))
       (dired-preview--get-buffers))
      size))

will look somewhat like:

    (let ((size (list 0)))
      (mapc
       (make-closure
         (lambda (buffer)
           (setcar V0 (+ (buffer-size buffer) (car V0))))
         size)
       (dired-preview--get-buffers))
      (car size))

See bytecode below.


        Stefan


byte code:
  args: nil
0       constant  0
1       list1     
2       constant  mapc
3       constant  make-closure
4       constant  <compiled-function>
      doc:   ...
      args: (arg1)
    0       constant  V0
    1       constant  buffer-size
    2       stack-ref 2
    3       call      1
    4       constant  V0
    5       car-safe  
    6       plus      
    7       setcar    
    8       return    

5       stack-ref 3
6       call      2
7       constant  dired-preview--get-buffers
8       call      0
9       call      2
10      discard   
11      car-safe  
12      return    

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

[Philip Kaludercic]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> 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)
>
> BTW, you can most of the benefit here by using `regexp-opt` instead:
>
>     (concat "\\."
>             (regexp-opt "mkv" "webm" "mp4" "mp3" "ogg" "m4a"
>                         "gz" "zst" "tar" "xz" "rar" "zip"
>                         "iso" "epub" "pdf"))

Of course, modulo

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (wrong-number-of-arguments (1 . 2) 15)
  regexp-opt("mkv" "webm" "mp4" "mp3" "ogg" "m4a" "gz" "zst" "tar" "xz" "rar" "zip" "iso" "epub" "pdf")
  (concat "\\." (regexp-opt "mkv" "webm" "mp4" "mp3" "ogg" "m4a" "gz" "zst" "tar" "xz" "rar" "zip" "iso" "epub" "pdf"))
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  command-execute(eval-last-sexp)
--8<---------------cut here---------------end--------------->8---

Obviously, one would just have to wrap the arguments in the list but
that together with the separate concat makes it less comfortable than
`rx' to me.  But worth mentioning!

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

[Stefan Monnier]

>>     (concat "\\."
>>             (regexp-opt "mkv" "webm" "mp4" "mp3" "ogg" "m4a"
>>                         "gz" "zst" "tar" "xz" "rar" "zip"
>>                         "iso" "epub" "pdf"))
>
> Of course, modulo
>
> --8<---------------cut here---------------start------------->8---
> Debugger entered--Lisp error: (wrong-number-of-arguments (1 . 2) 15)

:-)

> Obviously, one would just have to wrap the arguments in the list but
> that together with the separate concat makes it less comfortable than
> `rx' to me.  But worth mentioning!

I just mentioned it for those cases where `rx` is shunned for a reason
or another.


        Stefan

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

> [... 35 lines elided]

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

> [... 206 lines elided]

Hello again Philip,

I believe I pushed all the relevant changes you suggested.  The only one
I have not done yet is the 'dolist' VS 'mapc' that you and Stefan
commented on.  I need to understand the technical differences better, so
as to know when to use 'mapc'.

All the best,
Prot

-- 
Protesilaos Stavrou
https://protesilaos.com

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

[Philip Kaludercic]

Protesilaos Stavrou <info@protesilaos.com> writes:

>> From: Philip Kaludercic <philipk@posteo.net>
>> Date: Mon, 10 Jul 2023 18:29:23 +0000
>
>> [... 35 lines elided]
>
>> diff --git a/dired-preview.el b/dired-preview.el
>> index 853131b..b3517bd 100644
>> --- a/dired-preview.el
>> +++ b/dired-preview.el
>
>> [... 206 lines elided]
>
> Hello again Philip,
>
> I believe I pushed all the relevant changes you suggested.  The only one
> I have not done yet is the 'dolist' VS 'mapc' that you and Stefan
> commented on.  I need to understand the technical differences better, so
> as to know when to use 'mapc'.

Do you have any concrete questions?  Semantically they should be
equivalent, it is just a matter of estimating the cost of a funcall and
of closures, which is why I suggest replacing (mapc (lambda (x) ... x
...) xs) with (dolist (x xs) ... x ...).

> All the best,
> Prot

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

[Stefan Monnier]

>> I believe I pushed all the relevant changes you suggested.  The only one
>> I have not done yet is the 'dolist' VS 'mapc' that you and Stefan
>> commented on.  I need to understand the technical differences better, so
>> as to know when to use 'mapc'.
>
> Do you have any concrete questions?  Semantically they should be
> equivalent, it is just a matter of estimating the cost of a funcall and
> of closures, which is why I suggest replacing (mapc (lambda (x) ... x
> ...) xs) with (dolist (x xs) ... x ...).

Actually, there are a few semantic differences:

- `mapc` also works on other sequences such as vectors and strings.
- `called-interactively-p` will always return nil when called from
  within the `mapc` (since it will say whether the function that makes
  up the loop body was called interactively rather than whether the
  surrounding function has been called interactively).
- similarly, other tools that look under the hood (like
  `backtrace-frames`) will give slightly different results.
- there might be further corner case differences e.g. if you modify the
  list while you iterate over it.


-- Stefan

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

[Mattias Engdegård]

13 juli 2023 kl. 23.36 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> - `mapc` also works on other sequences such as vectors and strings.

Right. This also makes it less type-safe than `dolist` in practice: if fed a string by accident, `mapc` will happy iterate over a sequence of numbers instead of signalling a wrong-type-argument error.

`mapc` is also less safe, in a different sense, if recursed through since it could overflow the C stack if the `max-eval-lisp-depth` safety catch is off.

Using `dolist` typically results in slightly shorter code to write, which more importantly means less code to read. Most people also prefer seeing the list argument on top instead of buried after the loop body.

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

[Drew Adams]

While on the subject of `dolist' versus `mapc',
I'd add that code with `dolist' looks more like
usual imperative programming constructs (e.g.,
it looks like C or Fortran), and `mapc' looks,
perhaps misleadingly, more like functional
programming mapping over streams.

IOW, IMO there's no great use case for `mapc'
on a list, but I suppose maybe someone might
find it clearer than `dolist' when used in a
context where there's already some other
mapping involved.

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

[Philip Kaludercic]

Drew Adams <drew.adams@oracle.com> writes:

> While on the subject of `dolist' versus `mapc',
> I'd add that code with `dolist' looks more like
> usual imperative programming constructs (e.g.,
> it looks like C or Fortran), and `mapc' looks,
> perhaps misleadingly, more like functional
> programming mapping over streams.
>
> IOW, IMO there's no great use case for `mapc'
> on a list, but I suppose maybe someone might
> find it clearer than `dolist' when used in a
> context where there's already some other
> mapping involved.

I rgrep'ed my own code and found this block I still find convenient,
because I don't have to come up with a name (even though `var'  would
probably be a sufficiently good fit here, I guess):

--8<---------------cut here---------------start------------->8---
(mapc #'kill-local-variable
      '(enable-local-variables
        header-line-format
        buffer-read-only))
--8<---------------cut here---------------end--------------->8---

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

[Stefan Monnier]

> (mapc #'kill-local-variable
>       '(enable-local-variables
>         header-line-format
>         buffer-read-only))

Indeed, `mapc` is more efficient that `dolist` if you pass it just
a function name rather than a lambda expression.


        Stefan