Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata

Sat Jul 5 15:17:30 UTC 2014

> On Juli 5, 2014, 11:33 vorm., David Faure wrote:
> > I like the idea of having a method to list or load the plugins from a given subdir of the plugin path.
> > 
> > The method that returns a QList<QObject *> could/should actually go into Qt at some point. The rest is tied to our specific metadata keys, so it can't.
> > 
> > I am a bit concerned by KPluginMetaData vs KPluginInfo, but I lack full overview on that, maybe Sebas can comment on that part?

Problem with KPluginInfo is that is tied to KConfig with its isPluginEnabled() and setPluginEnabled(), load(), save(), setConfig(), config() functions.

Also the Hidden= key no longer makes sense when the metaData is inside the file, just don't install it if you don't want it there.

> On Juli 5, 2014, 11:33 vorm., David Faure wrote:
> > autotests/kpluginmetadatatest.cpp, line 137
> > <https://git.reviewboard.kde.org/r/119079/diff/3/?file=286681#file286681line137>
> >
> >     won't $QT_PLUGIN_PATH still extend the search to other directories? (maybe it should be unset, in initTestCase, or even in a Q_CONSTRUCTOR_FUNCTION if Qt reads it and caches it from the qapp ctor)

Just calling setLibraryPaths() is enough: http://code.woboq.org/qt5/qtbase/src/corelib/kernel/qcoreapplication.cpp.html#2404

> On Juli 5, 2014, 11:33 vorm., David Faure wrote:
> > src/lib/plugin/kpluginloader.h, line 308
> > <https://git.reviewboard.kde.org/r/119079/diff/3/?file=286683#file286683line308>
> >
> >     nullptr -> Q_NULLPTR? Or are you sure that nullptr is ok with all our supported compilers?

Ah forgot that it was only introduced in GCC 4.6 and we want to support 4.5

> On Juli 5, 2014, 11:33 vorm., David Faure wrote:
> > src/lib/plugin/kpluginmetadata.h, lines 204-205
> > <https://git.reviewboard.kde.org/r/119079/diff/3/?file=286685#file286685line204>
> >
> >     Make member vars private.
> >     
> >     It's not like deriving from a value class makes any sense anyway, and this makes future changes possible (as long as the object size doesn't change).

I thought applications that have extra metadata keys could just extend this class to provide a getter for that.

class FooPluginMetaData : public KPluginMetaData {
public:
  using KPluginMetaData::KPluginMetaData;
  QString fooBar() {
    return m_metadata["X-Foo-BarInfo"];
  }
}

But I can make them private anyway if you think this is not a valid use-case

> On Juli 5, 2014, 11:33 vorm., David Faure wrote:
> > src/lib/plugin/kpluginmetadata.cpp, line 30
> > <https://git.reviewboard.kde.org/r/119079/diff/3/?file=286686#file286686line30>
> >
> >     Not static global objects in libraries, they slow down application startup and can lead to issues due to the undefined order of creation/destruction.
> >     
> >     Use static const char[] here, and convert to QString at usage time.
> >     
> >     I guess 16-bit readonly data would be better, but I'm not confident about how to write that portably.

Yeah thats right, I though QStringLiteral didn't cause a global constructor call, but clang -Wglobal-constructors proved me wrong.

Since these keys are used only once (except for MetaData), I guess I could just get rid of those global statics and insert a QStringLiteral in the function that needs the string

- Alexander

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119079/#review61635
-----------------------------------------------------------

On Juli 2, 2014, 9:46 nachm., Alexander Richardson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119079/
> -----------------------------------------------------------
> 
> (Updated Juli 2, 2014, 9:46 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/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd 
>   autotests/kpluginloadertest.cpp c8225c02de3a64cae29d88954700dbc6f03ff1b0 
>   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 
> 
> 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
> 
>

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