D28460: Add KCModuleStateProbe as base class for plugin
bport updated this revision to Diff 79780. bport added a comment. Rename state probe to data (because can be useful for more than only state probe) REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28460?vs=79772=79780 REVISION DETAIL https://phabricator.kde.org/D28460 AFFECTED FILES src/CMakeLists.txt src/kcmoduledata.cpp src/kcmoduledata.h src/kcmoduleloader.cpp src/kcmoduleloader.h To: bport, #plasma, ervin Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
bport marked 10 inline comments as done. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28460 To: bport, #plasma, ervin Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
bport updated this revision to Diff 79772. bport added a comment. ervin feedback REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28460?vs=79496=79772 REVISION DETAIL https://phabricator.kde.org/D28460 AFFECTED FILES src/CMakeLists.txt src/kcmoduleloader.cpp src/kcmoduleloader.h src/kcmodulestateprobe.cpp src/kcmodulestateprobe.h To: bport, #plasma, ervin Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> kcmoduleloader.cpp:161
> +if (!mod.service() || mod.service()->noDisplay() ||
> mod.library().isEmpty())
> +{
> +return true;
Curly brace should be on the previous line
> kcmoduleloader.cpp:167
> +args2.reserve(args.count());
> +for (const QString : args) {
> +args2 << arg;
Wouldn't it be better to initialize args2 with arg iterators?
i.e. `QVariantList args2(arg.cbegin(), arg.cend());`
> kcmodulestateprobe.cpp:47
> +
> +KCModuleStateProbe::~KCModuleStateProbe() {
> +delete d;
Curly brace on the next line
> kcmodulestateprobe.cpp:55
> +
> +void KCModuleStateProbe::registerSkeleton(KCoreConfigSkeleton *skeleton) {
> +if (!skeleton || d->_skeletons.contains(skeleton)) {
Curly brace on the next line
> kcmodulestateprobe.h:44
> +
> +virtual void virtual_hook(int id, void *data);
> +
Should be protected not public
> broulik wrote in kcmodulestateprobe.h:39
> Please add a `virtual_hook` so we can extend this class in the future without
> breaking ABI should we have the need to extract more data out of a settings
> module:
>
> virtual void virtual_hook(int id, void *data)
I'd slightly disagree here though, if that inherits from QObject anyway I'd
just rely on meta call dispatching. But OK, let's go virtual_hook.
REPOSITORY
R295 KCMUtils
REVISION DETAIL
https://phabricator.kde.org/D28460
To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,
bruns
D28460: Add KCModuleStateProbe as base class for plugin
bport updated this revision to Diff 79496. bport added a comment. Take in consideration feedbacks REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28460?vs=78969=79496 REVISION DETAIL https://phabricator.kde.org/D28460 AFFECTED FILES src/CMakeLists.txt src/kcmoduleloader.cpp src/kcmoduleloader.h src/kcmodulestateprobe.cpp src/kcmodulestateprobe.h To: bport, #plasma, ervin Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
broulik added inline comments. INLINE COMMENTS > kcmodulestateprobe.h:21 > + > +#ifndef KCMODULEDEFAULT_H > +#define KCMODULEDEFAULT_H `KCMODULESTATEPROBE_H` > kcmodulestateprobe.h:25 > +#include > +#include > +#include Forward-declare > kcmodulestateprobe.h:39 > +virtual bool isDefaults() const = 0; > +}; > + Please add a `virtual_hook` so we can extend this class in the future without breaking ABI should we have the need to extract more data out of a settings module: virtual void virtual_hook(int id, void *data) REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28460 To: bport, #plasma, ervin Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
broulik added inline comments.
INLINE COMMENTS
> kcmoduleloader.cpp:165
> +
> +if (!mod.library().isEmpty()) {
> +QString error;
Use an early return here
> kcmoduleloader.cpp:176
> +if (factory) {
> +auto p = factory->create(nullptr, args2);
> +if (p) {
This looks like it leaks
> kcmoduleloader.cpp:182
> +
> +auto p = mod.service()->createInstance(nullptr,
> args2, );
> +if (p) {
Error can be `nullptr`. Either print a warning or remove it
REPOSITORY
R295 KCMUtils
REVISION DETAIL
https://phabricator.kde.org/D28460
To: bport, #plasma, ervin
Cc: broulik, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,
bruns
D28460: Add KCModuleStateProbe as base class for plugin
bport added a dependent revision: D28461: In sidebar mode show if a module is in default state or not. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28460 To: bport, #plasma, ervin Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
D28460: Add KCModuleStateProbe as base class for plugin
bport created this revision. bport added reviewers: Plasma, ervin. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. bport requested review of this revision. REVISION SUMMARY This class will allow to get state of a module without loading the UI. REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D28460 AFFECTED FILES src/CMakeLists.txt src/kcmoduleloader.cpp src/kcmoduleloader.h src/kcmodulestateprobe.cpp src/kcmodulestateprobe.h To: bport, #plasma, ervin Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
