D28460: Add KCModuleStateProbe as base class for plugin

[Kevin Ottens]

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 &arg : 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200407/dffbdf3d/attachment-0001.html>