Re: [ELPA] New package: listen

[Philip Kaludercic <philipk@posteo.net>]

[-- 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