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