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

[Stefan Monnier]

[ 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