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

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

[john muhl]

[-- Attachment #1: Type: text/plain, Size: 86 bytes --]

Tags: patch

This adds support for displaying a notification when the song
changes.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-notifications-support-to-mpc.patch --]
[-- Type: text/x-patch, Size: 4469 bytes --]

From eb495c851dafcf883b0c1cbcd22b2dedc074f226 Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
Date: Sun, 15 Sep 2024 19:52:25 -0500
Subject: [PATCH] Add notifications support to 'mpc'

* lisp/mpc.el (mpc-notifications):
(mpc-notifications-function): New option.
(mpc-notifications-id): New variable.
(mpc-notifications-notify):
(mpc-cover-image-find):
(mpc-cover-image-p): New function.
---
 lisp/mpc.el | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/lisp/mpc.el b/lisp/mpc.el
index 768c70c2e3a..30e295d4810 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-"
@@ -2766,6 +2768,83 @@ mpc-drag-n-drop
       (t
        (error "Unsupported drag'n'drop gesture"))))))
 
+;;; Notifications ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
+
+(declare-function notifications-notify "notifications")
+(declare-function w32-notification-notify "notifications")
+(declare-function haiku-notifications-notify "notifications")
+(declare-function android-notifications-notify "notifications")
+
+(defcustom mpc-notifications-function 'mpc-notifications-notify
+  "Function called to display notification."
+  :version "31.1"
+  :type 'function)
+
+(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)))
+
+(defvar mpc-notifications-id nil)
+
+(defun mpc-notifications-notify ()
+  "Display a notification with information about the current song."
+  (interactive)
+  (mpc-status-refresh)
+  (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"))
+             (icon (or (mpc-cover-image-find (alist-get 'file mpc-status))
+                       notifications-application-icon)))
+    (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)))))))
+
+(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)))))
+
+(when mpc-notifications
+  (add-to-list 'mpc-status-callbacks
+               (cons 'file mpc-notifications-function)))
+
 ;;; Toplevel ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
 (defcustom mpc-frame-alist '((name . "MPC") (tool-bar-lines . 1)
-- 
2.46.2

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[ Sorry for the delay.  ]

> This adds support for displaying a notification when the song changes.

Oh, wow!  A patch for `mpc.el`!  Thanks!
Looks pretty good overall.  I'm not familiar with the `notifications`
library, OTOH, so I have a few questions, comments, and nitpicks below.

> +(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.

> +(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`.

> +(defvar mpc-notifications-id nil)

Comment: AFAICT this is an internal var so it should use a "--".

> +(defun mpc-notifications-notify ()
> +  "Display a notification with information about the current song."
> +  (interactive)
> +  (mpc-status-refresh)

Question: Why is it interactive?
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`).

> +  (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.

> +    (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?

> +(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`.


        Stefan

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

[john muhl]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> writes:

> [ Sorry for the delay.  ]

No sweat. It’s plenty fast by my reckoning.

>> This adds support for displaying a notification when the song changes.
>
> Oh, wow!  A patch for `mpc.el`!  Thanks!

I hope you won’t mind a few more.

> Looks pretty good overall.  I'm not familiar with the `notifications`
> library, OTOH, so I have a few questions, comments, and nitpicks below.

Thanks for looking. I’ll address the rest in a follow up patch but
in the meantime could you elaborate on this point:

>> +  (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.

What do you think the custom vars should look like? Would it be a
FORMAT-SPEC like “%{title} %5{duration}” or something else? How
would it handle the case where you want to try different tags like
“use Artist or AlbumArtist”?

If you don’t have something specific in mind I’ll see what I can
come up but any ideas would be helpful.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> This adds support for displaying a notification when the song changes.
>> Oh, wow!  A patch for `mpc.el`!  Thanks!
> I hope you won’t mind a few more.

🙂

>>> +  (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.
>
> What do you think the custom vars should look like?
> Would it be a FORMAT-SPEC like “%{title} %5{duration}” or something else?

I don't have a strong opinion on what the default format should look like.

> How would it handle the case where you want to try different tags like
> “use Artist or AlbumArtist”?

Ah... Hmm... good point.  We don't support that currently in
`mpc-format`.  Maybe we should add support for this (and take the
opportunity to actually document the % thingies understood by `mpc-format`).

Currently, the main format is `%WIDTH{NAME-POST}` where `-POST` is
optional, and it means to use the empty string if NAME is absent or
else use the concatenation of the content of NAME with the string POST.
Currently `-POST` is used only for `%{Disc--}`.

Maybe we should simply extend that to something like `%{NAME1|NAME2|...-POST}`?

Not sure whether it's worth adding support for a "default string", nor
what format we could use for that.  Maybe simply make it so that if
there's a | then the last element is not a tag name but a literal string
to use, so we could use:

    %{Artist|AlbumArtist|Unknown Artist}

Of course we could combine the two into something like
`%{NAME-POST+DEFAULT}` where DEFAULT would be recursively parsed as
a format string so we could do

    %{Artist+%{AlbumArtist+Unknown Artist}}

but I think this is getting too fancy for my taste (at that point, an
S-exp based format would start looking pretty attractive).


        Stefan

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

[john muhl]

[-- Attachment #1: Type: text/plain, Size: 5746 bytes --]

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" <bug-gnu-emacs@gnu.org> 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.



[-- Attachment #2: 0001-Add-notifications-support-to-mpc-Bug-73538.patch --]
[-- Type: text/x-patch, Size: 7941 bytes --]

From 71deac5c51c90b24d886d26810d04a963d576c64 Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
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'.
+
 \f
 * 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

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

[john muhl]

[-- Attachment #1: Type: text/plain, Size: 67 bytes --]

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


[-- Attachment #2: 0001-Add-notifications-support-to-mpc-Bug-73538.patch --]
[-- Type: text/x-patch, Size: 7932 bytes --]

From 2f8a66174e37ab076de82eda3759230ed1c3a58c Mon Sep 17 00:00:00 2001
From: john muhl <jm@pub.pink>
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'.
+
 \f
 * 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


[-- Attachment #3: Type: text/plain, Size: 6026 bytes --]



john muhl <jm@pub.pink> 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" <bug-gnu-emacs@gnu.org> 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]...

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

john muhl <jm@pub.pink> writes:

Hi John,

> Well there are erc-notifications-notify, gnus-notifications-notify
> and org-show-notifications but none of those seemed appropriate
> for reuse by mpc.

There is alert.el in MELPA, and some other packages which use
it. Perhaps it could help?

--8<---------------cut here---------------start------------->8---
/home/albinus/.emacs.d/elpa/archives/melpa/elpa-packages.eld
  96:  (alert :url "https://github.com/jwiegley/alert.git")
  97:  (alert-termux :url "https://github.com/gergelypolonkai/alert-termux.git")
  99:  (alert-toast :url "https://github.com/gkowzan/alert-toast.git")
3294:  (mu4e-alert :url "https://github.com/xzz53/mu4e-alert.git")
3600:  (org-alert :url "https://github.com/spegoraro/org-alert.git")
4312:  (rcirc-alert :url "https://github.com/csantosb/rcirc-alert.git")
4314:  (rcirc-alertify :url "https://github.com/fgallina/rcirc-alertify.git")
4954:  (term-alert :url "https://github.com/calliecameron/term-alert.git")
5310:  (weechat-alert :url "https://github.com/Kungi/weechat-alert.git")
--8<---------------cut here---------------end--------------->8---

Yes, I would like to see alert.el in GNU ELPA.

> 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?

Perhaps they could be changed to use alert.el instead? In its
Commentary, alert.el says:

--8<---------------cut here---------------start------------->8---
;; There are several builtin styles, and it is trivial to create new ones.
;; The builtins are:
;;
;;   fringe        - Changes the current frame's fringe background color
;;   mode-line     - Changes the current frame's mode-line background color
;;   gntp          - Uses gntp, it requires gntp.el (see https://github.com/tekai/gntp.el)
;;   growl         - Uses Growl on OS X, if growlnotify is on the PATH
;;   ignore        - Ignores the alert entirely
;;   libnotify     - Uses libnotify if notify-send is on the PATH
;;   log           - Logs the alert text to *Alerts*, with a timestamp
;;   message       - Uses the Emacs `message' facility
;;   momentary     - Uses the Emacs `momentary-string-display' facility
;;   notifications - Uses notifications library via D-Bus
;;   notifier      - Uses terminal-notifier on OS X, if it is on the PATH
;;   osx-notifier  - Native OSX notifier using AppleScript
;;   toaster       - Use the toast notification system
;;   x11           - Changes the urgency property of the window in the X Window System
;;   termux        - Use termux-notification from the Termux API
--8<---------------cut here---------------end--------------->8---

> Thanks.

Best regards, Michael.

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

[john muhl]

Michael Albinus <michael.albinus@gmx.de> writes:

> john muhl <jm@pub.pink> writes:
>
> Hi John,
>
>> Well there are erc-notifications-notify, gnus-notifications-notify
>> and org-show-notifications but none of those seemed appropriate
>> for reuse by mpc.
>
> There is alert.el in MELPA, and some other packages which use
> it. Perhaps it could help?
>
> --8<---------------cut here---------------start------------->8---
> /home/albinus/.emacs.d/elpa/archives/melpa/elpa-packages.eld
>   96:  (alert :url "https://github.com/jwiegley/alert.git")
>   97:  (alert-termux :url "https://github.com/gergelypolonkai/alert-termux.git")
>   99:  (alert-toast :url "https://github.com/gkowzan/alert-toast.git")
> 3294:  (mu4e-alert :url "https://github.com/xzz53/mu4e-alert.git")
> 3600:  (org-alert :url "https://github.com/spegoraro/org-alert.git")
> 4312:  (rcirc-alert :url "https://github.com/csantosb/rcirc-alert.git")
> 4314:  (rcirc-alertify :url "https://github.com/fgallina/rcirc-alertify.git")
> 4954:  (term-alert :url "https://github.com/calliecameron/term-alert.git")
> 5310:  (weechat-alert :url "https://github.com/Kungi/weechat-alert.git")
> --8<---------------cut here---------------end--------------->8---
>
> Yes, I would like to see alert.el in GNU ELPA.

It looks like it was proposed for inclusion a few times over the
years but never got past the copyright assignment stage. Maybe a
dedicated bug report for that would get more attention than this
one.

>> 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?
>
> Perhaps they could be changed to use alert.el instead?

I didn’t look closely (at alert or what erc, &c. are doing) but I
guess it could be made to work for all these cases. It’ll need
some patches to support Android, Haiku and Windows but that should
be fairly trivial I think.

Should we table this patch until alert is integrated with Emacs?

Thanks.

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

john muhl <jm@pub.pink> writes:

Hi John,

>> Yes, I would like to see alert.el in GNU ELPA.
>
> It looks like it was proposed for inclusion a few times over the
> years but never got past the copyright assignment stage. Maybe a
> dedicated bug report for that would get more attention than this
> one.
>
>>> 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?
>>
>> Perhaps they could be changed to use alert.el instead?
>
> I didn’t look closely (at alert or what erc, &c. are doing) but I
> guess it could be made to work for all these cases. It’ll need
> some patches to support Android, Haiku and Windows but that should
> be fairly trivial I think.
>
> Should we table this patch until alert is integrated with Emacs?

I'm definitely in favor of getting alert.el into GNU ELPA (or Emacs
core, even better) first. Would you like to push the needed actions?

If we have a dedicated bug report for this, it is also simpler to
understand the current status (completeness of copyright assignments etc
pp). I'd like to help you, but I cannot start it by myself. I'm in a
miserable health condition. It could always happen that I'm unavailable
w/o prior notice.

> Thanks.

Best regards, Michael.

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Well there are erc-notifications-notify, gnus-notifications-notify
>> and org-show-notifications but none of those seemed appropriate
>> for reuse by mpc.
>
> There is alert.el in MELPA, and some other packages which use
> it. Perhaps it could help?

Looking at the above existing uses of `notifications-notify` I get the
impression that we should fix `notifications-notify` so it dispatches to
the applicable backend, regardless if we want to integrate `alert.el`.


        Stefan

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

[John Wiegley]

>>>>> Michael Albinus <michael.albinus@gmx.de> writes:

> I'm definitely in favor of getting alert.el into GNU ELPA (or Emacs core,
> even better) first. Would you like to push the needed actions?

> If we have a dedicated bug report for this, it is also simpler to understand
> the current status (completeness of copyright assignments etc pp). I'd like
> to help you, but I cannot start it by myself. I'm in a miserable health
> condition. It could always happen that I'm unavailable w/o prior notice.

I’d be very happy to see alert.el make it into either ELPA or Core. Looking at
the ‘git shortlog‘, it does seem that there will be quite a number of
assignments needed.

I have created this issue on GitHub to help us track assignments, the same way
we did for use-package (which took a couple of years to finally resolve, btw):

  https://github.com/jwiegley/alert/issues/115

John

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

[Michael Albinus via Bug reports for GNU Emacs, the Swiss army knife of text editors]

John Wiegley <jwiegley@gmail.com> writes:

Hi John,

> I’d be very happy to see alert.el make it into either ELPA or Core. Looking at
> the ‘git shortlog‘, it does seem that there will be quite a number of
> assignments needed.
>
> I have created this issue on GitHub to help us track assignments, the same way
> we did for use-package (which took a couple of years to finally resolve, btw):
>
>   https://github.com/jwiegley/alert/issues/115

Thanks! I've checked this list. There are just two people left we need
to contact, see my comments there. Would you like to ask them?

> John

Best regards, Michael.