Code review comment for lp:~charlesk/indicator-datetime/lp-1283142

Revision history for this message
Charles Kerr (charlesk) wrote :

>> is special casing notify-osd really the right thing to do there?
>> would it make more sense to use notify_get_server_caps() instead?
>
> I agree with Seb, we should check for the capabilities instead of the server's name.

That's reasonable. We could look to see if the notification server supports actions, which unity-notifications does and notify-osd does not. If actions are supported, then we would use the notify_notification_add_action() block of code in show_notification(). seb, larsu, what do you think of that?

> Why did you remove stop_alarm_sound() from the action callbacks?
> According to the spec[1], a server doesn't have to send CloseNotification
> in that case. notify-osd seems to to so, though.

The spec doesn't seem to say that; could you clarify?

Regardless, I don't mind re-adding the stop_alarm_sound() call to the
action callbacks as a safeguard.

> I think having sound on the desktop would make a lot of sense as well.
> Is it gone because there's no easy way to dismiss it without buttons on
> notifications?

Yes, that's the main issue. You don't want to have a looping sound that
you can't dismiss. :)

OTOH, maybe a one-time sound would be appropriate?

« Back to merge proposal