<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/4898/">http://reviewboard.kde.org/r/4898/</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 4th, 2010, 4:08 p.m., <b>Aurélien Gâteau</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;">The change looks OK, but I am wondering why you do not want to link with DBusMenuQt. Having (very) different code paths is potentially a source of trouble for maintenance.</pre>
</blockquote>
<p>On August 4th, 2010, 8:10 p.m., <b>Marc Mutz</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;">Having that kind of library as a hard dependency of KDE is just gross. Have a look at what other libraries kdelibs depends on, and how many of them are actually required. This library is used for a one-liner, literally. It shouldn't be required, esp. as there doesn't seem to be any release yet.
Symptomatic post: http://www.mail-archive.com/kde-buildsystem%40kde.org/msg05473.html
IIRC, there once was a policy to not require new (hard) dependencies in new releases. Seems to have slipped off the radar, though.
On top of that, on mobile, every dependency we don't have to have on the phone is a good dependency :)</pre>
</blockquote>
<p>On August 4th, 2010, 9 p.m., <b>Aurélien Gâteau</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;">I don't see how depending on such a library is "gross", can you explain a bit more?
Regarding the message you refer to, I agree the dependency could have been introduced in a much nicer way and I acknowledged this on my blog ( http://agateau.wordpress.com/2010/04/29/about-dbusmenu-holidays-and-other-hacks/ ). I thought that episode was behind us now.
Regarding the lack of releases, here is the url where you can download release tarballs: https://launchpad.net/libdbusmenu-qt/+download . There is a link to the Launchpad page in the FindDBusMenuQt.cmake file.
When DBusMenuQt is not used, the application is in charge of showing the menu, not the system tray. This is what I mean with a very different code path. I won't oppose against the change, though. I think the decision should be up to Marco Martin (can you add him to the "People" field?)</pre>
</blockquote>
<p>On August 5th, 2010, 4:29 a.m., <b>Marc Mutz</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;">> I don't see how depending on such a library is "gross", can you explain a bit more?
Sorry, it's not the depending on it per se. I find it gross that DBusMenuQt is required when it's only used in a single file in the notification subsystem of kdeui whereas not even OpenSSL is a required dependency, without which you can't even use half the world's web sites.
> Regarding the lack of releases, here is the url where you can download release tarballs:
> https://launchpad.net/libdbusmenu-qt/+download .
> There is a link to the Launchpad page in the FindDBusMenuQt.cmake file.
All that the macro_log_feature() in kdelibs/CMakeLists.txt told me was to git-clone something. I didn't occur to me to look in FindDBusMenuQt.cmake for a link to releases :)</pre>
</blockquote>
<p>On August 5th, 2010, 4:37 a.m., <b>Marc Mutz</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;">Oh, and what does libdbusmenu-qt do on Windows, WinCE, and OSA X?</pre>
</blockquote>
<p>On August 5th, 2010, 8:17 a.m., <b>Aurélien Gâteau</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;">> I find it gross that DBusMenuQt is required when it's only used in a single file in the notification subsystem of kdeui
One half of DBusMenuQt is used in kdeui, the other half is used in the systemtray applet (which you would need to patch as well).
> All that the macro_log_feature() in kdelibs/CMakeLists.txt told me was to git-clone something. I didn't occur to me to look in FindDBusMenuQt.cmake for a link to releases :)
The url is an argument to find_package_handle_standard_args(), not a comment. I assumed it would show up when you ran cmake and it failed to find the library. I may be wrong.
> Oh, and what does libdbusmenu-qt do on Windows, WinCE, and OS X?
Honestly I don't know, but I know the KDE Windows people have contributed patches to ensure it builds, so I guess it works.</pre>
</blockquote>
<p>On August 6th, 2010, 8:58 a.m., <b>Volker Krause</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;">It even builds on WinCE, but there is definitely no host using it, so it's just using rare resources without any benefit.</pre>
</blockquote>
<p>On August 6th, 2010, 3:23 p.m., <b>Aaron Seigo</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;">on WinCE / Windows Mobile and MacOS, there isn't a host. on MS Windows, there are people who use Plasma Desktop as their shell (i know because we get bug reports from them :P), and at that point there is a host on that platform (though the system tray needs some additional porting on the Windows platform as it doesn't yet support the native icons) ... probably more details than needed. :)</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;">regarding this: "there once was a policy to not require new (hard) dependencies in new releases. Seems to have slipped off the radar, though."
yes, i remember that as well (for kdelibs, anyways) and think we should get that back on track. unless there are any objections, i will add some text to this affect to the library policy page here so that we can make it part of our institutional memory, aka "the policies wiki":
http://techbase.kde.org/Policies/Library_Code_Policy</pre>
<br />
<p>- Aaron</p>
<br />
<p>On August 6th, 2010, 12:34 p.m., Marc Mutz 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, Plasma and Marco Martin.</div>
<div>By Marc Mutz.</div>
<p style="color: grey;"><i>Updated 2010-08-06 12:34:16</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;">Make DBusMenuQt optional.</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;">Compiles w/o DBusMenuQt present.</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>/branches/KDE/4.5/kdebase/workspace/CMakeLists.txt <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/ConfigureChecks.cmake <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/config-workspace.h.cmake <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusnotifieritem/CMakeLists.txt <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdebase/workspace/plasma/generic/dataengines/statusnotifieritem/statusnotifieritemsource.cpp <span style="color: grey">(1159836)</span></li>
<li>/branches/KDE/4.5/kdelibs/CMakeLists.txt <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/ConfigureChecks.cmake <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/config.h.cmake <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/kdeui/CMakeLists.txt <span style="color: grey">(1159039)</span></li>
<li>/branches/KDE/4.5/kdelibs/kdeui/notifications/kstatusnotifieritem.cpp <span style="color: grey">(1159039)</span></li>
</ul>
<p><a href="http://reviewboard.kde.org/r/4898/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>