Error handling in notifications-notify (was: Proposed changes to gnus-notifications.el)

[Basil L. Contovounesios]

Michael Albinus <address@hidden> writes:

> "Basil L. Contovounesios" <address@hidden> writes:
>
>> What is the best way to determine whether notifications are supported by
>> the current Emacs instance?  I skimmed through (info "(dbus) Top") and
>> (info "(elisp) Desktop Notifications"), as well as through the syms of
>> dbusbind.c, dbus.el, and notifications.el, but did not find any clear
>> answers.
>
> (featurep 'dbusbind)
>
> checks, whether Emacs is configured to use D-Bus. This does not
> necessarily tells you that notifications are supported, but it gives you
> a strong indication for this.

Thanks, I somehow missed this check littered all over the place.

> You could also wrap the call of notifications-notify by dbus-ignore-errors.
>
> A better check would be to ping notifications-service, but this should
> be done inside notifications.el, if needed.

Indeed, these two alternatives sound like the responsibility of
notifications.el, not its users.  The patch I proposed does not change
existing behaviour of gnus-notifications.el in this regard.

However, I'm not sure what the best thing to do in notifications-notify
is.  So far, it has returned nil for any type of error (unless
debug-on-error is non-nil), thanks to its use of with-demoted-errors.

Would it be too backward-incompatible to change this to
dbus-ignore-errors or pinging notifications-service, as you say?

If so, how's the following docfix instead?

>From 72e45056209f79d1a944ae7810b831cfc62f9d49 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <address@hidden>
Date: Sun, 21 Jul 2019 15:09:54 +0100
Subject: [PATCH 2/3] Document value of notifications-notify on failure

* doc/lispref/os.texi (Desktop Notifications):
* lisp/notifications.el (notifications-notify): Document return
value of notifications-notify on failure, e.g. when D-Bus support is
not available.
---
 doc/lispref/os.texi   | 25 ++++++++++++++-----------
 lisp/notifications.el | 11 +++++++----
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index fef954eb7a..bc01f3722f 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -2644,22 +2644,25 @@ Desktop Notifications
 Which parameters are accepted by the notification server can be
 checked via @code{notifications-get-capabilities}.
 
-This function returns a notification id, an integer, which can be used
-to manipulate the notification item with
-@code{notifications-close-notification} or the @code{:replaces-id}
-argument of another @code{notifications-notify} call.  For example:
+If successful, this function returns a notification ID, an integer,
+which can be used to manipulate the notification item with
+@code{notifications-close-notification} or as the @code{:replaces-id}
+argument of another @code{notifications-notify} call.  Otherwise, if
+sending the notification failed, this function returns @code{nil}.
+This may happen when D-Bus or notification support is not available,
+for example.
+
+Here is an example demonstrating the use of action callbacks:
 
 @example
 @group
 (defun my-on-action-function (id key)
   (message "Message %d, key \"%s\" pressed" id key))
-     @result{} my-on-action-function
 @end group
 
 @group
 (defun my-on-close-function (id reason)
   (message "Message %d, closed due to \"%s\"" id reason))
-     @result{} my-on-close-function
 @end group
 
 @group
@@ -2667,15 +2670,15 @@ Desktop Notifications
  :title "Title"
  :body "This is <b>important</b>."
  :actions '("Confirm" "I agree" "Refuse" "I disagree")
- :on-action 'my-on-action-function
- :on-close 'my-on-close-function)
+ :on-action #'my-on-action-function
+ :on-close #'my-on-close-function)
      @result{} 22
 @end group
 
 @group
-A message window opens on the desktop.  Press ``I agree''.
-     @result{} Message 22, key "Confirm" pressed
-        Message 22, closed due to "dismissed"
+A message window opens on the desktop.  Press @samp{I agree}.
+     @print{} Message 22, key "Confirm" pressed
+     @print{} Message 22, closed due to "dismissed"
 @end group
 @end example
 @end defun
diff --git a/lisp/notifications.el b/lisp/notifications.el
index 1d250e2d92..241f127b11 100644
--- a/lisp/notifications.el
+++ b/lisp/notifications.el
@@ -198,10 +198,13 @@ notifications-notify
 Which parameters are accepted by the notification server can be
 checked via `notifications-get-capabilities'.
 
-This function returns a notification id, an integer, which can be
-used to manipulate the notification item with
-`notifications-close-notification' or the `:replaces-id' argument
-of another `notifications-notify' call."
+If successful, this function returns a notification ID, an
+integer, which can be used to manipulate the notification item
+with `notifications-close-notification' or as the `:replaces-id'
+argument of another `notifications-notify' call.  Otherwise, if
+sending the notification failed, this function returns nil.  This
+may happen when D-Bus or notification support is not available,
+for example."
   (with-demoted-errors
     (let ((bus (or (plist-get params :bus) :session))
          (title (plist-get params :title))
-- 
2.20.1

>> Currently, gnus-notifications.el checks a) directly whether
>> notifications-notify is fboundp, and b) indirectly whether
>> notifications-notify returns nil.  Even without notification support,
>> (a) should always be true after loading the library, right?  So the
>> question is whether (b) is a sufficient condition.
>>
>> PS Should gnus-notifications.el be extended to support
>> w32-notification-notify?
>>
>> PPS Would it be possible/welcome to provide a common interface for both
>> notifications-notify and w32-notification-notify?  At the moment it
>> sounds like programmers need to choose between the two depending on
>> system-type.
>
> In ELPA, there are several notification libraries. alert.el seems to be
> the most advanced one, although it doesn't support w32-notification-notify.

Indeed, and it isn't in GNU ELPA either, though I seem to recall its
inclusion being discussed somewhere.

Thanks,

-- 
Basil