[ELPA] New package: listen

[ELPA] New package: listen

[Adam Porter]

Hi,

I'd like to submit a new package to ELPA, named "Listen".  It's a simple 
audio/music player that uses VLC as a backend.

https://github.com/alphapapa/listen.el

Compared to other Emacs music players like EMMS, Bongo, etc, it's much 
simpler to use: no setup or configuration is required--just "M-x listen 
RET" and use the Transient menu to begin.

Compared to packages that use MPD as a backend, Listen provides a 
simpler UI, and it doesn't require installing and configuring MPD; the 
user just selects one or more files to play.  (However, support is 
provided for discovering and playing tracks from a local MPD library by 
searching its metadata database.)

It's extensible, so e.g. a new backend could easily be added to use 
something like MPV to play files.

A queue (playlist) view is provided using the nice new Vtable library in 
Emacs 29.  And a library view is provided using the Taxy and 
Magit-Section libraries to browse tracks grouped by metadata.

For more details (though there's not much else to say), please see the 
readme/Info manual.

By the way, unlike with some other packages I've submitted, I've not 
fully settled on the name, so I'm open to suggestions.  I considered 
naming it "EMP" (for "Emacs Music Player", which is fun and concise, and 
which is surprisingly not yet taken--although Nic Vollmer has an 
unpackaged library by that name, but he's graciously offered me the name 
for use with this package).  But "Listen" seemed more descriptive and 
concise enough, so that's what I've gone with for now.

Thanks,
Adam

Re: [ELPA] New package: listen

[Philip Kaludercic]

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

Adam Porter <adam@alphapapa.net> writes:

> Hi,
>
> I'd like to submit a new package to ELPA, named "Listen".  It's a
> simple audio/music player that uses VLC as a backend.
>
> https://github.com/alphapapa/listen.el

Note that the copyright lines still have to be updated for it to be
added to GNU ELPA.

Here are a few comments, questions and suggestions from briefly skimming
the code:


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

diff --git a/listen-info.el b/listen-info.el
index e74d376df9..a95a5b1d1b 100644
--- a/listen-info.el
+++ b/listen-info.el
@@ -1,6 +1,6 @@
 ;;; listen-info.el --- Audio file metadata library  -*- lexical-binding: t; -*-
 
-;; Copyright (C) 2020-2021 Free Software Foundation, Inc.
+;; Copyright (C) 2020-2021, 2024 Free Software Foundation, Inc.
 
 ;; Author: Petteri Hintsanen <petterih@iki.fi>
 
@@ -56,7 +56,6 @@
 
 (require 'bindat)
 (require 'cl-lib)
-(require 'seq)
 (require 'subr-x)
 
 (defconst listen-info--max-peek-size (* 2048 1024)
@@ -942,10 +941,10 @@ extension.
 
 Return one of symbols `vorbis', `opus', `flac', or `mp3'."
   (let ((case-fold-search t))
-    (cond ((string-match ".ogg$" filename) 'vorbis)
-          ((string-match ".opus$" filename) 'opus)
-          ((string-match ".flac$" filename) 'flac)
-          ((string-match ".mp3$" filename) 'mp3)
+    (cond ((string-match ".ogg\\'" filename) 'vorbis)
+          ((string-match ".opus\\'" filename) 'opus)
+          ((string-match ".flac\\'" filename) 'flac)
+          ((string-match ".mp3\\'" filename) 'mp3)
           (t nil))))
 
 (provide 'listen-info)
diff --git a/listen-lib.el b/listen-lib.el
index 7743080c23..b3d48f51fa 100644
--- a/listen-lib.el
+++ b/listen-lib.el
@@ -50,7 +50,7 @@
 ;;;; Faces
 
 (defgroup listen-faces nil
-  "Faces."
+  "Faces used by listen.el."
   :group 'listen)
 
 (defface listen-artist '((t :inherit font-lock-variable-name-face))
diff --git a/listen-library.el b/listen-library.el
index 77022e1791..90d40468ad 100644
--- a/listen-library.el
+++ b/listen-library.el
@@ -100,8 +100,8 @@ mode line and bookmark name.  BUFFER may be specified in which to
 show the view."
   (interactive
    (list (list (read-file-name "View library for: "))
-         :name (when current-prefix-arg
-                 (read-string "Library name: "))))
+         :name (and current-prefix-arg
+                    (read-string "Library name: "))))
   (let* ((filenames (cl-loop for path in paths
                              if (file-directory-p path)
                              append (directory-files-recursively path "." t)
@@ -181,7 +181,7 @@ Interactively, read COMMAND and use tracks at point in
     ;; it ought to return sections in the region), it returns nil.
     ;; This may be confusing to users, but it seems like an upstream
     ;; issue.
-    (or (flatten-list (mapcar #'value-of (magit-region-sections)))
+    (or (flatten-list (mapcar #'value-of (magit-region-sections))) ;or `mapcan'?
         (value-of (magit-current-section)))))
 
 ;;;;; Bookmark support
@@ -192,7 +192,7 @@ Interactively, read COMMAND and use tracks at point in
   "Return a bookmark record for the current library buffer."
   (cl-assert listen-library-paths)
   `(,(format "Listen library: %s" (or listen-library-name listen-library-paths))
-    (handler . listen-library--bookmark-handler)
+    (handler . ,#'listen-library--bookmark-handler)
     (name . ,listen-library-name)
     (paths . ,listen-library-paths)))
 
diff --git a/listen-mpd.el b/listen-mpd.el
index b8a104febe..8c3a302f08 100644
--- a/listen-mpd.el
+++ b/listen-mpd.el
@@ -89,17 +89,17 @@ completion."
                                       (cons 'affixation-function #'affix)
                                       ;; (cons 'annotation-function #'annotate)
                                       ))))
-                  (`t (unless (string-empty-p str)
-                        (let ((tag (pcase tag
-                                     ('any 'file)
-                                     (_ tag))))
-                          (delete-dups
-                           (delq nil
-                                 (mapcar (lambda (row)
-                                           (when-let ((value (alist-get tag row)))
-                                             (propertize value
-                                                         :mpc-alist row)))
-                                         (search-any (split-string str))))))))))
+                  ((guard (not (string-empty-p str)))
+                   (let ((tag (pcase tag
+                                ('any 'file)
+                                (_ tag))))
+                     (delete-dups
+                      (delq nil
+                            (mapcar (lambda (row)
+                                      (when-let ((value (alist-get tag row)))
+                                        (propertize value
+                                                    :mpc-alist row)))
+                                    (search-any (split-string str)))))))))
               (try (string _table _pred point &optional _metadata)
                 (cons string point))
               (all (string table pred _point)
diff --git a/listen-queue.el b/listen-queue.el
index 9515e3d7dc..cf4bd494bd 100644
--- a/listen-queue.el
+++ b/listen-queue.el
@@ -312,7 +312,7 @@ PROMPT is passed to `format-prompt', which see."
      ;; homedir path (so queues could be portable with music
      ;; libraries).
      :filename (abbreviate-file-name filename)
-     :artist (map-elt metadata "artist")
+     :artist (map-elt metadata "artist") ;`metadata' is an alist, right?  So why not use `let-alist'?
      :title (map-elt metadata "title")
      :album (map-elt metadata "album")
      :number (map-elt metadata "tracknumber")
@@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see."
          n)
     (when current-track
       (cl-callf2 delete current-track tracks))
-    (setf n (length tracks))
+    (setf n (length tracks))   ;why the variable?
     ;; Don't use dotimes result (bug#16206)
     (dotimes (i n)
       (cl-rotatef (elt tracks i) (elt tracks (+ i (cl-random (- n i))))))
