<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/111686/">http://git.reviewboard.kde.org/r/111686/</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 25th, 2013, 4:33 p.m. UTC, <b>David Faure</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="http://git.reviewboard.kde.org/r/111686/diff/1/?file=173289#file173289line235" style="color: black; font-weight: bold; text-decoration: underline;">staging/kservice/src/plugin/kpluginloader.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="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KPluginFactory *KPluginLoader::factory()</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; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">235</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">args</span> <span class="o"><<</span> <span class="n">metaData</span><span class="p">().</span><span class="n">toVariantMap</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<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 relates to my comment in the other review. If what we have is a list with always just one parameter (which is a map), why not pass the map directly in setArgs, rather than a list of maps with one map in the list?
IOW: setArgs(QVariantMap).</pre>
</blockquote>
<p>On July 29th, 2013, 11:16 p.m. UTC, <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;">For this to work, we also need the macro to include the json file. That means that we have to introduce a new DECLARATION (temp-)macro to allow passing in a .json file. I was planning to put this ahead, and get this merged first, but as we need a coherent solution for the whole chain, a new version of the patch will add this macro as well, and then we can synchronize it also with the KPluginInfo ctor patch. I'm working on these changes right now, then I'll look into how the layout of the arguments passed around should look like.</pre>
</blockquote>
<p>On July 30th, 2013, 12:04 a.m. UTC, <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;">This is what we get from QPluginLoader::metaData() when baking in a simple json file:
QJsonObject({"IID": "org.kde.KPluginFactory","MetaData": {"Comment": "Date and time by timezone","Icon": "preferences-system-time","Name": "Date and Time","Type": "Service","X-KDE-Library": "plasma_engine_time","X-KDE-PluginInfo-Author": "Aaron Seigo","X-KDE-PluginInfo-Category": "Date and Time","X-KDE-PluginInfo-Depends": [],"X-KDE-PluginInfo-Email": "aseigo@kde.org","X-KDE-PluginInfo-EnabledByDefault": true,"X-KDE-PluginInfo-License": "LGPL","X-KDE-PluginInfo-Name": "time","X-KDE-PluginInfo-Version": "1.0","X-KDE-PluginInfo-Website": "http://plasma.kde.org/","X-KDE-ServiceTypes": ["Plasma/DataEngine"]},"className": "factory","debug": false,"version": 328192})
The ctor takes a QVariantList.
>From the other review:
> Not exactly, it returns a QJsonObject, which has a toVariantMap().
> Correct me if I'm wrong (I don't know anything about JSON), but I don't see where the "list of maps" comes from.
It comes from the type of the arg passed into the plugin. We want to keep this compatible, changing the QVariantList args to a map would mean porting all the plugins, i.e. nope.
This question then very much boils down to where to do the conversion from List to Map, i.e. where are we putting the metadata, assuming we want to keep all the metadata, and not just certain keys (which we'd still need to somehow put in a list). I've chosen to do the conversion (i.e. the args << QPluginLoader::metaData().toVariantMap() in the factory method, since that keeps the ctor, args() and setArgs() of KPluginLoader synchronous (i.e. no mixture of Map and List there).
As to the question "What happens if there's more than one "MetaData" map in there. We can take either the first, or the last one, taking the first seems sensible, since we know that's where Qt puts it.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Actually, this doesn't have to be one arg, we leave the QVariantList intact, and just attach the plugin's metadata more or less as-is to it, so KPluginInfo can find it.</pre>
<br />
<p>- Sebastian</p>
<br />
<p>On July 30th, 2013, 2:33 a.m. UTC, Sebastian Kügler 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 Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated July 30, 2013, 2:33 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;">Make K_EXPORT_PLUGIN work with Qt's new plugin system
This patch changes the K_EXPORT_MACRO and the class it generates to be compatible with Qt's new plugin / metadata system. It basically replaces the old macros around q_plugin_instance with the new ones, using Q_INTERFACES. There's also a setter for the args, which are used to pass metadata into the plugin.
Otherwise, this is the minimal change, to make old plugin factories work atop the new framework.
This change is source-compatible, but the right .moc file when this macro is used from the .cpp file.</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;">Loaded plugins using KService, KPluginLoader, QPluginLoader and Plasma::PluginLoader, all work as expected.</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/kservice/src/plugin/kexportplugin.h <span style="color: grey">(cc5d58b)</span></li>
<li>staging/kservice/src/plugin/kpluginfactory.h <span style="color: grey">(a5ea21b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111686/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>