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

From: john muhl <[email-masked]>
Date: Fri, 11 Oct 2024 20:39:29 -0500

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.

>From 71deac5c51c90b24d886d26810d04a963d576c64 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 | 96 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 96 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..a4b576cbd1b 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,65 @@ 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