Review Request: Ensure notifications sent to DBus notification daemons are compatible with that daemon's capabilities

Fri Jul 9 13:31:58 BST 2010

> On 2010-07-09 12:16:31, Aurélien Gâteau wrote:
> > /trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp, line 452
> > <http://reviewboard.kde.org/r/4555/diff/1/?file=30591#file30591line452>
> >
> >     This line will cause a synchronous DBus call for every notification. I think it would be more efficient to cache the capabilities after the first call.
> >     
> >     Be sure to reset the cached values in slotServiceOwnerChanged()

Agreed. I was thinking about this, but decided to skip it until a later patch. Will implement it right now, because it's pretty simple.

> On 2010-07-09 12:16:31, Aurélien Gâteau wrote:
> > /trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp, line 489
> > <http://reviewboard.kde.org/r/4555/diff/1/?file=30591#file30591line489>
> >
> >     Using an xml parser here feels dangerous: you can't assume text will be well-formed xml.

That's true. The QXmlStreamReader copes pretty well with malformed XML, but the moment it sees something it doesn't expect, it stops and skips the rest. What do you think is the best option?
1. Just stop reading as soon as you get an error and skip the rest
2. Stop reading as soon as you get an error, and append everything from r.characterOffset(), i.e. with "foo bar" you get "foo bar"
3. When you get an error, throw away what you have and just send the text you received
Or something else?

- Sjors

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4555/#review6442
-----------------------------------------------------------

On 2010-07-09 11:55:56, Sjors Gielen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4555/
> -----------------------------------------------------------
> 
> (Updated 2010-07-09 11:55:56)
> 
> 
> Review request for kdelibs, Olivier Goffart and Aurélien Gâteau.
> 
> 
> Summary
> -------
> 
> KNotify supports sending notifications to a DBus notification daemon, but it currently does not make sure the notifications it sends are compatible with that specific daemon. More concretely,  notify-osd by design does not support HTML bodies or 'actions' (clickable buttons). It reports it does not support them, when you ask for its capabilities; however, knotify does not currently do that. KNotify then sends actions and HTML markup anyway, resulting in notify-osd displaying an ugly focus-stealing dialog, sometimes even with plaintext HTML entities in them.
> 
> This patch implements asking the notification daemon for capabilities. If a capability is not supported, its use is removed before sending the notification. There are no public API changes.
> 
> I would like to thank Aurélien Gateau for the capability retrieving parts of this patch (they were in another one of his patches, but those got rejected earlier because of the general goal of that patchset). If this patch is accepted, I would like it to be in the KDE 4.5 release, primarily because it makes KDE applications that use KNotify usable on Ubuntu again.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/runtime/knotify/notifybypopup.h 1147918 
>   /trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp 1147918 
> 
> Diff: http://reviewboard.kde.org/r/4555/diff
> 
> 
> Testing
> -------
> 
> The patch compiles and works on my Mac. I have quickly written a notification daemon of my own, and changing the capabilities directly changes the way the notifications are sent. I haven't tested what happens when a notification server does not implement the GetCapabilities call, but if that happens the code should assume that the daemon does not support anything and it will strip everything off.
> 
> The code itself is only tested on Mac OS X, running my own notification daemon. I expect it to work on Gnome with notify-osd, and if Plasma implements GetCapabilities correctly, behavior should not change there. Testing on those platforms is still required.
> 
> 
> Thanks,
> 
> Sjors
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20100709/63ed627a/attachment.htm>