@@ -357,6 +357,8 @@ PROMPT is passed to `format-prompt', which see."
                (1+ (seq-position (listen-queue-tracks queue)
                                  (listen-queue-current queue)
                                  (lambda (a b)
+                                   ;; FWIW you still have a similar
+                                   ;; race condition here.
                                    (equal (listen-track-filename a)
                                           (listen-track-filename b))))))))
 
@@ -427,7 +429,7 @@ tracks in the queue unchanged)."
         (goto-char beg)
         (cl-loop collect (vtable-current-object)
                  do (forward-line 1)
-                 while (<= (point) end))))))
+                 until (eobp))))))
 
 ;;;;; Bookmark support
 
diff --git a/listen-vlc.el b/listen-vlc.el
index 12e7e58693..a387c1c3d5 100755
--- a/listen-vlc.el
+++ b/listen-vlc.el
@@ -43,7 +43,8 @@
     (save-excursion
       (insert (listen--send player "info")))
     (cl-loop while (re-search-forward (rx bol "| " (group (1+ (not blank))) ": "
-                                          (group (1+ (not (any ""))))) nil t)
+                                          (group (1+ (not (any "\C-m")))))
+                                      nil t)
              collect (cons (match-string 1) (match-string 2)))))
 
 (cl-defmethod listen--filename ((player listen-player-vlc))
@@ -116,7 +117,7 @@ Stops playing, clears playlist, adds FILE, and plays it."
   "Return or set PLAYER's VOLUME.
 VOLUME is an integer percentage."
   (if volume
-      (listen--send player (format "volume %s" (* 255 (/ volume 100.0))))
+      (listen--send player (format "volume %s" (* 255 (/ volume 100.0)))) ;what if this is OOB?
     (* 100 (/ (string-to-number (listen--send player "volume")) 255.0))))
 
 (provide 'listen-vlc)
diff --git a/listen.el b/listen.el
index 3e3d11d572..c381b2a113 100755
--- a/listen.el
+++ b/listen.el
@@ -75,7 +75,7 @@
   :link '(emacs-commentary-link "listen")
   :link '(emacs-library-link "listen"))
 
-(defcustom listen-directory "~/Music"
+(defcustom listen-directory "~/Music"   ;perhaps check XDG_MUSIC_DIR here?
   "Default music directory."
   :type 'directory)
 
@@ -101,7 +101,7 @@ Interactively, uses the default player."
   (when (eq player listen-player)
     (setf listen-player nil)))
 
-(declare-function listen-queue-next "listen-queue")
+(declare-function listen-queue-next "listen-queue") ;why not `require' it?
 (defun listen-next (player)
   "Play next track in PLAYER's queue.
 Interactively, uses the default player."
@@ -251,19 +251,19 @@ command with completion."
 (defun listen-read-time (time)
   "Return TIME in seconds.
 TIME is an HH:MM:SS string."
-  (string-match (rx (group (1+ num))
-                    (optional ":" (group (1+ num))
-                              (optional ":" (group (1+ num)))))
-                time)
-  (let ((fields (nreverse
-                 (remq nil
-                       (list (match-string 1 time)
-                             (match-string 2 time)
-                             (match-string 3 time)))))
-        (factors [1 60 3600]))
-    (cl-loop for field in fields
-             for factor across factors
-             sum (* (string-to-number field) factor))))
+  (when (string-match (rx (group (1+ num))
+                          (optional ":" (group (1+ num))
+                                    (optional ":" (group (1+ num)))))
+                      time)
+    (let ((fields (nreverse
+                   (remq nil
+                         (list (match-string 1 time)
+                               (match-string 2 time)
+                               (match-string 3 time)))))
+          (factors [1 60 3600]))
+      (cl-loop for field in fields
+               for factor across factors
+               sum (* (string-to-number field) factor)))))
 
 ;;;; Transient
 

