<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://git.reviewboard.kde.org/r/110833/">http://git.reviewboard.kde.org/r/110833/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 11th, 2013, 9:07 a.m. UTC, <b>Kevin Ottens</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 change go in. Note that I disagree with the aim though: KWidgets should *not* depend on KIconThemes. Now having KIconThemes not use KGlobalSettings is completely welcome (aim being to have KGlobalSettings in kde4support at some point).</pre>
</blockquote>
<p>On June 11th, 2013, 6:29 p.m. UTC, <b>Aleix Pol Gonzalez</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;">Well, then the KWidgets not depending on KIconItems contradicts this review request, right?</pre>
</blockquote>
<p>On June 12th, 2013, 5:58 a.m. UTC, <b>Kevin Ottens</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;">Well, not really. Here it's more about removing the dependency on KGlobalSettings than anything, and that is definitely welcome. Your commit log should probably reflect that, and likely you should remove the include kglobalsettings.h line.</pre>
</blockquote>
<p>On June 12th, 2013, 11:30 a.m. UTC, <b>Aleix Pol Gonzalez</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 can't do it because of:
KIconLoader::emitChange(KIconLoader::Group(arg));
If I removed that, we'd lose backwards compatibility. I could copy it over as:
QDBusMessage message = QDBusMessage::createSignal("/KIconLoader", "org.kde.KIconLoader", "iconChanged" );
message.setArguments(QList<QVariant>() << int(group));
QDBusConnection::sessionBus().send(message);
But then I'd worry about maintainability...</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;">You lost me here... I meant that KIconLoader shall not use KGlobalSettings (so should not include kglobalsettings.h), not the other way around (so it's fine for KGlobalSettings to call KIconLoader::emitChange).</pre>
<br />
<p>- Kevin</p>
<br />
<p>On June 10th, 2013, 11:07 a.m. UTC, Aleix Pol Gonzalez wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Aleix Pol Gonzalez.</div>
<p style="color: grey;"><i>Updated June 10, 2013, 11:07 a.m.</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;">As we discussed in the IRC meeting, I removed the KIconLoader dependency to KWidgets (so that we can make KWidgets depend on KIconLoader).
I'm unsure if it's the best approach, so I tried to do the simplest implementation I could. Don't hesitate to suggest changes.</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;">it builds</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>staging/kwidgets/src/utils/kglobalsettings.cpp <span style="color: grey">(53b648e)</span></li>
<li>staging/kwidgets/src/utils/kglobalsettings.h <span style="color: grey">(5fb1d9b)</span></li>
<li>staging/kiconthemes/src/kiconloader.cpp <span style="color: grey">(795ec93)</span></li>
<li>staging/kiconthemes/src/CMakeLists.txt <span style="color: grey">(82828dc)</span></li>
<li>staging/kiconthemes/src/kiconloader.h <span style="color: grey">(e4423eb)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110833/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>