Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'

From: john muhl <[email-masked]>
Date: Sat, 12 Oct 2024 09:43:56 -0500

Here’s the same patch but with the excess whitespace removed.

>From 2f8a66174e37ab076de82eda3759230ed1c3a58c Mon Sep 17 00:00:00 2001
From: john muhl <[email-masked]>
Date: Sun, 15 Sep 2024 19:52:25 -0500
Subject: [PATCH] Add notifications support to 'mpc' (Bug#73538)

* lisp/mpc.el (mpc-notifications, mpc-notifications-title)
(mpc-notifications-body): New option.
(mpc--notifications-id): New variable.
(mpc-notifications-notify, mpc-cover-image-find)
(mpc-cover-image-p, mpc--notifications-format): New function.
(mpc-format): Use 'mpc-cover-find' and expand docstring to
include details about the FORMAT-SPEC.
(mpc-status-callbacks): Add file callback for
notifications.
---
 etc/NEWS    |  8 +++++
 lisp/mpc.el | 93 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 93 insertions(+), 8 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index c2919169bbf..6ffec916848 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -535,6 +535,14 @@ instead.
 *** A new shortcut to navigate to previous menu.
 The hardcoded "^" shortcut gets you back to the previous menu.
 
+** MPC
+
+---
+*** New user option 'mpc-notifications'.
+When non-nil MPC displays a desktop notification when the song changes.
+The notification’s title and body can be customized with
+'mpc-notifications-title' and 'mpc-notifications-body'.
+
 
 * New Modes and Packages in Emacs 31.1
 
diff --git a/lisp/mpc.el b/lisp/mpc.el
index 768c70c2e3a..a8789b19c42 100644
--- a/lisp/mpc.el
+++ b/lisp/mpc.el
@@ -95,6 +95,8 @@
   (require 'cl-lib)
   (require 'subr-x))
 
+(require 'notifications)
+
 (defgroup mpc ()
   "Client for the Music Player Daemon (mpd)."
   :prefix "mpc-"
@@ -460,6 +462,7 @@ mpc-status-callbacks
     (state  . mpc--faster-toggle-refresh) ;Only ffwd/rewind while play/pause.
     (volume . mpc-volume-refresh)
     (file   . mpc-songpointer-refresh)
+    (file   . mpc-notifications-notify)
     ;; The song pointer may need updating even if the file doesn't change,
     ;; if the same song appears multiple times in a row.
     (song   . mpc-songpointer-refresh)
@@ -958,6 +961,20 @@ mpc-file-local-copy
     ;;   aux)
     ))
 
+(defun mpc-cover-image-find (file)
+  "Find cover image for FILE."
+  (and-let* ((default-directory mpc-mpd-music-directory)
+             (dir (mpc-file-local-copy (file-name-directory file)))
+             (files (directory-files dir))
+             (cover (seq-find #'mpc-cover-image-p files))
+             ((expand-file-name cover dir)))))
+
+(defun mpc-cover-image-p (file)
+  "Check if FILE is a cover image."
+  (let ((covers '(".folder.png" "folder.png" "cover.jpg" "folder.jpg")))
+    (or (seq-find (lambda (cover) (string= file cover)) covers)
+        (and mpc-cover-image-re (string-match-p mpc-cover-image-re file)))))
+
 ;;; Formatter ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defcustom mpc-cover-image-re nil ; (rx (or ".jpg" ".jpeg" ".png") string-end)
@@ -992,7 +1009,15 @@ mpc-tempfiles-add
   (push file mpc-tempfiles))
 
 (defun mpc-format (format-spec info &optional hscroll)
-  "Format the INFO according to FORMAT-SPEC, inserting the result at point."
+  "Format the INFO according to FORMAT-SPEC, inserting the result at point.
+
+FORMAT-SPEC is a string of the format '%-WIDTH{NAME-POST}' where the first
+'-', WIDTH and -POST are optional.  % followed by the optional '-' means
+to right align the output.  WIDTH limits the output to the specified
+number of characters by replacing any further output with a horizontal
+ellipsis.  The optional -POST means to use the empty string if NAME is
+absent or else use the concatenation of the content of NAME with the
+string POST."
   (let* ((pos 0)
          (start (point))
          (col (if hscroll (- hscroll) 0))
@@ -1026,7 +1051,8 @@ mpc-format
                                                (substring time (match-end 0))
                                              time)))))
                     ('Cover
-                     (let ((dir (file-name-directory (cdr (assq 'file info)))))
+                     (let* ((file (alist-get 'file info))
+                            (dir (file-name-directory file)))
                        ;; (debug)
                        (setq pred
                              ;; We want the closure to capture the current
@@ -1037,12 +1063,7 @@ mpc-format
                                  (and (funcall oldpred info)
                                       (equal dir (file-name-directory
                                                   (cdr (assq 'file info))))))))
-                       (if-let* ((covers '(".folder.png" "folder.png" "cover.jpg" "folder.jpg"))
-                                 (cover (cl-loop for file in (directory-files (mpc-file-local-copy dir))
-                                                 if (or (member (downcase file) covers)
-                                                        (and mpc-cover-image-re
-                                                             (string-match mpc-cover-image-re file)))
-                                                 return (concat dir file)))
+                       (if-let* ((cover (mpc-cover-image-find file))
                                  (file (with-demoted-errors "MPC: %s"
                                          (mpc-file-local-copy cover))))
                            (let (image)
@@ -2766,6 +2787,62 @@ mpc-drag-n-drop
       (t
        (error "Unsupported drag'n'drop gesture"))))))
 
+;;; Notifications ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+(declare-function notifications-notify "notifications")
+
+(defcustom mpc-notifications nil
+  "Non-nil means to display notifications when the song changes."
+  :version "31.1"
+  :type 'boolean)
+
+(defcustom mpc-notifications-title
+  '("%{Title}" "Unknown Title")
+  "FORMAT-SPEC used in the notification title.
+
+The first element that returns a non-emtpy string is used.  The last
+element is a plain string to use as fallback for when none of the tags
+are found.  See `mpc-format' for the definition of FORMAT-SPEC."
+  :version "31.1"
+  :type '(repeat string))
+
+(defcustom mpc-notifications-body
+  '("%{Artist}" "%{AlbumArtist}" "Unknown Artist")
+  "FORMAT-SPEC used in the notification body.
+
+The first element that returns a non-emtpy string is used.  The last
+element is a plain string to use as fallback for when none of the tags
+are found.  See `mpc-format' for the definition of FORMAT-SPEC."
+  :version "31.1"
+  :type '(repeat string))
+
+(defvar mpc--notifications-id nil)
+
+(defun mpc--notifications-format (format-specs)
+  "Use FORMAT-SPECS to get string for use in notification."
+  (seq-some
+   (lambda (spec)
+     (let ((text (with-temp-buffer
+                   (mpc-format spec mpc-status)
+                   (buffer-string))))
+       (if (string= "" text) nil text)))
+   format-specs))
+
+(defun mpc-notifications-notify ()
+  "Display a notification with information about the current song."
+  (when-let ((mpc-notifications)
+             ((notifications-get-server-information))
+             ((string= "play" (alist-get 'state mpc-status)))
+             (title (mpc--notifications-format mpc-notifications-title))
+             (body (mpc--notifications-format mpc-notifications-body))
+             (icon (or (mpc-cover-image-find (alist-get 'file mpc-status))
+                       notifications-application-icon)))
+    (setq mpc--notifications-id
+          (notifications-notify :title title
+                                :body body
+                                :app-icon icon
+                                :replaces-id mpc--notifications-id))))
+
 ;;; Toplevel ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defcustom mpc-frame-alist '((name . "MPC") (tool-bar-lines . 1)
-- 
2.46.2

john muhl <[email-masked]> writes:

> Here’s an updated patch. Should have all feedback incorporated and
> now with a NEWS entry.
>
> Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <[email-masked]> writes:
>
>>> +(defcustom mpc-notifications-function 'mpc-notifications-notify
>>> +  "Function called to display notification."
>>> +  :version "31.1"
>>> +  :type 'function)
>>
>> Nitpick: I'd prefer it to use #' to quote `mpc-notifications-notify`.
>> Nitpick: The docstring should describe how it's called (how many args
>> are passed to it, what those args will be, and what it should return).
>> Question: Is it worth making this a `defcustom`?  I have a hard time
>> imagining someone using the Customize UI to set this.
>
> With the title and body now customizable this isn’t needed anymore.
>
>>> +(defcustom mpc-notifications nil
>>> +  "Non-nil means to display notifications when the song changes."
>>> +  :version "31.1"
>>> +  :type 'boolean
>>> +  :set (lambda (symbol value)
>>> +         (if-let ((cb (cons 'file mpc-notifications-function))
>>> +                  value)
>>> +             (add-to-list 'mpc-status-callbacks cb)
>>> +           (setq mpc-status-callbacks (delete cb mpc-status-callbacks)))
>>> +         (set-default-toplevel-value symbol value)))
>>
>> Comment: I'd assume that `file` changes are rare enough that it's OK to
>> always have a callback in there regardless if notifications are enabled and
>> instead test the value of `mpc-notifications` in that callback before
>> calling `mpc-notifications-function`.
>
> Ok. Moved it in with the other callbacks.
>
>>> +(defvar mpc-notifications-id nil)
>>
>> Comment: AFAICT this is an internal var so it should use a "--".
>
> Done.
>
>>> +(defun mpc-notifications-notify ()
>>> +  "Display a notification with information about the current song."
>>> +  (interactive)
>>> +  (mpc-status-refresh)
>>
>> Question: Why is it interactive?
>
> I have a global mpc-minor-mode that adds a key binding for
> notifications and it must have bled through.
>
>> Question: Why does it need `mpc-status-refresh`?  It seems odd to call
>> `mpc-status-refresh` from an "mpc-status-callback" (i.e. a function
>> called in response to an `mpc-status-refresh`).
>
> Looks like debugging debris.
>
>>> +  (when-let (((string= "play" (alist-get 'state mpc-status)))
>>> +             (title (or (alist-get 'Title mpc-status) "Unknown Title"))
>>> +             (body (or (alist-get 'Artist mpc-status)
>>> +                       (alist-get 'AlbumArtist mpc-status)
>>> +                       "Unknown Artist"))
>>
>> Comment: I think it would make sense to use `mpc-format` here with
>> Custom vars to specify what goes in the title and body.
>
> Instead of changing mpc-format I made the customizable variables
> take a list of specs where the first element to return something
> interesting is used and a plain string can be added for fallback:
>
>   (setopt mcp-notifications-body-specs
>           '("%{Artist}" "%{AlbumArtist}" "Unknown Artist"))
>
> I added your description of the FORMAT-SPEC to the mpc-format
> docstring too.
>
>>> +    (pcase system-type
>>> +      ("windows-nt"
>>> +       (w32-notification-notify :title title :body body :icon icon))
>>> +      ("haiku"
>>> +       (setq mpc-notifications-id
>>> +             (haiku-notifications-notify :title title
>>> +                                         :body body
>>> +                                         :app-icon icon
>>> +                                         :replaces-id mpc-notifications-id)))
>>> +      ("android"
>>> +       (setq mpc-notifications-id
>>> +             (android-notifications-notify :title title
>>> +                                           :body body
>>> +                                           :icon icon
>>> +                                           :replaces-id mpc-notifications-id))
>>> +       (android-notifications-notify :title title :body body :icon icon))
>>> +      (_ (when (notifications-get-server-information)
>>> +           (setq mpc-notifications-id
>>> +                 (notifications-notify :title title
>>> +                                       :body body
>>> +                                       :app-icon icon
>>> +                                       :replaces-id mpc-notifications-id)))))))
>>
>> Comment: Eww!
>> Question: Don't we have a function in Emacs which knows which
>> notification backend to use so we don't need to do this ugly dispatch
>> dance here?
>
> Well there are erc-notifications-notify, gnus-notifications-notify
> and org-show-notifications but none of those seemed appropriate
> for reuse by mpc.
>
> Rather than add a fourth implementation I removed support for
> everything except DBus. I’ll try to work up a patch for
> notifications.el that could be used in erc, gnus, org and mpc then
> we can revisit support for the rest. Does that sound OK or do you
> have another idea?
>
>>> +(defun mpc-cover-image-find (file)
>>> +  "Find cover image for FILE."
>>> +  (and-let* ((default-directory mpc-mpd-music-directory)
>>> +             (dir (file-name-directory file))
>>> +             (files (directory-files (mpc-file-local-copy dir)))
>>> +             (cover (seq-find #'mpc-cover-image-p files))
>>> +             ((expand-file-name cover dir)))))
>>> +
>>> +(defun mpc-cover-image-p (file)
>>> +  "Check if FILE is a cover image."
>>> +  (let ((covers '(".folder.png" "folder.png" "cover.jpg" "folder.jpg")))
>>> +    (or (seq-find (lambda (cover) (string= file cover)) covers)
>>> +        (and mpc-cover-image-re (string-match-p mpc-cover-image-re file)))))
>>
>> Comment: This should be consolidated with the `Cover` code in `mpc-format`.
>
> mpc-format now uses mpc-cover-image-find.
>
> Thanks.
>
>
> [2. text/x-patch; 0001-Add-notifications-support-to-mpc-Bug-73538.patch]...