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