<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/4962/">http://reviewboard.kde.org/r/4962/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 10th, 2010, 5:04 p.m., <b>Olivier Goffart</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This is a bit hackish (the hardcoded paths, and the QSystemTrayIcon stuff)
But if it works, i'm ok with it.</pre>
</blockquote>
<p>On August 10th, 2010, 5:30 p.m., <b>Markus Slopianka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Amateur question: Wouldn't is make more sense if a "wrapper service" was implemented that calls Growl instead of calling Growl directly from KNotify? That way hackish code wouldn't hurt.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This has been one of the options for me before I started on the patch, a DBus notification daemon that sends everything to Growl; however I rejected this option because it would be hard to ensure the daemon is installed correctly and running constantly. It would also be just another daemon for notifications to show correctly (would add up to 4). The daemon would have to run on all platforms with Growl installed, and from a normal KDE installation, without the packager having to worry about it and all.
All in all, I thought implementing GNTP / Growl in KNotify itself was more reliable and easier. I discussed this with Olivier before starting on the patch. Thanks for your comment, though ;-)
BTW: copying the response by e-mail from Olivier Goffart to the above comment here: "KNotify is already a wrapper service. We do not need more :-)"</pre>
<br />
<p>- Sjors</p>
<br />
<p>On August 10th, 2010, 3:05 p.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 and Olivier Goffart.</div>
<div>By Sjors Gielen.</div>
<p style="color: grey;"><i>Updated 2010-08-10 15:05:45</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; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch implements, as promised, Growl support in KNotify. Support is added in a separate class with static methods, called sometimes in NotifyByPopup. It currently uses Growl via QApplication::message(), because the GNTP protocol using which icon sending and callbacks are possible isn't stabilized in the Mac versions yet. This patch makes it possible to display the title and the message, in stripped form as with the DBus daemons after my last patch, in a Growl notification, with a (quite random) Qt information icon.
Before calling QApplication::message(), it checks if Growl is installed using QFile::exists(). There is still a TODO in the source regarding the Windows path to Growl, I will fill that in soon, but it's trivial. With this patch applied, I expect no changes on any machine where Growl is not installed, since then the older mechanism of KPassivePopup is used. (DBus notification daemons are still preferred over Growl.)
Once the newest version of Growl has stable GNTP, I will implement it in NotifyByPopupGrowl and send in another patch.</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; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested on Mac OS X. Nowhere else yet.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=212751">212751</a>
</div>
<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/CMakeLists.txt <span style="color: grey">(1155631)</span></li>
<li>/trunk/KDE/kdebase/runtime/knotify/notifybypopup.cpp <span style="color: grey">(1155631)</span></li>
<li>/trunk/KDE/kdebase/runtime/knotify/notifybypopupgrowl.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/KDE/kdebase/runtime/knotify/notifybypopupgrowl.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4962/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>