Re: [PATCH] Add notifications.el

[Julien Danjou]

On Mon, Jun 07 2010, Michael Albinus wrote:

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

Thanks, added.

>> +(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.

>> + :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)

Good idea, thanks. Added.

> 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" ""))

Enhanced in that way.

>> + :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.

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

> 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. :-)

Feel free to comment, I'm still new at this.

-- 
Julien Danjou
// ᐰ <address@hidden>   http://julien.danjou.info

pgpH1JV9lINoa.pgp
Description: PGP signature