<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/119079/">https://git.reviewboard.kde.org/r/119079/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Juli 12th, 2014, 6:50 nachm. CEST, <b>Alex Merry</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'm generally in favour of this. Other than the issues noted below, I have two concerns:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Have you looked at some users of plugins and determined that the particular methods you've added to KPluginLoader are ones that will be useful to them? For example, I see you can get metadata or instances, but not both at once, and not [KQ]PluginLoader objects (except by using the foreach() method, of course, but that's less convenient).</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">The apidocs for KPluginMetaData could do with cleaning up: they aren't consistent in capitalisation or punctuation. I suggest running kgenapidox on the repo and looking at what Doxygen produces.</li>
</ol></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 agree having both may be useful, I have to look more at how this is currently used inside applications. Another alternative could be to add an instantiate() method to the metadata objects so that you don't have to manually create a plugin loader.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looking at your other comments I think it makes sense to move away completely from the KPluginInfo key names and create a new structure which can then be generated by desktoptojson from the old desktop files.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">apidocs definitively need cleaning up, will do this as soon as possible.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Juli 12th, 2014, 6:50 nachm. CEST, <b>Alex Merry</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="https://git.reviewboard.kde.org/r/119079/diff/5/?file=289011#file289011line44" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/plugin/kpluginmetadata.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<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">44</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * The keys that are recognized are the same as the ones used by the legacy KDE4 .desktop</span></pre></td>
</tr>
<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">45</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * file based plugin metadata:</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do we also want to allow simpler forms of these keys (with "X-KDE-PluginInfo-" removed)? It seems to me that the namespacing is less useful when these are necessarily Qt plugins and almost certainly in an application/library-specific directory.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This was done to make sure the current plugins that use desktoptojson continue functioning. I agree there should be no need for the namespacing, and this could be fixed inside desktoptojson</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Either by dropping the suffixes or by having them all inside an object type key, which is probably better for future extensibility so that those key names can be used for some application specific metadata.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I.e. something like</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"KPlugin" : {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"Name" : "My new plugin",<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"Description": "blablabla",<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"Id": "foobarplugin"<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
....<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
},<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Juli 12th, 2014, 6:50 nachm. CEST, <b>Alex Merry</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="https://git.reviewboard.kde.org/r/119079/diff/5/?file=289011#file289011line52" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/plugin/kpluginmetadata.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<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">52</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * X-KDE-PluginInfo-Author | authors() | string / string array</span></pre></td>
</tr>
<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">53</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * X-KDE-PluginInfo-Email | emailAddresses() | string / string array</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are these supposed to be in one-to-one correspondance? If so, that should be documented.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess returning a list of structures with "1 person+ 1 or more email addresses" probably makes more sense.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So something like this maybe:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"KPlugin": {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"Authors": [ <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{ "Name": "Abc Def", "Email": "abc@def.ghi" },<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
{ "Name": "Foo Bar", "Email": [ "foo@bar.org", "foobar@xyz.com"] }<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
],<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
....<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
},<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And also allowing this for the case with only one author:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">{<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"KPlugin": {<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
...<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
"Authors": { "Name": "Abc Def", "Email": "abc@def.ghi" },<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
....<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
},<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
}</p></pre>
<br />
<p>- Alexander</p>
<br />
<p>On Juli 8th, 2014, 1:51 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 Juli 8, 2014, 1:51 nachm.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcoreaddons
</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;">This class simplifies reading the metadata from a qt plugin by providing<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
type safe accessor functions for the standard plugininfo keys that are<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
also used by the .desktop file based KPluginInfo</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KPluginMetaData: Read the translated value for name and description</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The "Name" and "Comment" fields of the metadata should be translated<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
since they will be shown to the user (e.g. in the plugin selection<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
dialog)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Add a unit test for KPluginMetaData</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Add KPluginMetaData::findPlugins()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Add a unit test for KPluginMetaData::findPlugins()</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Introduce KPluginLoader::instantiatePlugins() and add a unit test</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This method allows easily instantiating all plugins in a given directory</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KPluginMetaData::pluginName() was changed to return the base name of the<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
plugin file if no plugin name was set in the JSON metadata</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;">Added a unit test</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Should easily allow loading all plugins from a given directory without needing kbuildsycoca</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/kpluginmetadatatest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/CMakeLists.txt <span style="color: grey">(26eb5a1d4d56742a3395ba2645290bea15aee181)</span></li>
<li>src/lib/plugin/kpluginloader.h <span style="color: grey">(0b7a53d3b879cec1d755b849d9d8c640d251a379)</span></li>
<li>src/lib/plugin/kpluginloader.cpp <span style="color: grey">(9b3c5b6aec537b03b0d8341b33f6f4d7a76c8344)</span></li>
<li>src/lib/plugin/kpluginmetadata.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/lib/plugin/kpluginmetadata.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(75d12932b36fcfe4ae1d538176ef9f85f60f15dd)</span></li>
<li>autotests/kpluginloadertest.cpp <span style="color: grey">(c8225c02de3a64cae29d88954700dbc6f03ff1b0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119079/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>