From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: [elpa] main 2ec80977e1: * elpa-packages (dired-preview): New package Date: Tue, 11 Jul 2023 07:47:18 +0000 Message-ID: <87v8eqc2k9.fsf@posteo.net> References: <168884609732.27984.6450580686777461843@vcs2.savannah.gnu.org> <20230708195457.95F1AC11DD8@vcs2.savannah.gnu.org> <87zg43iprw.fsf@posteo.net> <87edlfbjsx.fsf@protesilaos.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8842"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Stefan Monnier , emacs-devel@gnu.org To: Protesilaos Stavrou Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Jul 11 09:48:16 2023 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1qJ86Z-00026O-GL for ged-emacs-devel@m.gmane-mx.org; Tue, 11 Jul 2023 09:48:15 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qJ85o-0008Fi-4o; Tue, 11 Jul 2023 03:47:28 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qJ85l-0008FJ-Mr for emacs-devel@gnu.org; Tue, 11 Jul 2023 03:47:25 -0400 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qJ85j-0006o6-4n for emacs-devel@gnu.org; Tue, 11 Jul 2023 03:47:25 -0400 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 1A54A24002A for ; Tue, 11 Jul 2023 09:47:20 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1689061640; bh=OyRTf3c7jEpmIY26WOqq2o5+86aDnQAPZw0glowYyUY=; h=From:To:Cc:Subject:Autocrypt:Date:Message-ID:MIME-Version: Content-Transfer-Encoding:From; b=DO6PxVPbW6zvfEnE6DxbfnKT579q4KjSfBzWXyVsp55fSzGW60zjYcqUkh6YNNRP3 xghWP8U4ivp2qGbQKYPpzbubWDe7bnaDF0g2DOdjn6PKVfo8vFz+zBg7j2mTx98U83 z0RBZMSDxTpNNkZKHg1dojgegLdvnStzypJJDL1Pulet0UEDrObf2QSUEB9XbLXX11 +5DO70kglYCbCVG/zDMe1YYf0rR7vv++jwaFh1vNwtBc+AxFz5c4cFTRx0HyGSVVN0 Gpow7h0xX4lk3aDmn2oruX4T70KkWDsFKlzrTrjzEMMNWkDpdB/avxuDuo+ZjdFRbZ xEMBR9GnRKp3g== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4R0Xyb2x6Pz9rxH; Tue, 11 Jul 2023 09:47:19 +0200 (CEST) In-Reply-To: <87edlfbjsx.fsf@protesilaos.com> (Protesilaos Stavrou's message of "Mon, 10 Jul 2023 23:20:14 +0300") Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:307743 Archived-At: Protesilaos Stavrou writes: >> From: Philip Kaludercic >> Date: Mon, 10 Jul 2023 18:29:23 +0000 > > Hello Philip, > >> Stefan Monnier 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.=20 >> 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: >>=20=20 >> (require 'dired) >> +(require 'seq) >>=20=20 >> (defgroup dired-preview nil >> "Automatically preview file at point in Dired." >> :group 'dired) >>=20=20 >> (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 ;; doc: ... ;; args: (arg1) ;; 0 constant V0 ;; 1 stack-ref 1 ;; 2 call 1 ;; 3 return=20=20=20=20 ;; 3 stack-ref 3 ;; 4 call 2 ;; 5 stack-ref 3 ;; 6 call 2 ;; 7 return=09=20=20 (disassemble (lambda (y f) (dolist (x y) (funcall f x)))) ;; byte code: ;; doc: ... ;; args: (arg1 arg2) ;; 0 stack-ref 1 ;; 1:1 dup=09=20=20 ;; 2 goto-if-nil-else-pop 2 ;; 5 dup=09=20=20 ;; 6 car=09=20=20 ;; 7 stack-ref 2 ;; 8 stack-ref 1 ;; 9 call 1 ;; 10 discard=09=20=20 ;; 11 stack-ref 1 ;; 12 cdr=09=20=20 ;; 13 discardN-preserve-tos 2 ;; 15 goto 1 ;; 18:2 return=09=20=20 (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=09=20=20 --8<---------------cut here---------------end--------------->8--- So unless you can make use of =CE=B7-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-re= gexp'." >>=20=20 >> (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-re= gexp'." >> (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-d= ups', 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 :lo= cal)))) >>=20=20 >> @@ -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)) >>=20=20 >> (defun dired-preview--get-preview-buffer (file) >> "Return buffer to preview FILE in." >> (dired-preview--add-to-previews file)) >>=20=20 >> -(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 >>=20=20 >> (defun dired-preview-display-action-side () >> "Pick a side window that is appropriate for the given frame." >> - (if-let* ((width (window-body-width)) >> - ((>=3D width (window-body-height))) >> - ((>=3D width split-width-threshold))) >> + (if-let ((width (window-body-width)) >> + ((>=3D width (window-body-height))) >> + ((>=3D width split-width-threshold))) >> `(:side right :dimension window-width :size ,(dired-preview-get-w= indow-size :width)) >> `(:side bottom :dimension window-height :size ,(dired-preview-get-w= indow-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." >>=20=20 >> (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))) >>=20=20 >> ;;;###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")