Re: [ELPA] New package: listen

[Philip Kaludercic <philipk@posteo.net>]

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