----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review61635 -----------------------------------------------------------
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? autotests/kpluginmetadatatest.cpp <https://git.reviewboard.kde.org/r/119079/#comment42864> 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) src/lib/plugin/kpluginloader.h <https://git.reviewboard.kde.org/r/119079/#comment42866> 2 spaces before "will" src/lib/plugin/kpluginloader.h <https://git.reviewboard.kde.org/r/119079/#comment42865> @since 5.1 (in all 3 new methods) src/lib/plugin/kpluginloader.h <https://git.reviewboard.kde.org/r/119079/#comment42874> Qt more commonly uses QList for qobject pointers (and since they are pointers, they fit in the acceptable use cases for QList: item is no bigger than a pointer). src/lib/plugin/kpluginloader.h <https://git.reviewboard.kde.org/r/119079/#comment42867> nullptr -> Q_NULLPTR? Or are you sure that nullptr is ok with all our supported compilers? src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42868> Why a template and not just using std::function as the parameter type, like you did in the public API? src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42869> Why use findPlugin() when you already have a full path? (the whole point of findPlugin being to resolve relative paths) If this is just to check that it has a valid extension, QLibrary::isLibrary() would be enough. src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42882> Why the resolution to absoluteFilePath *again* when you already did it 2 lines before? src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42870> coding style: space before &, not after (especially since you did that everywhere else) src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42871> this will have to be adjusted/removed, if you agree that findPlugin isn't needed here. But since isValid returns true as soon as there's a filename, I indeed can't see how we could hit this line. src/lib/plugin/kpluginloader.cpp <https://git.reviewboard.kde.org/r/119079/#comment42872> I would use if + continue here, after removing the use of findPlugin. Overall I think this means loading the plugin only once (here) instead of twice (in findPlugin and here). src/lib/plugin/kpluginmetadata.h <https://git.reviewboard.kde.org/r/119079/#comment42876> @since 5.1 src/lib/plugin/kpluginmetadata.h <https://git.reviewboard.kde.org/r/119079/#comment42877> IIRC from very long ago, doxygen doesn't parse this style of docu, it's either "///" for a single line, or the / * * ... * ... * / style on multiple lines. I could be wrong, but this is worth checking. src/lib/plugin/kpluginmetadata.h <https://git.reviewboard.kde.org/r/119079/#comment42875> 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). src/lib/plugin/kpluginmetadata.cpp <https://git.reviewboard.kde.org/r/119079/#comment42878> 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. src/lib/plugin/kpluginmetadata.cpp <https://git.reviewboard.kde.org/r/119079/#comment42879> The use of [] on a non-const object can lead to insertion if not found. Prefer .value(s_metaData) instead. This issue doesn't occur in all the const getters further down, because the QJsonObject is const there. However I personally use .value() everywhere, for simplicity :-) src/lib/plugin/kpluginmetadata.cpp <https://git.reviewboard.kde.org/r/119079/#comment42880> The use of [] on a non-const object can lead to insertion if not found. Prefer .value(s_metaData) instead. This issue doesn't occur in all the const getters further down, because the QJsonObject is const there. However I personally use .value() everywhere, for simplicity :-) src/lib/plugin/kpluginmetadata.cpp <https://git.reviewboard.kde.org/r/119079/#comment42881> QJsonValue would be more readable than auto - David Faure On July 2, 2014, 7:46 p.m., Alexander Richardson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119079/ > ----------------------------------------------------------- > > (Updated July 2, 2014, 7:46 p.m.) > > > 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
