<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/123669/">https://git.reviewboard.kde.org/r/123669/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 7th, 2015, 7:24 a.m. UTC, <b>Alex Richardson</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;">Seems like this is duplicated in a few places already so I agree we should add it. But won't most users of the API want only a single plugin returned?
Maybe also add a function <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KPluginMetaData KPluginLoader::findPluginById(const QString& directory, const QString& pluginId)</code>?
Do we also need the function that returns a vector for a given ID?</p></pre>
</blockquote>
<p>On May 7th, 2015, 12:05 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">At least our changes in libplasma need a QVector<KPluginMetaData>. Otherwise, a list seems easy enough to check if something's found. Returning a single metadata won't be very useful for us at this point (but I see it making sense).</p></pre>
</blockquote>
<p>On May 7th, 2015, 10:47 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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Ow, also, and perhaps more importantly, multiple ids are technically perfectly valid (only the plugin name and directory are important here). I think that fact should be reflected in the API. Perhaps a word on ordering would be in place in the API docs, plugin locations are cascaded properly in code using it. The most local plugin is at the end of the list, system-widely installed ones at the beginning, so code that uses plugins.first() would not allow the user to override plugins installed for example into /usr/lib with a plugin with the same id and relative path installed into ~/.local). So an extra method returning the .last() plugin found might take away this caveat from the API. We'll still need the method returning a vector for libplasma, though (and I think it's a semantically useful addition).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">About adding another method to return the most-local plugin, I'm on the edge. If others think it's useful and we think the additional API (with its long-term maintainance implications) is worth it, I'm happy to add it as well. (Perhaps in a separate review.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Opinions welcome.</p></pre>
</blockquote>
<p>On May 8th, 2015, 6:18 p.m. UTC, <b>Alex Richardson</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;">The problem is that .last() won't work either. QCoreApplication::libraryPaths() has this order (http://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#_ZN16QCoreApplication12libraryPathsEv): $QT_INSTALLDIR/plugins, then the current executable directory and then QT_PLUGIN_PATH. This means that the lowest priority entry from QT_PLUGIN_PATH will be chosen in that case.</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;">So, can I ship it with the QVector<>? Patch has been lingering for a bit, would be nice to get it in (we have just written another potential user of this method).</p></pre>
<br />
<p>- Sebastian</p>
<br />
<p>On May 6th, 2015, 11:21 p.m. UTC, Sebastian Kügler 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, Alex Richardson and David Faure.</div>
<div>By Sebastian Kügler.</div>
<p style="color: grey;"><i>Updated May 6, 2015, 11:21 p.m.</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;">Add findPluginsById convenience API</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's a quite common case to load a plugin from an ID. This makes it
easy.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">CHANGELOG:New KPluginLoader::findPluginById() convenience API
REVIEW:123669</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 autotests, everything passes.</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/kpluginloadertest.cpp <span style="color: grey">(3ded0ebca2e0fd20e09bf6e4eca152d13ac11f46)</span></li>
<li>src/lib/plugin/kpluginloader.h <span style="color: grey">(124bfab7e207b17d3c8ab4d5a88321af476aad42)</span></li>
<li>src/lib/plugin/kpluginloader.cpp <span style="color: grey">(4310d6cd7792329511f12b28d7c68b0fdd013538)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123669/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>