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

[john muhl]

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.

0001-Add-notifications-support-to-mpc-Bug-73538.patch
Description: Text Data