Re: [PATCH] * elpa-packages (radio): New package

[Philip Kaludercic]

Stefan Kangas <stefankangas@gmail.com> writes:

> Roi Martin <jroi.martin@gmail.com> writes:
>
>> This change adds the radio package to NonGNU ELPA.
>>
>> Radio is a GNU Emacs package that enables users to listen to Internet
>> radio stations.  Its main focus is in simplicity.  It exposes the
>> minimum required interface to interact with a list of radio stations
>> and offloads media playing to an external program configured by the
>> user.
>>
>> The code is licensed under GPL-3.0 and the documentation under
>> FDL-1.3.
>>
>> By default, it depends on mpv, which is licensed under GPL-2.0 and
>> LGPL-2.1.  Although, users can configure a different media player
>> through the radio-command variable.
>>
>> More information:
>>
>> * Source code repository: https://github.com/jroimartin/radio
>> * Online manual: https://wip.jroi.dev/emacs/radio/index.html
>
> Stefan, Philip, any comments?

Here are my comments:

diff --git a/lisp/radio.el b/lisp/radio.el
index ea96cc3..b5b4b44 100644
--- a/lisp/radio.el
+++ b/lisp/radio.el
@@ -6,7 +6,7 @@
 ;; Maintainer: Roi Martin <jroi.martin@gmail.com>
 ;; URL: https://github.com/jroimartin/radio
 ;; Version: 0.1.3
-;; Package-Requires: ((emacs "29.1"))
+;; Package-Requires: ((emacs "28.1"))
 ;; Keywords: multimedia
 
 ;; This file is NOT part of GNU Emacs.
@@ -31,32 +31,34 @@
 
 ;;; Code:
 
-(defcustom radio-stations-alist nil
-  "List of radio stations.
+(defgroup radio ()
+  "Listen to Internet radio."
+  :group 'multimedia)
 
-Elements are of the form (NAME . URL).
+(defcustom radio-stations-alist nil ;wouldn't it be nice to populate the list 
with some stations?
+  "Alist of radio stations.
 
-NAME is the name of the radio station.
-URL  is the URL of the radio station."
-  :type '(alist :key-type string :value-type string)
-  :group 'radio)
+Elements are of the form (NAME . URL), where NAME is the name of the
+radio station and URL is the URL of the radio station."
+  :type '(alist :key-type string :value-type string))
 
-(defcustom radio-command "mpv --terminal=no --video=no"
+(defcustom radio-command "mpv --terminal=no --video=no" ;
   "Command used to play a radio station."
-  :type 'string
-  :group 'radio)
+  :type 'string)                       ;isn't it better to take a
+                                       ;list of strings?  perhaps
+                                       ;even with a `:url' to
+                                       ;indicate where the URL should
+                                       ;be inserted.
 
 (defvar radio--current-station nil
+  ;; perhaps you could store this information in the `process-plist'
+  ;; of `radio--current-proc' to avoid duplicating state and being
+  ;; able to replace the current station with a single "transaction".
   "Radio station currently being played.")
 
 (defvar radio--current-proc nil
   "Current media player process.")
 
-;; From tabulated-list.el
-(defvar tabulated-list-entries)
-(defvar tabulated-list-format)
-(defvar tabulated-list-sort-key)
-
 (defun radio--play (station)
   "Play radio station.
 
@@ -68,9 +70,8 @@ being played, it is stopped first."
         (cmd (split-string-shell-command radio-command))
         (program (car cmd))
         (program-args `(,@(cdr cmd) ,url))
-        (start-process-args `(,program nil ,program ,@program-args))
-        (proc (apply #'start-process start-process-args)))
-    (setq radio--current-proc proc)))
+        (start-process-args `(,program nil ,program ,@program-args)))
+    (setq radio--current-proc (apply #'start-process start-process-args))))
 
 ;;;###autoload
 (defun radio-stop ()
@@ -80,14 +81,14 @@ If no station is being played, calling this function has no
 effect."
   (interactive)
   (setq radio--current-station nil)
-  (when radio--current-proc
+  (when radio--current-proc            ;what if the process is already dead?
     (delete-process radio--current-proc))
   (setq radio--current-proc nil))
 
 (defun radio-list-stations--play ()
   "Play the selected radio station and refresh the station list."
   (interactive)
-  (when-let ((station (tabulated-list-get-id)))
+  (when-let* ((station (tabulated-list-get-id)))
     (radio--play station)
     (tabulated-list-revert)))
 
@@ -97,16 +98,14 @@ effect."
   (radio-stop)
   (tabulated-list-revert))
 
-(defun radio-list-stations--refresh ()
-  "Refresh the radio station list."
-  (setq tabulated-list-entries nil)
-  (dolist (station radio-stations-alist)
-    (let ((name (car station))
-         (url (cdr station))
-         (status (if (equal station radio--current-station) "▶" "")))
-      (push (list station (vector status name url))
-           tabulated-list-entries)))
-  (tabulated-list-init-header))
+(defun radio-list-stations--generate ()
+  "Generate the radio station list for `tabulated-list-mode'."
+  (mapcar
+   (lambda (station)
+     `(,station [,(if (equal station radio--current-station) "▶" "") ;perhaps 
you could also highlight the line with a face?
+                ,(car station)
+                ,(cdr station)]))
+   radio-stations-alist))
 
 (defvar-keymap radio-mode-map
   :doc "Keymap used by `radio-mode'."
@@ -119,18 +118,17 @@ effect."
                               ("Station" 30 t)
                               ("URL" 0 t)])
   (setq tabulated-list-sort-key '("Station" . nil))
-  (add-hook 'tabulated-list-revert-hook #'radio-list-stations--refresh nil t))
+  (setq tabulated-list-entries #'radio-list-stations--generate))
 
 ;;;###autoload
 (defun radio-list-stations ()
   "Display a list of all radio stations."
   (interactive)
-  (let ((buf (get-buffer-create "*Station List*")))
-    (with-current-buffer buf
-      (radio-mode)
-      (radio-list-stations--refresh)
-      (tabulated-list-print))
-    (pop-to-buffer-same-window buf)))
+  (with-current-buffer (get-buffer-create "*Station List*")
+    (radio-mode)
+    (radio-list-stations--refresh)
+    (tabulated-list-print)
+    (pop-to-buffer-same-window (current-buffer))))
 
 ;;;###autoload
 (defun radio (station-name)
@@ -139,11 +137,18 @@ effect."
 When called from Lisp, STATION-NAME must be the name of one of
 the stations defined in `radio-stations-alist'."
   (interactive (list (completing-read "Play radio station: "
-                                     (mapcar #'car radio-stations-alist)
+                                     radio-stations-alist ;the keys of an 
alist are used by default
                                      nil t)))
-  (if-let ((station (assoc station-name radio-stations-alist)))
+  ;; Or just:
+  ;;
+  ;; (radio--play
+  ;;  (or (assoc station-name radio-stations-alist)
+  ;;      (user-error "Unknown station `%s'" station-name)))
+  (if-let* ((station (assoc station-name radio-stations-alist)))
       (radio--play station)
-    (message "Unknown station `%s'" station-name)))
+    (user-error "Unknown station `%s'" station-name)))
+
+;; I would also suggest adding some indicator via `global-mode-string'.
 
 (provide 'radio)