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