> On Juli 12, 2014, 6:50 nachm., Alex Merry wrote: > > I'm generally in favour of this. Other than the issues noted below, I have > > two concerns: > > > > 1. 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). > > 2. 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.
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. 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. apidocs definitively need cleaning up, will do this as soon as possible. > On Juli 12, 2014, 6:50 nachm., Alex Merry wrote: > > src/lib/plugin/kpluginmetadata.h, lines 44-45 > > <https://git.reviewboard.kde.org/r/119079/diff/5/?file=289011#file289011line44> > > > > 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. 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 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. I.e. something like { ... "KPlugin" : { "Name" : "My new plugin", "Description": "blablabla", "Id": "foobarplugin" .... }, ... } > On Juli 12, 2014, 6:50 nachm., Alex Merry wrote: > > src/lib/plugin/kpluginmetadata.h, lines 52-53 > > <https://git.reviewboard.kde.org/r/119079/diff/5/?file=289011#file289011line52> > > > > Are these supposed to be in one-to-one correspondance? If so, that > > should be documented. I guess returning a list of structures with "1 person+ 1 or more email addresses" probably makes more sense. So something like this maybe: { "KPlugin": { ... "Authors": [ { "Name": "Abc Def", "Email": "a...@def.ghi" }, { "Name": "Foo Bar", "Email": [ "f...@bar.org", "foo...@xyz.com"] } ], .... }, } And also allowing this for the case with only one author: { "KPlugin": { ... "Authors": { "Name": "Abc Def", "Email": "a...@def.ghi" }, .... }, } - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62194 ----------------------------------------------------------- On Juli 8, 2014, 1:51 nachm., Alexander Richardson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119079/ > ----------------------------------------------------------- > > (Updated Juli 8, 2014, 1:51 nachm.) > > > Review request for KDE Frameworks. > > > Repository: kcoreaddons > > > Description > ------- > > This class simplifies reading the metadata from a qt plugin by providing > type safe accessor functions for the standard plugininfo keys that are > also used by the .desktop file based KPluginInfo > > KPluginMetaData: Read the translated value for name and description > > The "Name" and "Comment" fields of the metadata should be translated > since they will be shown to the user (e.g. in the plugin selection > dialog) > > Add a unit test for KPluginMetaData > > > Add KPluginMetaData::findPlugins() > > > Add a unit test for KPluginMetaData::findPlugins() > > > Introduce KPluginLoader::instantiatePlugins() and add a unit test > > This method allows easily instantiating all plugins in a given directory > > KPluginMetaData::pluginName() was changed to return the base name of the > plugin file if no plugin name was set in the JSON metadata > > > Diffs > ----- > > autotests/kpluginmetadatatest.cpp PRE-CREATION > src/lib/CMakeLists.txt 26eb5a1d4d56742a3395ba2645290bea15aee181 > src/lib/plugin/kpluginloader.h 0b7a53d3b879cec1d755b849d9d8c640d251a379 > src/lib/plugin/kpluginloader.cpp 9b3c5b6aec537b03b0d8341b33f6f4d7a76c8344 > src/lib/plugin/kpluginmetadata.h PRE-CREATION > src/lib/plugin/kpluginmetadata.cpp PRE-CREATION > autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd > autotests/kpluginloadertest.cpp c8225c02de3a64cae29d88954700dbc6f03ff1b0 > > Diff: https://git.reviewboard.kde.org/r/119079/diff/ > > > Testing > ------- > > Added a unit test > > Should easily allow loading all plugins from a given directory without > needing kbuildsycoca > > > Thanks, > > Alexander Richardson > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
