<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120198/">https://git.reviewboard.kde.org/r/120198/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Oktober 21st, 2014, 11:19 nachm. CEST, <b>Sebastian Kügler</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've looked at the patch and tested it, it looks like a really useful improvement, and one more step of untangling the plugin / metadata situation from KService/KSycoca. I don't feel comfortable enough hitting ship it, but I don't see anything wrong with the patch, so +1.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've been working on indexing the plugins for faster lookups, and going forward, this patch here is one of the steps that is really useful to my approach. You can find that in kservice[sebas/plugintrader] in KService, I've merged this patch and https://git.reviewboard.kde.org/r/120199/diff/# (the bit that makes KPluginTrader::query use KCoreAddons' KPluginLoader::findPlugins into that branch, so I can benefit from its improvements already, in fact they seem to be needed to not have to introduce further complexity there, and keep the delta as small as possible. This indexing doesn't really work, that is I had it working, and benchmarks already looked promising, but I've been reworking some bits to use KPluginMetaData, and in that process, it broke. I'm working on it. :)</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I have been using this patch locally for quite a while now and didn't spot any issues so far. If no one objects, I'll commit it next week.</p></pre>
<br />
<p>- Alexander</p>
<br />
<p>On September 14th, 2014, 4:05 nachm. CEST, Alexander Richardson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Alexander Richardson.</div>
<p style="color: grey;"><i>Updated Sept. 14, 2014, 4:05 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kservice
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A series of 4 commits:</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KPluginInfo: use KPluginMetaData instead of a QVariantMap for storage</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This means that KPluginInfo can now simply reuse the QJsonObject
returned by QPluginLoader.metaData() (by storing it in a
KPluginMetaData object instead of having to convert the JSON to a
QVariantMap first.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Additionally this allows very efficient conversion between KPluginInfo
and KPluginMetaData.</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Add compatibility key names to KPluginInfo::property()</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KPluginInfo: Fix loading JSON metadata that only has compatibility keys</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This can be removed in KF6, but for now allows loading all both old
style as well as new style metadata</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kplugininfotest: also test objects constructed from JSON</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This tests both new style JSON as well as JSON using the old key names</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">All unit tests still work</p></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>autotests/CMakeLists.txt <span style="color: grey">(913e848ba5d1754ef7726f92604d1aaa398fa107)</span></li>
<li>autotests/kplugininfotest.cpp <span style="color: grey">(34f87028ce08f2db1e5f57edbc6f99a237bf90ac)</span></li>
<li>src/services/kplugininfo.h <span style="color: grey">(dea07e6e63baf2483afc4a6d43d0892efc485903)</span></li>
<li>src/services/kplugininfo.cpp <span style="color: grey">(50a6564edbbb1890c0b91badad69db967035231f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120198/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>