From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Adam Porter Newsgroups: gmane.emacs.devel Subject: Re: [ELPA] New package: listen Date: Sun, 25 Feb 2024 07:14:59 -0600 Message-ID: <6e496679-63f0-4a30-aa6a-8402ff96a6a1@alphapapa.net> References: <51bcca65-2690-4f23-98bd-d929e63d7f94@alphapapa.net> <87il2cdb17.fsf@posteo.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21428"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Mozilla Thunderbird Cc: emacs-devel To: Philip Kaludercic Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Feb 25 14:15:26 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 1reELl-0005MK-HN for ged-emacs-devel@m.gmane-mx.org; Sun, 25 Feb 2024 14:15:25 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1reELZ-000815-Rn; Sun, 25 Feb 2024 08:15:13 -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 1reELX-0007xK-OY for emacs-devel@gnu.org; Sun, 25 Feb 2024 08:15:11 -0500 Original-Received: from black.elm.relay.mailchannels.net ([23.83.212.19]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1reELR-0001GG-1f for emacs-devel@gnu.org; Sun, 25 Feb 2024 08:15:11 -0500 X-Sender-Id: dreamhost|x-authsender|adam@alphapapa.net Original-Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id B4144829D6; Sun, 25 Feb 2024 13:15:01 +0000 (UTC) Original-Received: from pdx1-sub0-mail-a207.dreamhost.com (unknown [127.0.0.6]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id 52FA7827BC; Sun, 25 Feb 2024 13:15:01 +0000 (UTC) ARC-Seal: i=1; s=arc-2022; d=mailchannels.net; t=1708866901; a=rsa-sha256; cv=none; b=L22nS785WW4k5iUBbaf6U3NRKOIqCVew3467C03Qyjvne4Co95lAd9jhBTOV0T0nVMkSgO RJXqlmpe8TzFw0aQIrJj3OHMpko5K3Pm7BeduhAPLTlL9HM8V6quMxhgvbJHg8FgKDozVe P+VaTA85AgnbMZ/WoGP7pj7Oeilc4sP34kJdyqICQzw4F6lqjYKWeEtzsLGXXeJRNBXK46 wjUixePGYHBAPBQPAWgW1HphjreCV+wj1brcllPiyymp2ChWbwsCnIbp6LtquDKxcavql5 ykfc76pBrdSru0hb5ahYtQLlzety+3orpsNT4Hs7mUDsdS+xV6YxoPW5l27UJw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=mailchannels.net; s=arc-2022; t=1708866901; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5GO1ZZESHlj9kCzpTsYg6wvoft3gvshf15SADNrQm7A=; b=4gV3Jh0Gbk0r1FK44+mjq52w4VOI7iBHMtVct8pZ7mt1PRFTF3xgT5gOxKCjeg5BgAjaRG GABaVASXXXOuDCymLVoFy24/a2bTKfH8DokBNttLvW6PaNVQPB2QucWf6ehC4O9o4AsfCi G/piXm4Fz819q3d4TDijE5vmC47HMZ3rpJw46WgxXhUhNNpH4y7Vi5t8AJTN88EP3Zl6Us ZRr8NFVKFYztPlpSOxxS3QM84BkAC6wm/CUeXat2jeiQ6hf0WGQ7aJyFGOotDrcXKb1YfX /glP26Unts6eO0bZ4EsWz78o4iS0N8kcEt7CU5/mggEtvpSZuFLPUAORm6sqEw== ARC-Authentication-Results: i=1; rspamd-6bdc45795d-xsq8r; auth=pass smtp.auth=dreamhost smtp.mailfrom=adam@alphapapa.net X-Sender-Id: dreamhost|x-authsender|adam@alphapapa.net X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|adam@alphapapa.net X-MailChannels-Auth-Id: dreamhost X-Suffer-Turn: 56961c5c095d8005_1708866901617_3382320095 X-MC-Loop-Signature: 1708866901617:3207760740 X-MC-Ingress-Time: 1708866901617 Original-Received: from pdx1-sub0-mail-a207.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384) by 100.127.49.71 (trex/6.9.2); Sun, 25 Feb 2024 13:15:01 +0000 Original-Received: from [10.43.0.210] (unknown [193.56.116.15]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: adam@alphapapa.net) by pdx1-sub0-mail-a207.dreamhost.com (Postfix) with ESMTPSA id 4TjPP025rmz26; Sun, 25 Feb 2024 05:15:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=alphapapa.net; s=dreamhost; t=1708866901; bh=5GO1ZZESHlj9kCzpTsYg6wvoft3gvshf15SADNrQm7A=; h=Date:Subject:To:Cc:From:Content-Type:Content-Transfer-Encoding; b=tyBx3u9yw6mZH8sovqr9d4uczOg+ZPgVYwvry4lqy4mBC/fjSkfWaUF8n6cQDblG6 /E5HsQ7E8f8fhIsQnSjL3HSaK85fF1VM7U53bIvwbILcYQD2OiobKzZY7HEKKzwLcp uDvTB5V2krWKWU1tPddoQS6bEIZ0eaaX1Gv5dKQunmONQjpNRFm0g9wpb3cV/iY4VS z3rF1gty3j5XbJPbUchnbsRvTCY5ijrz1GiG6Nq7By6srANObd+k/zNxAcgUQGDICj 4ijLO3JfqPOyT4/4nL6t+3tgFUyoiFvvIXAG/lp/tSa8waK7YiePzes2Dqp4bbRHWy UK4nQsLe5yd0Q== Content-Language: en-US In-Reply-To: <87il2cdb17.fsf@posteo.net> Received-SPF: neutral client-ip=23.83.212.19; envelope-from=adam@alphapapa.net; helo=black.elm.relay.mailchannels.net X-Spam_score_int: 0 X-Spam_score: -0.0 X-Spam_bar: / X-Spam_report: (-0.0 / 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_NONE=-0.0001, RCVD_IN_MSPIKE_H4=-0.01, RCVD_IN_MSPIKE_WL=-0.01, RCVD_IN_VALIDITY_RPBL=1.31, SPF_HELO_NONE=0.001, SPF_NEUTRAL=0.779, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no 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:316517 Archived-At: Hi Philip, Thanks for your review. Comments to follow: On 2/25/24 05:43, Philip Kaludercic wrote: > 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. 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