From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philip Kaludercic Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: listen Date: Sun, 25 Feb 2024 11:43:32 +0000 Message-ID: <87il2cdb17.fsf@posteo.net> References: <51bcca65-2690-4f23-98bd-d929e63d7f94@alphapapa.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="24870"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel To: Adam Porter Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Feb 25 12:44:37 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1reCvs-0006GW-Mr for ged-emacs-devel@m.gmane-mx.org; Sun, 25 Feb 2024 12:44:36 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1reCux-0005Pw-0f; Sun, 25 Feb 2024 06:43:40 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1reCuw-0005Po-D1 for emacs-devel@gnu.org; Sun, 25 Feb 2024 06:43:38 -0500 Original-Received: from mout01.posteo.de ([185.67.36.65]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1reCut-0000au-RB for emacs-devel@gnu.org; Sun, 25 Feb 2024 06:43:38 -0500 Original-Received: from submission (posteo.de [185.67.36.169]) by mout01.posteo.de (Postfix) with ESMTPS id 5BBD1240028 for ; Sun, 25 Feb 2024 12:43:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.net; s=2017; t=1708861413; bh=3+pGOMr3ZutRRaDAbNH8S1NHPt+3g2dEcyyYEG46bgk=; h=From:To:Cc:Subject:Autocrypt:Date:Message-ID:MIME-Version: Content-Type:From; b=NmpnD1ClanvUDxYEfnwhKPGBqO1NxZAtqvWG5ddhnb0POcTK0uKzQUuw3V/3prWYz LpJt9Q8h4ME4d8lTdZNX6Q1hrWXEs1Y2uI5mpiv6RPTjWNkHyyl+L7M5HVPDOUrpvS pCTlzT3m5hUnSz9pvotvgu3PRGrNizMy3mPMAQH+hYZWqeHtHzGESUHlNy6AmoRHn2 ZtGIF6ZW/7aE3RtgC1uI43CuIx4mRWAXAtf+LgsstN4vU9SsX7xKOm43oEtWaFeYJZ ji4dPKBcnqsRo1P1ROqrU9aGL93gmHZC3m58Ac2QhHyHA8XGpGDcvly+/SNAJfBheW uGzlKWtvExhDw== Original-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4TjMMS4R1mz6tsb; Sun, 25 Feb 2024 12:43:32 +0100 (CET) In-Reply-To: <51bcca65-2690-4f23-98bd-d929e63d7f94@alphapapa.net> (Adam Porter's message of "Sun, 25 Feb 2024 01:28:40 -0600") X-Hashcash: 1:20:240225:emacs-devel@gnu.org::O91ZaT7sP7kPNWl/:1frv X-Hashcash: 1:20:240225:adam@alphapapa.net::ELegX1WD+sODwtp9:8t+a Autocrypt: addr=philipk@posteo.net; keydata= mDMEZBBQQhYJKwYBBAHaRw8BAQdAHJuofBrfqFh12uQu0Yi7mrl525F28eTmwUDflFNmdui0QlBo aWxpcCBLYWx1ZGVyY2ljIChnZW5lcmF0ZWQgYnkgYXV0b2NyeXB0LmVsKSA8cGhpbGlwa0Bwb3N0 ZW8ubmV0PoiWBBMWCAA+FiEEDg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwMFCQHhM4AFCwkI BwIGFQoJCAsCBBYCAwECHgECF4AACgkQ8xYDWXahwulikAEA77hloUiSrXgFkUVJhlKBpLCHUjA0 mWZ9j9w5d08+jVwBAK6c4iGP7j+/PhbkxaEKa4V3MzIl7zJkcNNjHCXmvFcEuDgEZBBQQhIKKwYB BAGXVQEFAQEHQI5NLiLRjZy3OfSt1dhCmFyn+fN/QKELUYQetiaoe+MMAwEIB4h+BBgWCAAmFiEE Dg7HY17ghYlni8XN8xYDWXahwukFAmQQUEICGwwFCQHhM4AACgkQ8xYDWXahwukm+wEA8cml4JpK NeAu65rg+auKrPOP6TP/4YWRCTIvuYDm0joBALw98AMz7/qMHvSCeU/hw9PL6u6R2EScxtpKnWof z4oM Received-SPF: pass client-ip=185.67.36.65; envelope-from=philipk@posteo.net; helo=mout01.posteo.de X-Spam_score_int: -43 X-Spam_score: -4.4 X-Spam_bar: ---- X-Spam_report: (-4.4 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_MED=-2.3, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:316513 Archived-At: --=-=-= Content-Type: text/plain Adam Porter 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: --=-=-= Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit 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 @@ -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 --=-=-= Content-Type: text/plain > 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 --=-=-=--