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

[Philip Kaludercic]

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:

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