Review Request 111686: KPluginFactory macros port

[Sebastian Kügler]

> On July 25, 2013, 4:33 p.m., David Faure wrote:
> > staging/kservice/src/plugin/kpluginloader.cpp, line 235
> > <http://git.reviewboard.kde.org/r/111686/diff/1/?file=173289#file173289line235>
> >
> >     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).
> 
> Sebastian Kügler wrote:
>     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.
> 
> Sebastian Kügler wrote:
>     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 at 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.

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.


- Sebastian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111686/#review36497
-----------------------------------------------------------


On July 30, 2013, 2:33 a.m., Sebastian Kügler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111686/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 2:33 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Description
> -------
> 
> 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.
> 
> 
> Diffs
> -----
> 
>   staging/kservice/src/plugin/kexportplugin.h cc5d58b 
>   staging/kservice/src/plugin/kpluginfactory.h a5ea21b 
> 
> Diff: http://git.reviewboard.kde.org/r/111686/diff/
> 
> 
> Testing
> -------
> 
> Loaded plugins using KService, KPluginLoader, QPluginLoader and Plasma::PluginLoader, all work as expected.
> 
> 
> Thanks,
> 
> Sebastian Kügler
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130730/cce5bbf9/attachment.html>