<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://reviewboard.kde.org/r/4555/">http://reviewboard.kde.org/r/4555/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2010, 12:16 p.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4555/diff/1/?file=30591#file30591line452" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">QStringList NotifyByPopup::popupServerCapabilities() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">452</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">QDBusMessage</span> <span class="n">m</span> <span class="o">=</span> <span class="n">QDBusMessage</span><span class="o">::</span><span class="n">createMethodCall</span><span class="p">(</span> <span class="n">dbusServiceName</span><span class="p">,</span> <span class="n">dbusPath</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<pre>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()</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">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.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 9th, 2010, 12:16 p.m., <b>Aurélien Gâteau</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/4555/diff/1/?file=30591#file30591line489" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;">QStringList NotifyByPopup::popupServerCapabilities() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">489</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; white-space: pre-wrap; word-wrap: break-word;"> <span class="n">QXmlStreamReader</span> <span class="n">r</span><span class="p">(</span> <span class="s">"<elem>"</span> <span class="o">+</span> <span class="n">text</span> <span class="o">+</span> <span class="s">"</elem>"</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre>Using an xml parser here feels dangerous: you can't assume text will be well-formed xml.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em">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 "<b>foo</i> <u>bar</u>" you get "foo</i> <u>bar</u>"
3. When you get an error, throw away what you have and just send the text you received
Or something else?</pre>
<br />
<p>- Sjors</p>
<br />
<p>On July 9th, 2010, 11:55 a.m., Sjors Gielen wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Olivier Goffart and Aurélien Gâteau.</div>
<div>By Sjors Gielen.</div>
<p style="color: grey;"><i>Updated 2010-07-09 11:55:56</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/runtime/knotify/notifybypopup.h <span style="color: grey">(1147918)</span></li>
<li>/trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp <span style="color: grey">(1147918)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4555/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>