Re: [PATCH] Add notifications.el

[Michael Albinus]

Julien Danjou <address@hidden> writes:

>>> +(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")
>
> They are not mandatory, even if it's weird. It works fine here, even if
> the text is blank.

They are not mandatory functionally (there is no D-Bus error), but as
you say, the result is weird. I like Stefan's idea to make them mandatory
parameters; if somebody really wants to left them empty, she could pass "".

>>> + :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.
>
> Well, it is mentionned but since you did not saw that, I modified the help.

Grrr. I need new glasses!

>> 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 did not have any error, but well, I fixed it anyway.

Maybe you have disabled D-Bus error in one way or the other, I've got an
error. Did you run dbus-monitor in parallel, during tests?

>> 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'.
>
> That's clearly bonus, but added. :-)

It is more than just bonus. Otherwise, :actions could not be handled
properly.

Thanks for adding.

Best regards, Michael.