GNU bug report logs - #73538
[PATCH] Add notifications support to 'mpc'

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73538 in the body.
You can then email your comments to 73538 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add notifications support to 'mpc'
Date: Sat, 28 Sep 2024 17:48:21 -0500

[Message part 1 (text/plain, inline)]

Tags: patch

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

[0001-Add-notifications-support-to-mpc.patch (text/x-patch, attachment)]

Message #10 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: john muhl <jm <at> pub.pink>
Cc: 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Thu, 03 Oct 2024 10:57:23 -0400

[ 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

Message #13 received at submit <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
 text editors" <bug-gnu-emacs <at> gnu.org>
Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Thu, 03 Oct 2024 22:55:54 -0500

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs <at> 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.

Message #19 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: john muhl <jm <at> pub.pink>
Cc: 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Fri, 04 Oct 2024 09:42:37 -0400

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

Message #22 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Fri, 11 Oct 2024 20:39:29 -0500

[Message part 1 (text/plain, inline)]

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 <at> 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.

[0001-Add-notifications-support-to-mpc-Bug-73538.patch (text/x-patch, attachment)]

Message #25 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Sat, 12 Oct 2024 09:43:56 -0500

[Message part 1 (text/plain, inline)]

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

[0001-Add-notifications-support-to-mpc-Bug-73538.patch (text/x-patch, attachment)]

[Message part 3 (text/plain, inline)]

john muhl <jm <at> 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 <at> 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]...

Message #28 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: john muhl <jm <at> pub.pink>
Cc: John Wiegley <jwiegley <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Sun, 13 Oct 2024 14:11:58 +0200

john muhl <jm <at> 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.

Message #31 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: John Wiegley <jwiegley <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Sun, 13 Oct 2024 23:04:22 -0500

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

> john muhl <jm <at> 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.

Message #34 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: john muhl <jm <at> pub.pink>
Cc: John Wiegley <jwiegley <at> gmail.com>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Mon, 14 Oct 2024 12:53:01 +0200

john muhl <jm <at> 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.

Message #37 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: John Wiegley <jwiegley <at> gmail.com>, john muhl <jm <at> pub.pink>,
 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Mon, 14 Oct 2024 09:08:30 -0400

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

Message #40 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: John Wiegley <jwiegley <at> gmail.com>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: john muhl <jm <at> pub.pink>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Mon, 14 Oct 2024 07:56:42 -0700

>>>>> Michael Albinus <michael.albinus <at> 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

Message #43 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: John Wiegley <jwiegley <at> gmail.com>
Cc: john muhl <jm <at> pub.pink>, Stefan Monnier <monnier <at> iro.umontreal.ca>,
 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Tue, 15 Oct 2024 12:48:15 +0200

John Wiegley <jwiegley <at> 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.

Message #46 received at 73538 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: john muhl <jm <at> pub.pink>
Cc: 73538 <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Thu, 17 Oct 2024 12:43:45 -0400

> Here’s an updated patch. Should have all feedback incorporated and
> now with a NEWS entry.

Thanks, merged into `master` (after which I installed some further tweaks).

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

🙂

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

I tweaked the docstring a bit further.  Regarding the
`mpc-notifications-body/title`, the problem I can see with it is that
the "conditionality" is based on the fact that the overall result is
an empty string, so you can use something like

    (setopt mcp-notifications-body
            '("By %{Artist}" "By %{AlbumArtist}" "Unknown Artist"))

since the first will expand to the non-empty "By ".  🙁


        Stefan

Message #51 received at 73538-done <at> debbugs.gnu.org (full text, mbox):

From: john muhl <jm <at> pub.pink>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 73538-done <at> debbugs.gnu.org
Subject: Re: bug#73538: [PATCH] Add notifications support to 'mpc'
Date: Thu, 17 Oct 2024 19:26:03 -0500

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Here’s an updated patch. Should have all feedback incorporated and
>> now with a NEWS entry.
>
> Thanks, merged into `master` (after which I installed some further tweaks).

Thanks for your time. I’ll have another one for you to look at
soonish.

>>> 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.
>
> 🙂
>
>> 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.
>
> I tweaked the docstring a bit further.  Regarding the
> `mpc-notifications-body/title`, the problem I can see with it is that
> the "conditionality" is based on the fact that the overall result is
> an empty string, so you can use something like
>
>     (setopt mcp-notifications-body
>             '("By %{Artist}" "By %{AlbumArtist}" "Unknown Artist"))
>
> since the first will expand to the non-empty "By ".  🙁

Whoops. I was trying to mimic how other players do it and
overlooked that possibility since I don’t think I’ve ever seen
something like that. I guess there is still the chance to tweak it
in time for 31 in case the complaints come pouring in.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.