dfaure added a comment.
Thanks for the review! I was getting desperate to get one ;-) INLINE COMMENTS > svuorela wrote in kcmoduleinfo.cpp:73 > At a later point, I*m not sure what the purpose is for these members are - > but that's probably for another changeset. Right, I was wondering the same. We could just implement the getters so that they call these things directly. But then I'm also wondering what's the purpose of KCModuleInfo at all and whether we could just use KPluginInfo directly instead -- but that's a KF6 consideration, since it's part of the API for KCMultiDialog and others. Well, we could add a addModule(KPluginInfo) overload if that's the direction we're going for; I just don't have full overview on the KCM stuff. > svuorela wrote in kcmoduleinfo.h:131 > Can we mark it as deprecated? It's complicated. 1. If you use the QString constructor, you know service() is usable. That's the case for all users of KCModuleInfo except KCModuleLoader. [Not that there are many] 2. Even KCModuleLoader calls service(), to test for noDisplay(). The whole concept of NoDisplay only makes sense for desktop files, unless we want to have that in JSON metadata as well. I suppose we do, but this currently seems to be completely missing (e.g. from KPluginMetaData, if we want this in all plugins, not just KCMs). We'd have to duplicate the logic currently in KService::noDisplay(). Any volunteers to look into this? :-) REPOSITORY R295 KCMUtils BRANCH master REVISION DETAIL https://phabricator.kde.org/D28765 To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela Cc: svuorela, cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
