Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Aug. 24, 2014, 12:13 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kcoreaddons Description --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Aug. 24, 2014, 12:13 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kcoreaddons Description --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Aug. 24, 2014, 12:13 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kcoreaddons Description --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review63182 --- Ship it! Ship It! - David Faure On juil. 22, 2014, 11:17 matin, Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated juil. 22, 2014, 11:17 matin) Review request for KDE Frameworks. Repository: kcoreaddons Description --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62844 --- src/lib/plugin/kpluginloader.h https://git.reviewboard.kde.org/r/119079/#comment43577 is there a unittest for that? (directory being a relative path) - David Faure On July 20, 2014, 9:45 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 20, 2014, 9:45 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 - src/lib/plugin/kpluginmetadata.cpp PRE-CREATION src/lib/plugin/kpluginloader.cpp 9b3c5b6aec537b03b0d8341b33f6f4d7a76c8344 src/lib/plugin/kpluginmetadata.h PRE-CREATION src/lib/CMakeLists.txt 26eb5a1d4d56742a3395ba2645290bea15aee181 src/lib/plugin/kpluginloader.h 0b7a53d3b879cec1d755b849d9d8c640d251a379 autotests/kpluginmetadatatest.cpp PRE-CREATION autotests/kpluginloadertest.cpp c8225c02de3a64cae29d88954700dbc6f03ff1b0 autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 22, 2014, 1:17 nachm.) Review request for KDE Frameworks. Changes --- added unit test for forEachPlugin Repository: kcoreaddons Description (updated) --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs (updated) - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
On Juli 22, 2014, 9:24 vorm., David Faure wrote: src/lib/plugin/kpluginloader.h, line 316 https://git.reviewboard.kde.org/r/119079/diff/9/?file=291388#file291388line316 is there a unittest for that? (directory being a relative path) Yes there is for KPluginLoader::findPlugins(), but I have now added some more tests that should cover everything - Alexander --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62844 --- On Juli 22, 2014, 1:17 nachm., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 22, 2014, 1:17 nachm.) Review request for KDE Frameworks. Repository: kcoreaddons Description --- - Add new class KPluginMetaData This class simplifies reading the metadata from a qt plugin by providing type-safe accessor functions for known keys. It is meant as a replacment for KPluginInfo for applications that do not need all the features provided by KService. - Add functions for loading all plugins from a given directory KPluginLoader::findPlugins() can generally be used as a replacement for KServiceTypeTrader::self()-query(...). - Adapt jsonplugin metadata to the format used by KPluginMetaData - Add a unit test for KPluginLoader::instantiatePlugins() - Add a unit test for KPluginMetaData - Improve KPluginLoader unit test This adds a test for KPluginLoader::forEachPlugin() and also adds a test using a relative path for KPluginLoader::instantiatePlugins() Diffs - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json PRE-CREATION 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62731 --- src/lib/plugin/kpluginloader.h https://git.reviewboard.kde.org/r/119079/#comment43481 Even that isn't technically guaranteed - QLibrary::isLibrary() just looks at the extension. autotests/kpluginloadertest.cpp https://git.reviewboard.kde.org/r/119079/#comment43482 Just noticed that none of these tests check that your methods ever load more than one plugin :-) - Alex Merry On July 19, 2014, 3:41 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 19, 2014, 3:41 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/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 20, 2014, 11:45 nachm.) Review request for KDE Frameworks. Changes --- improved unit test 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 (updated) - src/lib/plugin/kpluginmetadata.cpp PRE-CREATION src/lib/plugin/kpluginloader.cpp 9b3c5b6aec537b03b0d8341b33f6f4d7a76c8344 src/lib/plugin/kpluginmetadata.h PRE-CREATION src/lib/CMakeLists.txt 26eb5a1d4d56742a3395ba2645290bea15aee181 src/lib/plugin/kpluginloader.h 0b7a53d3b879cec1d755b849d9d8c640d251a379 autotests/kpluginmetadatatest.cpp PRE-CREATION autotests/kpluginloadertest.cpp c8225c02de3a64cae29d88954700dbc6f03ff1b0 autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df autotests/jsonplugin2.h PRE-CREATION autotests/jsonplugin2.cpp PRE-CREATION autotests/jsonplugin2.json 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62680 --- src/lib/plugin/kpluginloader.h https://git.reviewboard.kde.org/r/119079/#comment43448 What about plugins that do not have metadata? src/lib/plugin/kpluginmetadata.h https://git.reviewboard.kde.org/r/119079/#comment43449 If you don't put at least minimal documentation in, Doxygen will warn about undocumented methods. src/lib/plugin/kpluginmetadata.cpp https://git.reviewboard.kde.org/r/119079/#comment43450 I think this would be clearer as passing QFileInfo an empty string gives the CWD, which is not what we want. And put the comment one line up, so it applies to the if() statement, rather than the assignment. I see you haven't changed the KPluginLoader methods - have you looked to see if they are the ones that are likely to be useful? - Alex Merry On July 18, 2014, 1:34 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 18, 2014, 1:34 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/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
On Juli 19, 2014, 11:27 vorm., Alexander Richardson wrote: I see you haven't changed the KPluginLoader methods - have you looked to see if they are the ones that are likely to be useful? I looked at KDevelop and Kate, and it seems there the version return a list of KPluginMetaData objects is sufficient. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62680 --- On Juli 18, 2014, 3:34 nachm., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 18, 2014, 3:34 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/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 19, 2014, 3:15 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 (updated) - src/lib/plugin/kpluginmetadata.cpp PRE-CREATION 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 autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/#review62699 --- Ship it! One more thing then I, at least, am happy for it to go in. src/lib/plugin/kpluginloader.h https://git.reviewboard.kde.org/r/119079/#comment43454 I think it's worth explicitly saying that the plugins will not necessarily have JSON metadata, and in fact are not even necessarily loadable via QPluginLoader (although they will generally be loadable by QLibrary). - Alex Merry On July 19, 2014, 1:15 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 19, 2014, 1:15 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 - src/lib/plugin/kpluginmetadata.cpp PRE-CREATION 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 autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 19, 2014, 5:41 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 (updated) - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 18, 2014, 3:34 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 (updated) - autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd autotests/jsonplugin.json d86fad49e5d074762d70282b3ace4bf3e6db58df 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- 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 (updated) - 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- 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 QListQObject * 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 :-)
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
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 QListQObject * 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated Juli 5, 2014, 5:20 nachm.) Review request for KDE Frameworks. Changes --- Fixed issues (except for the global constructor one) 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 (updated) - 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
On July 5, 2014, 9:33 a.m., 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).
Alexander Richardson wrote:
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
Well, you can have a public or protected method instead, like
QString value(const QString value) { return m_metadata.value(value); }
- Alex
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119079/#review61635
---
On July 5, 2014, 3:20 p.m., Alexander Richardson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119079/
---
(Updated July 5, 2014, 3:20 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
On July 5, 2014, 9:33 a.m., 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).
Alexander Richardson wrote:
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
Alex Merry wrote:
Well, you can have a public or protected method instead, like
QString value(const QString value) { return m_metadata.value(value); }
Deriving from KPluginMetaData just doesn't make sense. How would apps do that,
when the API from KPluginLoader returns them a QVectorKPluginMetaData in the
first place? They could copy each item into something else, but then better use
composition than inheritance for that something else, to avoid any risk of
slicing (= copying from derived to base and losing the derived stuff).
This is the same reason why inheriting from QString or any other value class is
a very bad idea (TM).
Alex's suggestion sounds good, in any case (with a const before the '{') --
as long as the docu doesn't mention inheriting from KPluginMetaData, of course
:)
On July 5, 2014, 9:33 a.m., 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.
Alexander Richardson wrote:
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
Right.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119079/#review61635
---
On July 5, 2014, 3:20 p.m., Alexander Richardson wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119079/
---
(Updated July 5, 2014, 3:20 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 2, 2014, 9: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 (updated) - 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
Re: Review Request 119079: Add utility function for loading all plugins from a given dir + easy accessor for metadata
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119079/ --- (Updated July 2, 2014, 12:25 a.m.) Review request for KDE Frameworks. Summary (updated) - Add utility function for loading all plugins from a given dir + easy accessor for metadata Repository: kcoreaddons Description (updated) --- 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 (updated) - src/lib/plugin/kpluginmetadata.cpp PRE-CREATION src/lib/plugin/kpluginmetadata.h PRE-CREATION src/lib/plugin/kpluginloader.cpp 9b3c5b6aec537b03b0d8341b33f6f4d7a76c8344 src/lib/plugin/kpluginloader.h 0b7a53d3b879cec1d755b849d9d8c640d251a379 src/lib/CMakeLists.txt 26eb5a1d4d56742a3395ba2645290bea15aee181 autotests/kpluginmetadatatest.cpp PRE-CREATION autotests/kpluginloadertest.cpp c8225c02de3a64cae29d88954700dbc6f03ff1b0 autotests/CMakeLists.txt 75d12932b36fcfe4ae1d538176ef9f85f60f15dd 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