[-- Attachment #3: Type: text/plain, Size: 1956 bytes --]


> Compared to other Emacs music players like EMMS, Bongo, etc, it's much
> simpler to use: no setup or configuration is required--just "M-x
> listen RET" and use the Transient menu to begin.
>
> Compared to packages that use MPD as a backend, Listen provides a
> simpler UI, and it doesn't require installing and configuring MPD; the
> user just selects one or more files to play.  (However, support is
> provided for discovering and playing tracks from a local MPD library
> by searching its metadata database.)
>
> It's extensible, so e.g. a new backend could easily be added to use
> something like MPV to play files.

That would be nice.

> A queue (playlist) view is provided using the nice new Vtable library
> in Emacs 29.  And a library view is provided using the Taxy and
> Magit-Section libraries to browse tracks grouped by metadata.
>
> For more details (though there's not much else to say), please see the
> readme/Info manual.

On the topic of the readme/manual, wouldn't it be better to have a
separate README file?  Then again, the manual is pretty short, and I
don't know if it is worth having it in the first place... 

Also, your README includes this line

  :vc (:fetcher github :repo "alphapapa/listen.el")

which is malformed.  What you want is 

  :vc (:url "https://github.com/alphapapa/listen.el")

or after the package has been added to ELPA, then just

  :vc t

> By the way, unlike with some other packages I've submitted, I've not
> fully settled on the name, so I'm open to suggestions.  I considered
> naming it "EMP" (for "Emacs Music Player", which is fun and concise,
> and which is surprisingly not yet taken--although Nic Vollmer has an
> unpackaged library by that name, but he's graciously offered me the
> name for use with this package).  But "Listen" seemed more descriptive
> and concise enough, so that's what I've gone with for now.

I like the name ;)

> Thanks,
> Adam
>
>

-- 
	Philip Kaludercic on icterid

Re: [ELPA] New package: listen

[Adam Porter]

Hi Philip,

Thanks for your review.  Comments to follow:

On 2/25/24 05:43, Philip Kaludercic wrote:
> Adam Porter <adam@alphapapa.net> writes:
> 
>> Hi,
>>
>> I'd like to submit a new package to ELPA, named "Listen".  It's a
>> simple audio/music player that uses VLC as a backend.
>>
>> https://github.com/alphapapa/listen.el
> 
> Note that the copyright lines still have to be updated for it to be
> added to GNU ELPA.

Oops, I forgot to update the ones other than in listen.el.  Done.

> Here are a few comments, questions and suggestions from briefly skimming
> the code:
> 
> 
> diff --git a/listen-info.el b/listen-info.el
> index e74d376df9..a95a5b1d1b 100644
> --- a/listen-info.el
> +++ b/listen-info.el
> @@ -56,7 +56,6 @@
>   
>   (require 'bindat)
>   (require 'cl-lib)
> -(require 'seq)

The `seq' library is used in the file, so shouldn't this line be present?

> @@ -942,10 +941,10 @@ extension.
>   
>   Return one of symbols `vorbis', `opus', `flac', or `mp3'."
>     (let ((case-fold-search t))
> -    (cond ((string-match ".ogg$" filename) 'vorbis)
> -          ((string-match ".opus$" filename) 'opus)
> -          ((string-match ".flac$" filename) 'flac)
> -          ((string-match ".mp3$" filename) 'mp3)
> +    (cond ((string-match ".ogg\\'" filename) 'vorbis)
> +          ((string-match ".opus\\'" filename) 'opus)
> +          ((string-match ".flac\\'" filename) 'flac)
> +          ((string-match ".mp3\\'" filename) 'mp3)
>             (t nil))))

According to the Elisp manual: "When matching a string instead of a 
buffer, ‘$’ matches at the end of the string or before a newline 
character."  So it appears to be correct here (and its meaning is widely 
known, whereas the Emacsism "\\'" is not (I can never remember the 
escape sequences).

A good change to make, though, would be to escape the period, as 
otherwise it would match any character, as in "fooogg" rather than 
"foo.ogg".

> diff --git a/listen-lib.el b/listen-lib.el
> index 7743080c23..b3d48f51fa 100644
> --- a/listen-lib.el
> +++ b/listen-lib.el
> @@ -50,7 +50,7 @@
>   ;;;; Faces
>   
>   (defgroup listen-faces nil
> -  "Faces."
> +  "Faces used by listen.el."
>     :group 'listen)

As the `listen-faces' group is within the `listen' group, I would think 
that that its faces are related to `listen' goes without saying.

> diff --git a/listen-library.el b/listen-library.el
> index 77022e1791..90d40468ad 100644
> --- a/listen-library.el
> +++ b/listen-library.el
> @@ -100,8 +100,8 @@ mode line and bookmark name.  BUFFER may be specified in which to
>   show the view."
>     (interactive
>      (list (list (read-file-name "View library for: "))
> -         :name (when current-prefix-arg
> -                 (read-string "Library name: "))))
> +         :name (and current-prefix-arg
> +                    (read-string "Library name: "))))

No, thanks.  I'm aware of this minor tradition, but I disagree with it.

> @@ -181,7 +181,7 @@ Interactively, read COMMAND and use tracks at point in
>       ;; it ought to return sections in the region), it returns nil.
>       ;; This may be confusing to users, but it seems like an upstream
>       ;; issue.
> -    (or (flatten-list (mapcar #'value-of (magit-region-sections)))
> +    (or (flatten-list (mapcar #'value-of (magit-region-sections))) ;or `mapcan'?
>           (value-of (magit-current-section)))))

Probably a good idea.

> @@ -192,7 +192,7 @@ Interactively, read COMMAND and use tracks at point in
>     "Return a bookmark record for the current library buffer."
>     (cl-assert listen-library-paths)
>     `(,(format "Listen library: %s" (or listen-library-name listen-library-paths))
> -    (handler . listen-library--bookmark-handler)
> +    (handler . ,#'listen-library--bookmark-handler)

Is there any advantage to this, given that the function is defined a few 
lines later?  Compiler warnings don't seem relevant here.

> diff --git a/listen-mpd.el b/listen-mpd.el
> index b8a104febe..8c3a302f08 100644
> --- a/listen-mpd.el
> +++ b/listen-mpd.el
> @@ -89,17 +89,17 @@ completion."
>                                         (cons 'affixation-function #'affix)
>                                         ;; (cons 'annotation-function #'annotate)
>                                         ))))
> -                  (`t (unless (string-empty-p str)
> -                        (let ((tag (pcase tag
> -                                     ('any 'file)
> -                                     (_ tag))))
> -                          (delete-dups
> -                           (delq nil
> -                                 (mapcar (lambda (row)
> -                                           (when-let ((value (alist-get tag row)))
> -                                             (propertize value
> -                                                         :mpc-alist row)))
> -                                         (search-any (split-string str))))))))))
> +                  ((guard (not (string-empty-p str)))
> +                   (let ((tag (pcase tag
> +                                ('any 'file)
> +                                (_ tag))))
> +                     (delete-dups
> +                      (delq nil
> +                            (mapcar (lambda (row)
> +                                      (when-let ((value (alist-get tag row)))
> +                                        (propertize value
> +                                                    :mpc-alist row)))
> +                                    (search-any (split-string str)))))))))

AFAICT, this change would not be strictly equivalent, as the original 
tests explicitly for t before testing the string, which, AFAICT, is the 
correct way to handle this part of the completion API.

> diff --git a/listen-queue.el b/listen-queue.el
> index 9515e3d7dc..cf4bd494bd 100644
> --- a/listen-queue.el
> +++ b/listen-queue.el
> @@ -312,7 +312,7 @@ PROMPT is passed to `format-prompt', which see."
>        ;; homedir path (so queues could be portable with music
>        ;; libraries).
>        :filename (abbreviate-file-name filename)
> -     :artist (map-elt metadata "artist")
> +     :artist (map-elt metadata "artist") ;`metadata' is an alist, right?  So why not use `let-alist'?

Please note that the keys are strings, which AFAICT `let-alist' is not 
compatible with (nor `pcase-let', as it does not allow the test function 
to be specified).  `map-elt' tests with `equal'.

> @@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see."
>            n)
>       (when current-track
>         (cl-callf2 delete current-track tracks))
> -    (setf n (length tracks))
> +    (setf n (length tracks))   ;why the variable?

Because the value is used elsewhere in the function.  Am I missing 
something?  (Anyway, as noted in the source, I did not write that function.)

> @@ -357,6 +357,8 @@ PROMPT is passed to `format-prompt', which see."
>                  (1+ (seq-position (listen-queue-tracks queue)
>                                    (listen-queue-current queue)
>                                    (lambda (a b)
> +                                   ;; FWIW you still have a similar
> +                                   ;; race condition here.

I don't understand what you mean.  The comment before this hunk of code 
is not about a race condition (not a subtle or random one, anyway--it's 
a matter of whether the user manually modifies a track and then 
refreshes the queue's tracks from disk while the track is playing, 
something that is unlikely to happen unless the user is actively tidying 
their tracks' metadata, and fixing it will require more careful 
consideration; as well, if the file gets renamed in the process, there's 
little that can be done, as the track's primary identifier will no 
longer exist).

> @@ -427,7 +429,7 @@ tracks in the queue unchanged)."
>           (goto-char beg)
>           (cl-loop collect (vtable-current-object)
>                    do (forward-line 1)
> -                 while (<= (point) end))))))
> +                 until (eobp))))))

This code does not test for the end of the buffer, so `eobp' would not 
be appropriate.

> --- a/listen-vlc.el
> +++ b/listen-vlc.el
> @@ -43,7 +43,8 @@
>       (save-excursion
>         (insert (listen--send player "info")))
>       (cl-loop while (re-search-forward (rx bol "| " (group (1+ (not blank))) ": "
> -                                          (group (1+ (not (any ""))))) nil t)
> +                                          (group (1+ (not (any "\C-m")))))
> +                                      nil t)

Other than the formatting, what is the purpose of this change?  This 
function parses output from VLC, and it appears to work correctly.

> @@ -116,7 +117,7 @@ Stops playing, clears playlist, adds FILE, and plays it."
>     "Return or set PLAYER's VOLUME.
>   VOLUME is an integer percentage."
>     (if volume
> -      (listen--send player (format "volume %s" (* 255 (/ volume 100.0))))
> +      (listen--send player (format "volume %s" (* 255 (/ volume 100.0)))) ;what if this is OOB?

"Out Of Bounds"?  What do you mean?  VLC returns a value from 0-255, and 
`volume' is specified to be an integer percentage (i.e. from 0-100).  As 
far as I can tell, this works correctly.

> diff --git a/listen.el b/listen.el
> index 3e3d11d572..c381b2a113 100755
> --- a/listen.el
> +++ b/listen.el
> @@ -75,7 +75,7 @@
>     :link '(emacs-commentary-link "listen")
>     :link '(emacs-library-link "listen"))
>   
> -(defcustom listen-directory "~/Music"
> +(defcustom listen-directory "~/Music"   ;perhaps check XDG_MUSIC_DIR here?

I cannot find such a variable defined, neither on my system nor on a Web 
search.  It doesn't appear to be part of the XDG standard.

> -(declare-function listen-queue-next "listen-queue")
> +(declare-function listen-queue-next "listen-queue") ;why not `require' it?

Because it is an autoloaded command, one which the user might not use, 
so loading the library might be unnecessary.

> @@ -251,19 +251,19 @@ command with completion."
>   (defun listen-read-time (time)
>     "Return TIME in seconds.
>   TIME is an HH:MM:SS string."
> -  (string-match (rx (group (1+ num))
> -                    (optional ":" (group (1+ num))
> -                              (optional ":" (group (1+ num)))))
> -                time)
> -  (let ((fields (nreverse
> -                 (remq nil
> -                       (list (match-string 1 time)
> -                             (match-string 2 time)
> -                             (match-string 3 time)))))
> -        (factors [1 60 3600]))
> -    (cl-loop for field in fields
> -             for factor across factors
> -             sum (* (string-to-number field) factor))))
> +  (when (string-match (rx (group (1+ num))
> +                          (optional ":" (group (1+ num))
> +                                    (optional ":" (group (1+ num)))))
> +                      time)
> +    (let ((fields (nreverse
> +                   (remq nil
> +                         (list (match-string 1 time)
> +                               (match-string 2 time)
> +                               (match-string 3 time)))))
> +          (factors [1 60 3600]))
> +      (cl-loop for field in fields
> +               for factor across factors
> +               sum (* (string-to-number field) factor)))))

If this function fails, I want it to signal an error, not return nil.

>> For more details (though there's not much else to say), please see the
>> readme/Info manual.
> 
> On the topic of the readme/manual, wouldn't it be better to have a
> separate README file?  Then again, the manual is pretty short, and I
> don't know if it is worth having it in the first place...

As you said, this readme is currently trivial.  Were it larger, 
however--well, I have other packages with much larger readmes that are 
also converted to manuals.  There would not be much gained by separating 
into files.

> Also, your README includes this line
> 
>    :vc (:fetcher github :repo "alphapapa/listen.el")
> 
> which is malformed.  

I just tested that, and it works for me.

> What you want is
> 
>    :vc (:url "https://github.com/alphapapa/listen.el")
> 
> or after the package has been added to ELPA, then just
> 
>    :vc t

I added that to the readme for before the package is available through 
ELPA.  Afterward I intend to recommend installing it from ELPA, not from 
git.

Please let me know if any other changes are needed.

--Adam

Re: [ELPA] New package: listen

[Philip Kaludercic]

Adam Porter <adam@alphapapa.net> writes:

> Hi Philip,
>
> Thanks for your review.  Comments to follow:
>
> On 2/25/24 05:43, Philip Kaludercic wrote:
>> Adam Porter <adam@alphapapa.net> writes:
>> 
>>> Hi,
>>>
>>> I'd like to submit a new package to ELPA, named "Listen".  It's a
>>> simple audio/music player that uses VLC as a backend.
>>>
>>> https://github.com/alphapapa/listen.el
>> Note that the copyright lines still have to be updated for it to be
>> added to GNU ELPA.
>
> Oops, I forgot to update the ones other than in listen.el.  Done.
>
>> Here are a few comments, questions and suggestions from briefly skimming
>> the code:
>> diff --git a/listen-info.el b/listen-info.el
>> index e74d376df9..a95a5b1d1b 100644
>> --- a/listen-info.el
>> +++ b/listen-info.el
>> @@ -56,7 +56,6 @@
>>     (require 'bindat)
>>   (require 'cl-lib)
>> -(require 'seq)
>
> The `seq' library is used in the file, so shouldn't this line be present?

Sorry, should have explained that.  If you depend on Emacs 29, then seq
is preloaded, so you don't need the line.  If you prefer to, you can
keep it in though.

>> @@ -942,10 +941,10 @@ extension.
>>     Return one of symbols `vorbis', `opus', `flac', or `mp3'."
>>     (let ((case-fold-search t))
>> -    (cond ((string-match ".ogg$" filename) 'vorbis)
>> -          ((string-match ".opus$" filename) 'opus)
>> -          ((string-match ".flac$" filename) 'flac)
>> -          ((string-match ".mp3$" filename) 'mp3)
>> +    (cond ((string-match ".ogg\\'" filename) 'vorbis)
>> +          ((string-match ".opus\\'" filename) 'opus)
>> +          ((string-match ".flac\\'" filename) 'flac)
>> +          ((string-match ".mp3\\'" filename) 'mp3)
>>             (t nil))))
>
> According to the Elisp manual: "When matching a string instead of a
> buffer, ‘$’ matches at the end of the string or before a newline
> character."  So it appears to be correct here (and its meaning is
> widely known, whereas the Emacsism "\\'" is not (I can never remember
> the escape sequences).

Hmm, I just went by the convention used in auto-mode-alist here (the
trick to remember \\' is that \\' and \\' always occur in the same order
as you would quote a symbol in a docstring).

> A good change to make, though, would be to escape the period, as
> otherwise it would match any character, as in "fooogg" rather than
> "foo.ogg".

Right.

>> diff --git a/listen-lib.el b/listen-lib.el
>> index 7743080c23..b3d48f51fa 100644
>> --- a/listen-lib.el
>> +++ b/listen-lib.el
>> @@ -50,7 +50,7 @@
>>   ;;;; Faces
>>     (defgroup listen-faces nil
>> -  "Faces."
>> +  "Faces used by listen.el."
>>     :group 'listen)
>
> As the `listen-faces' group is within the `listen' group, I would
> think that that its faces are related to `listen' goes without saying.

But that context would be missing if you were to use something like
`customize-apropos-groups'.

>> diff --git a/listen-library.el b/listen-library.el
>> index 77022e1791..90d40468ad 100644
>> --- a/listen-library.el
>> +++ b/listen-library.el
>> @@ -100,8 +100,8 @@ mode line and bookmark name.  BUFFER may be specified in which to
>>   show the view."
>>     (interactive
>>      (list (list (read-file-name "View library for: "))
>> -         :name (when current-prefix-arg
>> -                 (read-string "Library name: "))))
>> +         :name (and current-prefix-arg
>> +                    (read-string "Library name: "))))
>
> No, thanks.  I'm aware of this minor tradition, but I disagree with it.

I wouldn't call it a minor tradition (e.g. in Scheme the return value of
`when' is unspecified.  CLtL2 says "If the value is relevant, then it
may be stylistically more appropriate to use and or if."), and I am
curious why you disagree with it, but you are of course free to do as
you want.

>> @@ -181,7 +181,7 @@ Interactively, read COMMAND and use tracks at point in
>>       ;; it ought to return sections in the region), it returns nil.
>>       ;; This may be confusing to users, but it seems like an upstream
>>       ;; issue.
>> -    (or (flatten-list (mapcar #'value-of (magit-region-sections)))
>> +    (or (flatten-list (mapcar #'value-of (magit-region-sections))) ;or `mapcan'?
>>           (value-of (magit-current-section)))))
>
> Probably a good idea.

There were a few other places where you did (delq nil (mapcar ...)) that
might be replaced by this pattern.

>> @@ -192,7 +192,7 @@ Interactively, read COMMAND and use tracks at point in
>>     "Return a bookmark record for the current library buffer."
>>     (cl-assert listen-library-paths)
>>     `(,(format "Listen library: %s" (or listen-library-name listen-library-paths))
>> -    (handler . listen-library--bookmark-handler)
>> +    (handler . ,#'listen-library--bookmark-handler)
>
> Is there any advantage to this, given that the function is defined a
> few lines later?  Compiler warnings don't seem relevant here.

Not really, if you don't care about compiler warnings.  It just seems
like the kind of things that could cause problems at some later time,
when definitions are moved around.

>> diff --git a/listen-mpd.el b/listen-mpd.el
>> index b8a104febe..8c3a302f08 100644
>> --- a/listen-mpd.el
>> +++ b/listen-mpd.el
>> @@ -89,17 +89,17 @@ completion."
>>                                         (cons 'affixation-function #'affix)
>>                                         ;; (cons 'annotation-function #'annotate)
>>                                         ))))
>> -                  (`t (unless (string-empty-p str)
>> -                        (let ((tag (pcase tag
>> -                                     ('any 'file)
>> -                                     (_ tag))))
>> -                          (delete-dups
>> -                           (delq nil
>> -                                 (mapcar (lambda (row)
>> -                                           (when-let ((value (alist-get tag row)))
>> -                                             (propertize value
>> -                                                         :mpc-alist row)))
>> -                                         (search-any (split-string str))))))))))
>> +                  ((guard (not (string-empty-p str)))
>> +                   (let ((tag (pcase tag
>> +                                ('any 'file)
>> +                                (_ tag))))
>> +                     (delete-dups
>> +                      (delq nil
>> +                            (mapcar (lambda (row)
>> +                                      (when-let ((value (alist-get tag row)))
>> +                                        (propertize value
>> +                                                    :mpc-alist row)))
>> +                                    (search-any (split-string str)))))))))
>
> AFAICT, this change would not be strictly equivalent, as the original
> tests explicitly for t before testing the string, which, AFAICT, is
> the correct way to handle this part of the completion API.

Ah, the `t did confuse me momentarily, but in that case you can replace
the (guard ...) with (and 't (guard ...)).

>> diff --git a/listen-queue.el b/listen-queue.el
>> index 9515e3d7dc..cf4bd494bd 100644
>> --- a/listen-queue.el
>> +++ b/listen-queue.el
>> @@ -312,7 +312,7 @@ PROMPT is passed to `format-prompt', which see."
>>        ;; homedir path (so queues could be portable with music
>>        ;; libraries).
>>        :filename (abbreviate-file-name filename)
>> -     :artist (map-elt metadata "artist")
>> + :artist (map-elt metadata "artist") ;`metadata' is an alist,
>> right?  So why not use `let-alist'?
>
> Please note that the keys are strings, which AFAICT `let-alist' is not
> compatible with (nor `pcase-let', as it does not allow the test
> function to be specified).  `map-elt' tests with `equal'.

OK, I see.

>> @@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see."
>>            n)
>>       (when current-track
>>         (cl-callf2 delete current-track tracks))
>> -    (setf n (length tracks))
>> +    (setf n (length tracks))   ;why the variable?
>
> Because the value is used elsewhere in the function.  Am I missing
> something?  (Anyway, as noted in the source, I did not write that
> function.)

Then I missed something, because I just saw the variable being declared
in the let-head, set here and used once later.

>> @@ -357,6 +357,8 @@ PROMPT is passed to `format-prompt', which see."
>>                  (1+ (seq-position (listen-queue-tracks queue)
>>                                    (listen-queue-current queue)
>>                                    (lambda (a b)
>> +                                   ;; FWIW you still have a similar
>> +                                   ;; race condition here.
>
> I don't understand what you mean.  The comment before this hunk of
> code is not about a race condition (not a subtle or random one,
> anyway--it's a matter of whether the user manually modifies a track
> and then refreshes the queue's tracks from disk while the track is
> playing, something that is unlikely to happen unless the user is
> actively tidying their tracks' metadata, and fixing it will require
> more careful consideration; as well, if the file gets renamed in the
> process, there's little that can be done, as the track's primary
> identifier will no longer exist).

My understanding was that you were handling the case where files could
be renamed, but if that is not your concern, then disregard the comment.

>> @@ -427,7 +429,7 @@ tracks in the queue unchanged)."
>>           (goto-char beg)
>>           (cl-loop collect (vtable-current-object)
>>                    do (forward-line 1)
>> -                 while (<= (point) end))))))
>> +                 until (eobp))))))
>
> This code does not test for the end of the buffer, so `eobp' would not
> be appropriate.

You are right, never mind.

>> --- a/listen-vlc.el
>> +++ b/listen-vlc.el
>> @@ -43,7 +43,8 @@
>>       (save-excursion
>>         (insert (listen--send player "info")))
>>       (cl-loop while (re-search-forward (rx bol "| " (group (1+ (not blank))) ": "
>> -                                          (group (1+ (not (any ""))))) nil t)
>> +                                          (group (1+ (not (any "\C-m")))))
>> +                                      nil t)
>
> Other than the formatting, what is the purpose of this change?  This
> function parses output from VLC, and it appears to work correctly.

Formatting.  Emacs highlights the "nil t" as occurring in-between closed
parentheses, since it can be easily missed.

>> @@ -116,7 +117,7 @@ Stops playing, clears playlist, adds FILE, and plays it."
>>     "Return or set PLAYER's VOLUME.
>>   VOLUME is an integer percentage."
>>     (if volume
>> -      (listen--send player (format "volume %s" (* 255 (/ volume 100.0))))
>> +      (listen--send player (format "volume %s" (* 255 (/ volume 100.0)))) ;what if this is OOB?
>
> "Out Of Bounds"?  What do you mean?  VLC returns a value from 0-255,
> and `volume' is specified to be an integer percentage (i.e. from
> 0-100).  As far as I can tell, this works correctly.

I couldn't infer that from reading the function, so I wondered what
happens when `volume' is not between 0 and 100.  Perhaps a cl-assert
would be nice to have.

>> diff --git a/listen.el b/listen.el
>> index 3e3d11d572..c381b2a113 100755
>> --- a/listen.el
>> +++ b/listen.el
>> @@ -75,7 +75,7 @@
>>     :link '(emacs-commentary-link "listen")
>>     :link '(emacs-library-link "listen"))
>>   -(defcustom listen-directory "~/Music"
>> +(defcustom listen-directory "~/Music"   ;perhaps check XDG_MUSIC_DIR here?
>
> I cannot find such a variable defined, neither on my system nor on a
> Web search.  It doesn't appear to be part of the XDG standard.
>
>> -(declare-function listen-queue-next "listen-queue")
>> +(declare-function listen-queue-next "listen-queue") ;why not `require' it?
>
> Because it is an autoloaded command, one which the user might not use,
> so loading the library might be unnecessary.
>
>> @@ -251,19 +251,19 @@ command with completion."
>>   (defun listen-read-time (time)
>>     "Return TIME in seconds.
>>   TIME is an HH:MM:SS string."
>> -  (string-match (rx (group (1+ num))
>> -                    (optional ":" (group (1+ num))
>> -                              (optional ":" (group (1+ num)))))
>> -                time)
>> -  (let ((fields (nreverse
>> -                 (remq nil
>> -                       (list (match-string 1 time)
>> -                             (match-string 2 time)
>> -                             (match-string 3 time)))))
>> -        (factors [1 60 3600]))
>> -    (cl-loop for field in fields
>> -             for factor across factors
>> -             sum (* (string-to-number field) factor))))
>> +  (when (string-match (rx (group (1+ num))
>> +                          (optional ":" (group (1+ num))
>> +                                    (optional ":" (group (1+ num)))))
>> +                      time)
>> +    (let ((fields (nreverse
>> +                   (remq nil
>> +                         (list (match-string 1 time)
>> +                               (match-string 2 time)
>> +                               (match-string 3 time)))))
>> +          (factors [1 60 3600]))
>> +      (cl-loop for field in fields
>> +               for factor across factors
>> +               sum (* (string-to-number field) factor)))))
>
> If this function fails, I want it to signal an error, not return nil.

Am I missing something, or where will the error be signaled?  If the
pattern doesn't match, the match data won't be modified, and you'll
extract arbitrary substrings out of TIME. `String-to-number' doesn't
raise an error on malformed input,

  (string-to-number "31-") ;=> 31 (#o37, #x1f, ?\C-_)
  (string-to-number "x")   ;=> 0 (#o0, #x0, ?\C-@)
  (string-to-number "")    ;=> 0 (#o0, #x0, ?\C-@)

the only exception being if there was no match data for some n

  (string-to-number (match-string 100)) ;signals (wrong-type-argument stringp nil)

If you want to signal an error, then I think the robust thing would be
to check if `string-match' succeeds as proposed above, but using an `if'
not a `when' to raise an error in the ELSE case.

>>> For more details (though there's not much else to say), please see the
>>> readme/Info manual.
>> On the topic of the readme/manual, wouldn't it be better to have a
>> separate README file?  Then again, the manual is pretty short, and I
>> don't know if it is worth having it in the first place...
>
> As you said, this readme is currently trivial.  Were it larger,
> however--well, I have other packages with much larger readmes that are
> also converted to manuals.  There would not be much gained by
> separating into files.

I don't think that is a good practice.  A README for when you have
fetched the sources and want to figure out what is what, a manual for
when you have already installed a package and a package description for
when you are previewing a package using C-h P are three different
things.  One shouldn't cover all of it with the same file if you ask me,
since they all have different audiences with different levels of
interest.

>> Also, your README includes this line
>>    :vc (:fetcher github :repo "alphapapa/listen.el")
>> which is malformed.  
>
> I just tested that, and it works for me.

On Emacs 30?  That is not the code we merged...

--8<---------------cut here---------------start------------->8---
Debugger entered--Lisp error: (error "use-package: Keyword :vc received unknown argument: :fetcher. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev)")
  signal(error ("use-package: Keyword :vc received unknown argument: :fetcher. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev)"))
  error("use-package: %s" "Keyword :vc received unknown argument: :fetcher. Supported keywords are: (:url :branch :lisp-dir :main-file :vc-backend :rev)")
  use-package-normalize--vc-arg((listen :fetcher github :repo "alphapapa/listen.el"))
  use-package-normalize/:vc(listen :vc ((:fetcher github :repo "alphapapa/listen.el")))
  use-package-normalize-plist(listen (:vc (:fetcher github :repo "alphapapa/listen.el")) nil use-package-merge-keys)
  use-package-normalize-keywords(listen (:vc (:fetcher github :repo "alphapapa/listen.el")))
  ...
--8<---------------cut here---------------end--------------->8---

>> What you want is
>>    :vc (:url "https://github.com/alphapapa/listen.el")
>> or after the package has been added to ELPA, then just
>>    :vc t
>
> I added that to the readme for before the package is available through
> ELPA.  Afterward I intend to recommend installing it from ELPA, not
> from git.

Yes, I noticed that after I had sent my message.  That is better
practice anyway, at least for the regular user.

> Please let me know if any other changes are needed.
>
> --Adam
>

-- 
	Philip Kaludercic on icterid

Re: [ELPA] New package: listen

[Andreas Schwab]

On Feb 25 2024, Adam Porter wrote:

>> @@ -942,10 +941,10 @@ extension.
>>     Return one of symbols `vorbis', `opus', `flac', or `mp3'."
>>     (let ((case-fold-search t))
>> -    (cond ((string-match ".ogg$" filename) 'vorbis)
>> -          ((string-match ".opus$" filename) 'opus)
>> -          ((string-match ".flac$" filename) 'flac)
>> -          ((string-match ".mp3$" filename) 'mp3)
>> +    (cond ((string-match ".ogg\\'" filename) 'vorbis)
>> +          ((string-match ".opus\\'" filename) 'opus)
>> +          ((string-match ".flac\\'" filename) 'flac)
>> +          ((string-match ".mp3\\'" filename) 'mp3)
>>             (t nil))))
>
> According to the Elisp manual: "When matching a string instead of a
> buffer, ‘$’ matches at the end of the string or before a newline
> character."  So it appears to be correct here

It should only match at the end of the string, not before a newline.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [ELPA] New package: listen

[Adam Porter]

Hi Andreas,

On 2/25/24 08:17, Andreas Schwab wrote:
> On Feb 25 2024, Adam Porter wrote:
> 
>>> @@ -942,10 +941,10 @@ extension.
>>>      Return one of symbols `vorbis', `opus', `flac', or `mp3'."
>>>      (let ((case-fold-search t))
>>> -    (cond ((string-match ".ogg$" filename) 'vorbis)
>>> -          ((string-match ".opus$" filename) 'opus)
>>> -          ((string-match ".flac$" filename) 'flac)
>>> -          ((string-match ".mp3$" filename) 'mp3)
>>> +    (cond ((string-match ".ogg\\'" filename) 'vorbis)
>>> +          ((string-match ".opus\\'" filename) 'opus)
>>> +          ((string-match ".flac\\'" filename) 'flac)
>>> +          ((string-match ".mp3\\'" filename) 'mp3)
>>>              (t nil))))
>>
>> According to the Elisp manual: "When matching a string instead of a
>> buffer, ‘$’ matches at the end of the string or before a newline
>> character."  So it appears to be correct here
> 
> It should only match at the end of the string, not before a newline.

As the manual says:

‘$’
      is similar to ‘^’ but matches only at the end of a line (or the end
      of the accessible portion of the buffer).  Thus, ‘x+$’ matches a
      string of one ‘x’ or more at the end of a line.

      When matching a string instead of a buffer, ‘$’ matches at the end
      of the string or before a newline character.

Re: [ELPA] New package: listen

[Adam Porter]

Hi Philip,

On 2/25/24 07:45, Philip Kaludercic wrote:

>> The `seq' library is used in the file, so shouldn't this line be present?
> 
> Sorry, should have explained that.  If you depend on Emacs 29, then seq
> is preloaded, so you don't need the line.  If you prefer to, you can
> keep it in though.

I'll keep it for now, because I'm not ruling out the possibility of 
supporting earlier versions at some point.

> Hmm, I just went by the convention used in auto-mode-alist here (the
> trick to remember \\' is that \\' and \\' always occur in the same order
> as you would quote a symbol in a docstring).

This is why I like `rx'.  :)

>> As the `listen-faces' group is within the `listen' group, I would
>> think that that its faces are related to `listen' goes without saying.
> 
> But that context would be missing if you were to use something like
> `customize-apropos-groups'.

Good point.  I'll fix that.

>> No, thanks.  I'm aware of this minor tradition, but I disagree with it.
> 
> I wouldn't call it a minor tradition (e.g. in Scheme the return value of
> `when' is unspecified.  CLtL2 says "If the value is relevant, then it
> may be stylistically more appropriate to use and or if."), and I am
> curious why you disagree with it, but you are of course free to do as
> you want.

Well, Elisp isn't Scheme, and CLtL2 seems to say that it is exactly a 
minor tradition.  :)

I don't like it for a few reasons:

1. Using AND puts the conditional expressions on the same level, 
visually and logically, as the value.  In contrast, using WHEN cleanly 
separates the condition(s) from the value.  IMO that's a significant 
advantage, as it makes the purpose of the code much clearer.

2. WHEN's indentation also saves space, helping to avoid long lines or 
awkwardly wrapped ones.

> There were a few other places where you did (delq nil (mapcar ...)) that
> might be replaced by this pattern.

I confess that I've hardly ever used MAPCAN in any of my code--still, 
I'm not sure how that would help to avoid that pattern.  For example:

   (let ((a '(1 nil 2 nil 3))
         (b '(4 nil 5 nil 6)))
     (mapcan #'identity (list a b)))

   ;; (1 nil 2 nil 3 4 nil 5 nil 6)

But I'm probably missing something.

> Not really, if you don't care about compiler warnings.  It just seems
> like the kind of things that could cause problems at some later time,
> when definitions are moved around.

I do care about compiler warnings--I wrote makem.sh to catch such 
warnings in my projects before they reach users--but in this case, that 
code's not going anywhere, because it's already with bookmark-related code.

> Ah, the `t did confuse me momentarily, but in that case you can replace
> the (guard ...) with (and 't (guard ...)).

As much as I advocate using pcase and its powerful expressions, I think 
that would make this example harder to follow.  The pcase pattern is 
used to test an argument, and the string test is a separate concern.

>>> @@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see."
>>>             n)
>>>        (when current-track
>>>          (cl-callf2 delete current-track tracks))
>>> -    (setf n (length tracks))
>>> +    (setf n (length tracks))   ;why the variable?
>>
>> Because the value is used elsewhere in the function.  Am I missing
>> something?  (Anyway, as noted in the source, I did not write that
>> function.)
> 
> Then I missed something, because I just saw the variable being declared
> in the let-head, set here and used once later.

It's used in two places, and it would be undesirable to recalculate the 
list's length every time through the loop.

> My understanding was that you were handling the case where files could
> be renamed, but if that is not your concern, then disregard the comment.

Handling the case of files being renamed "out from under" the queue 
seems like a non-trivial problem to solve well.  The `listen-library' 
command and view record the initial paths used, i.e. both directories 
and files, but the queue only records filenames, so if those are 
changed, there's not a reliable way to rediscover the renamed files. 
AFAIK other music library software handles it by removing the 
now-missing files from its database and rescanning the whole directory 
tree to find new files--but even that wouldn't replace tracks in a 
playlist if their underlying files disappear.  `listen' doesn't keep a 
database, just queues of tracks.  So I don't know if this particular 
problem is reasonably solvable.  (Again, it can only happen if the user 
intentionally renames files during playback, which I discovered while 
tidying up the metadata on some of my files.)

> Formatting.  Emacs highlights the "nil t" as occurring in-between closed
> parentheses, since it can be easily missed.

Yes, but in this case, the "nil t" is a common enough pattern that I'm 
willing to allow it to be a "trailer" to avoid "nil t" on a line by 
themselves.

>> "Out Of Bounds"?  What do you mean?  VLC returns a value from 0-255,
>> and `volume' is specified to be an integer percentage (i.e. from
>> 0-100).  As far as I can tell, this works correctly.
> 
> I couldn't infer that from reading the function, so I wondered what
> happens when `volume' is not between 0 and 100.  Perhaps a cl-assert
> would be nice to have.

Good idea.  I'll add that.

>> If this function fails, I want it to signal an error, not return nil.
> 
> Am I missing something, or where will the error be signaled?  If the
> pattern doesn't match, the match data won't be modified, and you'll
> extract arbitrary substrings out of TIME. `String-to-number' doesn't
> raise an error on malformed input,
> 
>    (string-to-number "31-") ;=> 31 (#o37, #x1f, ?\C-_)
>    (string-to-number "x")   ;=> 0 (#o0, #x0, ?\C-@)
>    (string-to-number "")    ;=> 0 (#o0, #x0, ?\C-@)
> 
> the only exception being if there was no match data for some n
> 
>    (string-to-number (match-string 100)) ;signals (wrong-type-argument stringp nil)
> 
> If you want to signal an error, then I think the robust thing would be
> to check if `string-match' succeeds as proposed above, but using an `if'
> not a `when' to raise an error in the ELSE case.

It could be more robust, yes.  Note that this function is only passed a 
string read from the user and returns an integer, so if it fails, it's 
not a big deal.  I'll think about how to make it fail more usefully...

>>> On the topic of the readme/manual, wouldn't it be better to have a
>>> separate README file?  Then again, the manual is pretty short, and I
>>> don't know if it is worth having it in the first place...
>>
>> As you said, this readme is currently trivial.  Were it larger,
>> however--well, I have other packages with much larger readmes that are
>> also converted to manuals.  There would not be much gained by
>> separating into files.
> 
> I don't think that is a good practice.  A README for when you have
> fetched the sources and want to figure out what is what, a manual for
> when you have already installed a package and a package description for
> when you are previewing a package using C-h P are three different
> things.  One shouldn't cover all of it with the same file if you ask me,
> since they all have different audiences with different levels of
> interest.

I don't know about that.  Especially for small packages with trivial 
documentation.  Maintaining documentation and commentaries, keeping them 
reasonably in sync, etc. is enough work without having them split into 
multiple files.  Having a README.org which is viewable at the package's 
repo also generate the manual is a relief to me.

>>> Also, your README includes this line
>>>     :vc (:fetcher github :repo "alphapapa/listen.el")
>>> which is malformed.
>>
>> I just tested that, and it works for me.
> 
> On Emacs 30?  That is not the code we merged...

No, I'm using Emacs 29 with `vc-use-package'.  Its documentation seems 
to suggest that it uses the same format as that merged into Emacs 30, 
since it says that its features were merged into Emacs 30.

Maybe `vc-use-package's documentation should be updated to reflect this?

>>> What you want is
>>>     :vc (:url "https://github.com/alphapapa/listen.el")

Ok, so no ".git" on the end (i.e. relying on the GitHub redirect)?  And 
does this mean that none of the host-specific "fetchers" are available 
in Emacs 30?  (Which FTR is fine with me, as the URL should be enough, 
I'm just curious.)

--Adam

Re: [ELPA] New package: listen

[Philip Kaludercic]

Adam Porter <adam@alphapapa.net> writes:

> Hi Andreas,
>
> On 2/25/24 08:17, Andreas Schwab wrote:
>> On Feb 25 2024, Adam Porter wrote:
>> 
>>>> @@ -942,10 +941,10 @@ extension.
>>>>      Return one of symbols `vorbis', `opus', `flac', or `mp3'."
>>>>      (let ((case-fold-search t))
>>>> -    (cond ((string-match ".ogg$" filename) 'vorbis)
>>>> -          ((string-match ".opus$" filename) 'opus)
>>>> -          ((string-match ".flac$" filename) 'flac)
>>>> -          ((string-match ".mp3$" filename) 'mp3)
>>>> +    (cond ((string-match ".ogg\\'" filename) 'vorbis)
>>>> +          ((string-match ".opus\\'" filename) 'opus)
>>>> +          ((string-match ".flac\\'" filename) 'flac)
>>>> +          ((string-match ".mp3\\'" filename) 'mp3)
>>>>              (t nil))))
>>>
>>> According to the Elisp manual: "When matching a string instead of a
>>> buffer, ‘$’ matches at the end of the string or before a newline
>>> character."  So it appears to be correct here
>> It should only match at the end of the string, not before a newline.
>
> As the manual says:
>
> ‘$’
>      is similar to ‘^’ but matches only at the end of a line (or the end
>      of the accessible portion of the buffer).  Thus, ‘x+$’ matches a
>      string of one ‘x’ or more at the end of a line.
>
>      When matching a string instead of a buffer, ‘$’ matches at the end
>      of the string or before a newline character.
                     ^
                     this is the problem.

Since it is possible for file-names to contain newline characters,
matching the end of a line can result in false-positives:

(string-match-p
 "\\.el$"
 "foo.el\nbar.le")
;;=> 3

(string-match-p
 "\\.el\\'"
 "foo.el\nbar.le")
;;=> nil

(It's called the "manual", not the "infallible" (hehe) for a reason)

-- 
	Philip Kaludercic on peregrine

Re: [ELPA] New package: listen

[Philip Kaludercic]

Adam Porter <adam@alphapapa.net> writes:

>> There were a few other places where you did (delq nil (mapcar ...)) that
>> might be replaced by this pattern.
>
> I confess that I've hardly ever used MAPCAN in any of my code--still,
> I'm not sure how that would help to avoid that pattern.  For example:
>
>   (let ((a '(1 nil 2 nil 3))
>         (b '(4 nil 5 nil 6)))
>     (mapcan #'identity (list a b)))
>
>   ;; (1 nil 2 nil 3 4 nil 5 nil 6)
>
> But I'm probably missing something.

The pattern I usually use is something like this:

(mapcan
 (lambda (n)
   (if (> n 0) (list n) nil))
 '(1 -2 5 0 -10 10))
;;=> (1 5 10)

The idea is that when using `mapcan', you provide the cons-cells of the
resulting list, instead of having `mapcar' generate them for you and
then immediately discard them via `delq'.

There should be a seq method for this pattern imo.

>> Not really, if you don't care about compiler warnings.  It just seems
>> like the kind of things that could cause problems at some later time,
>> when definitions are moved around.
>
> I do care about compiler warnings--I wrote makem.sh to catch such
> warnings in my projects before they reach users--but in this case,
> that code's not going anywhere, because it's already with
> bookmark-related code.

Feel free to do as you like (none of this is barring inclusion, if that
wasn't clear), that was just a suggestion.

>> Ah, the `t did confuse me momentarily, but in that case you can replace
>> the (guard ...) with (and 't (guard ...)).
>
> As much as I advocate using pcase and its powerful expressions, I
> think that would make this example harder to follow.  The pcase
> pattern is used to test an argument, and the string test is a separate
> concern.

But consider the saved indentation!

>>>> @@ -328,7 +328,7 @@ PROMPT is passed to `format-prompt', which see."
>>>>             n)
>>>>        (when current-track
>>>>          (cl-callf2 delete current-track tracks))
>>>> -    (setf n (length tracks))
>>>> +    (setf n (length tracks))   ;why the variable?
>>>
>>> Because the value is used elsewhere in the function.  Am I missing
>>> something?  (Anyway, as noted in the source, I did not write that
>>> function.)
>> Then I missed something, because I just saw the variable being
>> declared
>> in the let-head, set here and used once later.
>
> It's used in two places, and it would be undesirable to recalculate
> the list's length every time through the loop.

I see it now, sorry.

>>>> On the topic of the readme/manual, wouldn't it be better to have a
>>>> separate README file?  Then again, the manual is pretty short, and I
>>>> don't know if it is worth having it in the first place...
>>>
>>> As you said, this readme is currently trivial.  Were it larger,
>>> however--well, I have other packages with much larger readmes that are
>>> also converted to manuals.  There would not be much gained by
>>> separating into files.
>> I don't think that is a good practice.  A README for when you have
>> fetched the sources and want to figure out what is what, a manual for
>> when you have already installed a package and a package description for
>> when you are previewing a package using C-h P are three different
>> things.  One shouldn't cover all of it with the same file if you ask me,
>> since they all have different audiences with different levels of
>> interest.
>
> I don't know about that.  Especially for small packages with trivial
> documentation.  Maintaining documentation and commentaries, keeping
> them reasonably in sync, etc. is enough work without having them split
> into multiple files.  Having a README.org which is viewable at the
> package's repo also generate the manual is a relief to me.

My point is that there shouldn't be an overlap.  I think a README
shouldn't contain too much detail, but serve as a signpost (suitable
both for online and offline (!) reading): "This is brief summary of what
you have found, the source code is hosted here, you can find the
documentation there, my contact information somewhere else, etc.", while
the package description gives a high level overview that doesn't have to
updated unless the entire idea of the package changes, while the
documentation goes into the nitty-gritty details.

>>>> Also, your README includes this line
>>>>     :vc (:fetcher github :repo "alphapapa/listen.el")
>>>> which is malformed.
>>>
>>> I just tested that, and it works for me.
>> On Emacs 30?  That is not the code we merged...
>
> No, I'm using Emacs 29 with `vc-use-package'.  Its documentation seems
> to suggest that it uses the same format as that merged into Emacs 30,
> since it says that its features were merged into Emacs 30.
>
> Maybe `vc-use-package's documentation should be updated to reflect this?

Do you mean this: https://github.com/slotThe/vc-use-package?  I have no
involvement with that project, but I don't see where they mention the
"fetcher" notation you mention.

>>>> What you want is
>>>>     :vc (:url "https://github.com/alphapapa/listen.el")
>
> Ok, so no ".git" on the end (i.e. relying on the GitHub redirect)?

That doesn't matter (FWIW I didn't know either of the two was a redirect).

> And does this mean that none of the host-specific "fetchers" are
> available in Emacs 30?  (Which FTR is fine with me, as the URL should
> be enough, I'm just curious.)

No, the package-vc extension for use-package uses the same package
specifications as package-vc?

-- 
	Philip Kaludercic on peregrine

Re: [ELPA] New package: listen

[Adam Porter]

On 2/26/24 02:09, Philip Kaludercic wrote:

> The pattern I usually use is something like this:
> 
> (mapcan
>   (lambda (n)
>     (if (> n 0) (list n) nil))
>   '(1 -2 5 0 -10 10))
> ;;=> (1 5 10)
> 
> The idea is that when using `mapcan', you provide the cons-cells of the
> resulting list, instead of having `mapcar' generate them for you and
> then immediately discard them via `delq'.
> 
> There should be a seq method for this pattern imo.

Interesting, thanks.  It looks like the performance of that technique is 
slightly better than (delq nil (mapcar ...)), but if performance is a 
concern, then it looks like one should use a loop:

(benchmark-run-compiled 1000000
   (mapcan
    (lambda (n)
      (if (> n 0) (list n) nil))
    '(1 -2 5 0 -10 10)))
;; (0.21211651699999998 0 0.0)

(benchmark-run-compiled 1000000
   (delq nil
         (mapcar
          (lambda (n)
            (when (> n 0)
              n))
          '(1 -2 5 0 -10 10))))
;; (0.21806460800000002 0 0.0)

(benchmark-run-compiled 1000000
   (cl-loop for n in '(1 -2 5 0 -10 10)
            when (> n 0)
            collect n))
;; (0.052528039 0 0.0)

>>> Ah, the `t did confuse me momentarily, but in that case you can replace
>>> the (guard ...) with (and 't (guard ...)).
>>
>> As much as I advocate using pcase and its powerful expressions, I
>> think that would make this example harder to follow.  The pcase
>> pattern is used to test an argument, and the string test is a separate
>> concern.
> 
> But consider the saved indentation!

Ok, I will.  :)

> My point is that there shouldn't be an overlap.  I think a README
> shouldn't contain too much detail, but serve as a signpost (suitable
> both for online and offline (!) reading): "This is brief summary of what
> you have found, the source code is hosted here, you can find the
> documentation there, my contact information somewhere else, etc.", while
> the package description gives a high level overview that doesn't have to
> updated unless the entire idea of the package changes, while the
> documentation goes into the nitty-gritty details.

I understand what you're saying, but I think there is often value in 
having all of those categories of information in a single file, 
especially for smaller packages.  It's similar to the experience of 
being able to quickly scan through a man page compared to having to page 
through an Info manual.

>>>>> Also, your README includes this line
>>>>>      :vc (:fetcher github :repo "alphapapa/listen.el")
>>>>> which is malformed.
>>>>
>>>> I just tested that, and it works for me.
>>> On Emacs 30?  That is not the code we merged...
>>
>> No, I'm using Emacs 29 with `vc-use-package'.  Its documentation seems
>> to suggest that it uses the same format as that merged into Emacs 30,
>> since it says that its features were merged into Emacs 30.
>>
>> Maybe `vc-use-package's documentation should be updated to reflect this?
> 
> Do you mean this: https://github.com/slotThe/vc-use-package?  I have no
> involvement with that project, but I don't see where they mention the
> "fetcher" notation you mention.

I see it in its readme under "Usage".

>>>>> What you want is
>>>>>      :vc (:url "https://github.com/alphapapa/listen.el")
>>
>> Ok, so no ".git" on the end (i.e. relying on the GitHub redirect)?
> 
> That doesn't matter (FWIW I didn't know either of the two was a redirect).
> 
>> And does this mean that none of the host-specific "fetchers" are
>> available in Emacs 30?  (Which FTR is fine with me, as the URL should
>> be enough, I'm just curious.)
> 
> No, the package-vc extension for use-package uses the same package
> specifications as package-vc?

Sorry, I don't understand: "No, it doesn't mean that," or, "No, the 
fetchers are not available in Emacs 30"?

Re: [ELPA] New package: listen

[Philip Kaludercic]

Adam Porter <adam@alphapapa.net> writes:

> On 2/26/24 02:09, Philip Kaludercic wrote:
>
>> The pattern I usually use is something like this:
>> (mapcan
>>   (lambda (n)
>>     (if (> n 0) (list n) nil))
>>   '(1 -2 5 0 -10 10))
>> ;;=> (1 5 10)
>> The idea is that when using `mapcan', you provide the cons-cells of
>> the
>> resulting list, instead of having `mapcar' generate them for you and
>> then immediately discard them via `delq'.
>> There should be a seq method for this pattern imo.
>
> Interesting, thanks.  It looks like the performance of that technique
> is slightly better than (delq nil (mapcar ...)), but if performance is
> a concern, then it looks like one should use a loop:
>
> (benchmark-run-compiled 1000000
>   (mapcan
>    (lambda (n)
>      (if (> n 0) (list n) nil))
>    '(1 -2 5 0 -10 10)))
> ;; (0.21211651699999998 0 0.0)
>
> (benchmark-run-compiled 1000000
>   (delq nil
>         (mapcar
>          (lambda (n)
>            (when (> n 0)
>              n))
>          '(1 -2 5 0 -10 10))))
> ;; (0.21806460800000002 0 0.0)
>
> (benchmark-run-compiled 1000000
>   (cl-loop for n in '(1 -2 5 0 -10 10)
>            when (> n 0)
>            collect n))
> ;; (0.052528039 0 0.0)

True, invoking the lambda expression is probably the most expensive
part, but testing this on a 6-element list with a check that is
basically an opcode, restricts the universality of the claim we are
trying to make.

>> My point is that there shouldn't be an overlap.  I think a README
>> shouldn't contain too much detail, but serve as a signpost (suitable
>> both for online and offline (!) reading): "This is brief summary of what
>> you have found, the source code is hosted here, you can find the
>> documentation there, my contact information somewhere else, etc.", while
>> the package description gives a high level overview that doesn't have to
>> updated unless the entire idea of the package changes, while the
>> documentation goes into the nitty-gritty details.
>
> I understand what you're saying, but I think there is often value in
> having all of those categories of information in a single file,
> especially for smaller packages.  It's similar to the experience of
> being able to quickly scan through a man page compared to having to
> page through an Info manual.

I am not sure how the last point related to what you are saying, because
that sounds to me like an argument for not using the manual as the
package description.  And you already have a shorter package description
in the main file in your commentary section.  (It is possible to
instruct ELPA to use this for C-h P, instead of rendering the manual).

>>>>>> Also, your README includes this line
>>>>>>      :vc (:fetcher github :repo "alphapapa/listen.el")
>>>>>> which is malformed.
>>>>>
>>>>> I just tested that, and it works for me.
>>>> On Emacs 30?  That is not the code we merged...
>>>
>>> No, I'm using Emacs 29 with `vc-use-package'.  Its documentation seems
>>> to suggest that it uses the same format as that merged into Emacs 30,
>>> since it says that its features were merged into Emacs 30.
>>>
>>> Maybe `vc-use-package's documentation should be updated to reflect this?
>> Do you mean this: https://github.com/slotThe/vc-use-package?  I have
>> no
>> involvement with that project, but I don't see where they mention the
>> "fetcher" notation you mention.
>
> I see it in its readme under "Usage".

Oh, right.  Then I guess that should be noted somewhere to avoid
confusion.

>>>>>> What you want is
>>>>>>      :vc (:url "https://github.com/alphapapa/listen.el")
>>>
>>> Ok, so no ".git" on the end (i.e. relying on the GitHub redirect)?
>> That doesn't matter (FWIW I didn't know either of the two was a
>> redirect).
>> 
>>> And does this mean that none of the host-specific "fetchers" are
>>> available in Emacs 30?  (Which FTR is fine with me, as the URL should
>>> be enough, I'm just curious.)
>> No, the package-vc extension for use-package uses the same package
>> specifications as package-vc?
>
> Sorry, I don't understand: "No, it doesn't mean that," or, "No, the
> fetchers are not available in Emacs 30"?

There is no ":fetcher" keyword in Emacs 30.

To rephrase my above sentence: The version of use-package in Emacs 30
has a :vc keyword, that accepts mostly the same syntax for package
specifications as `package-vc-install' does (which in turn is the same
as what elpa-admin uses).

-- 
	Philip Kaludercic on peregrine

Re: [ELPA] New package: listen

[Adam Porter]

Ok, this thread has been interesting.  Is there anything that hinders 
the package from being added to ELPA now?

Thanks,
Adam

Re: [ELPA] New package: listen

[Philip Kaludercic]

Adam Porter <adam@alphapapa.net> writes:

> Ok, this thread has been interesting.  Is there anything that hinders
> the package from being added to ELPA now?

Don't think so, so I've pushed the package specification to elpa.git.

> Thanks,
> Adam
>

-- 
	Philip Kaludercic on peregrine

Re: [ELPA] New package: listen

[Adam Porter]

On 2/26/24 09:26, Philip Kaludercic wrote:
> Adam Porter <adam@alphapapa.net> writes:
> 
>> Ok, this thread has been interesting.  Is there anything that hinders
>> the package from being added to ELPA now?
> 
> Don't think so, so I've pushed the package specification to elpa.git.

Thank you.

--Adam

RE: [External] : Re: [ELPA] New package: listen

[Drew Adams]

Commenting only on the (minor) stylistic
convention you guys've discussed a bit -

> >> No, thanks.  I'm aware of this minor tradition,
> >> but I disagree with it.
> >
> > I wouldn't call it a minor tradition (e.g. in
> > Scheme the return value or `when' is unspecified.
> > CLtL2 says "If the value is relevant, then it
> > may be stylistically more appropriate to use and
> > or if."), and I am curious why you disagree with
> > it, but you are of course free to do as you want.
> 
> Well, Elisp isn't Scheme, and CLtL2 seems to
> say that it is exactly a minor tradition.  :)

It's not a tradition, minor or otherwise.
Never was, AFAIK.  It's one Lisp stylistic
approach/convention.  Dunno how widely it is,
was, or has been used, or in what contexts.

> I don't like it for a few reasons:

Very glad to see reasons!  Too much gets
discussed here without enough in the way of
reasons given, IMO.

> 1. Using AND puts the conditional expressions on the same level,
> visually and logically, as the value.  In contrast, using WHEN cleanly
> separates the condition(s) from the value.  IMO that's a significant
> advantage, as it makes the purpose of the code much clearer.

There's no separation of return value from
conditions in `when' - it simply always
returns `nil'.  It has only one condition,
the body has no bearing on the return value
(unless it `throw's etc.).

The visual separation of condition code and
body is irrelevant to the logical separation
of condition and return value.  The body's
irrelevant to the return value.

Your point that `and', `or', and `not' don't
separate the conditions from the result is
_precisely the point_.  These are Boolean
functions (with the particularity of ordered
evaluation).

With such a Boolean function - even a pure
one that returns only TRUE or FALSE - the
return value of any of the conditions _can
be_ the return value of the entire function.
Simple, elegant.

Of course, Lisp treats any non-nil value as
TRUE, so `and' and `or' (but not `not') can
return _any value whatsoever_.  Even _more_
simple and elegant.  That's Lisp, like it
or not.  (Most of us love it.)  

Think `member', as the classic example of
this leverage - exactly the same use case
as Lisp `and' and `or'.

> 2. WHEN's indentation also saves space, helping
> to avoid long lines or awkwardly wrapped ones.

OK.  So you prefer this:

(when (or xxxxxx
          yyyyyyyy
          zzzzzzzzz)
  foobar)

to this:

(or xxxxxx
    yyyyyyyy
    zzzzzzzzz)
    foobar)

saving 2 chars (or 3 for `and') in the foobar
indentation.

And I guess you prefer this:

(when
    foo
  bar)

to this?

(or
 foo
 bar)

wasting 1 char in the bar indentation.
___

Again, it's just a style/convention, and not
not one of the GNU Elisp coding conventions:

https://www.gnu.org/software/emacs/manual/html_node/elisp/Coding-Conventions.html

Let a hundred flowers bloom.  Different strokes
for different folks. ;-)

Re: [External] : Re: [ELPA] New package: listen

[Philip Kaludercic]

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

>> 2. WHEN's indentation also saves space, helping
>> to avoid long lines or awkwardly wrapped ones.
>
> OK.  So you prefer this:
>
> (when (or xxxxxx
>           yyyyyyyy
>           zzzzzzzzz)
>   foobar)
>
> to this:
>
> (or xxxxxx
>     yyyyyyyy
>     zzzzzzzzz)
>     foobar)

I suppose you mean (and ...)?

-- 
	Philip Kaludercic on peregrine

RE: [External] : Re: [ELPA] New package: listen

[Drew Adams]

> >> 2. WHEN's indentation also saves space, helping
> >> to avoid long lines or awkwardly wrapped ones.
> >
> > OK.  So you prefer this:
> >
> > (when (or xxxxxx
> >           yyyyyyyy
> >           zzzzzzzzz)
> >   foobar)
> >
> > to this:
> >
> > (or xxxxxx
> >     yyyyyyyy
> >     zzzzzzzzz)
> >     foobar)
> 
> I suppose you mean (and ...)?

Sorry; yes.

Re: [External] : Re: [ELPA] New package: listen

[Adam Porter]

On 2/26/24 11:17, Drew Adams wrote:

> There's no separation of return value from
> conditions in `when' - it simply always
> returns `nil'.  It has only one condition,
> the body has no bearing on the return value
> (unless it `throw's etc.).
> 
> The visual separation of condition code and
> body is irrelevant to the logical separation
> of condition and return value.  The body's
> irrelevant to the return value.

According to elisp.info:

> when is a Lisp macro in ‘subr.el’.
> 
> (when COND &rest BODY)
> 
> If COND yields non-nil, do BODY, else return nil.
> When COND yields non-nil, eval BODY forms sequentially and return
> value of last one, or nil if there are none.

According to CLHS:

> In a when form, if the test-form yields true, the forms are evaluated in order from left to right and the values returned by the forms are returned from the when form. Otherwise, if the test-form yields false, the forms are not evaluated, and the when form returns nil. 

RE: [External] : Re: [ELPA] New package: listen

[Drew Adams]

Yes, I misspoke.  If the condition is
true `when' evaluates the body and
returns what it returns.

(I do use `when' and `unless' correctly,
as well as `and' & compagnie.  I just
didn't describe them well. ;-))

Re: [External] : Re: [ELPA] New package: listen

[Stephen Berman]

On Mon, 26 Feb 2024 21:18:01 +0000 Drew Adams <drew.adams@oracle.com> wrote:

> Yes, I misspoke.  If the condition is
> true `when' evaluates the body and
> returns what it returns.
>
> (I do use `when' and `unless' correctly,
> as well as `and' & compagnie.  I just
> didn't describe them well. ;-))

Maybe you were thinking of `while'?

Steve Berman