Re: [PATCH] Add notifications.el

[Michael Albinus <michael.albinus@gmx.de>]

Julien Danjou <julien@danjou.info> writes:

Hi,

> FYI I've signed the copyright assignment papers.

Looks nice. I have some comments, 'tho.

You should require 'cl when byte-compiling, due to the `case' macro.

> +(defun notifications-notify (&rest params)
> +  "Send notification via D-Bus using the Freedesktop notification protocol.
> +Various PARAMS can be set:
> +
> + :title          The notification title.
> + :body           The notification body text.

You might mention, that these are mandatory parameters. Check the effect of

(notifications-notify :urgency "low")

> + :app-icon       The notification icon.

If not given, you could apply a default value. A good value might be

(expand-file-name "images/icons/hicolor/scalable/apps/emacs.svg" data-directory)

> + :action         A list of actions.

I would call this parameter :actions, because you allow several actions
in that list. Or you allow multiple :action parameters, and concat them.

I would also be more descriptive, with examples.

And I would explain, that the action identifier "default" has a special
meaning. Try for effect

(notifications-notify :title "title" :body "body" :urgency "low" :action '("default" ""))

> + :timeout        The timeout time in milliseconds since the display
> +                 of the notification at which the notification should
> +                 automatically close.
> +                 If -1, the notification's expiration time is dependent
> +                 on the notification server's settings, and may vary for
> +                 the type of notification (default).
> +                 If 0, the notification never expires.

You might mention, that -1 is the default value.

> + :urgency        The urgency level.
> +                 Either `low', `normal' or `critical'.
> + :category       The type of notification this is.
> + :desktop-entry  This specifies the name of the desktop filename representing
> +                 the calling program.
> + :image-data     This is a raw data image format which describes the width,
> +                 height, rowstride, has alpha, bits per sample, channels and
> +                 image data respectively.
> + :sound-file     The path to a sound file to play when the notification pops up.
> + :suppress-sound Causes the server to suppress playing any sounds, if it has
> +                 that ability.
> + :x              Specifies the X location on the screen that the notification
> +                 should point to. The \"y\" hint must also be specified.
> + :y              Specifies the Y location on the screen that the notification
> +                 should point to. The \"x\" hint must also be specified.

All of them are hints. If none of them is given, you get a D-Bus error, try

(notifications-notify)

This is because you initialize your `hints' variable as '(:array). If no
hint is given, you cannot pass it to `dbus-call-method' as such. You
must pass '(:array :signature "{sv}") as empty hint.

I also propose to add the handling of the "NotificationClosed" and
"ActionInvoked" signals. This could be done by adding a callback
function to `notifications-notify'.

Best regards, Michael.