D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a comment.


  https://invent.kde.org/frameworks/kcmutils/-/merge_requests/2

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: ahmadsamir, rikmills, wbauer, kossebau, svuorela, cblack, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Ahmad Samir]

ahmadsamir added a comment.


  I guess a proper fix will need to revert part of this to have the KModuleInfo 
ctor that took a KService not use KPluginInfo internally; better still, of 
course, is having the KPluginInfo ctor work for the case of 
X-KDE-ServiceTypes=SystemSettingsExternalApp, (I couldn't grok KPluginInfo 
stuff fully yet).

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: ahmadsamir, rikmills, wbauer, kossebau, svuorela, cblack, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Ahmad Samir]

ahmadsamir added a comment.


  systemsettings uses KCModuleInfo::service() to check whether the moduleinfo 
is valid[1], the problem is creating a KPluginInfo from the KService based on a 
.desktop file with X-KDE-ServiceTypes=SystemSettingsExternalApp fails because 
it doesn't seem to have valid metadata. Since the KPluginInfo ctor fails, 
accessing d->pluginInfo.service() causes a crash, I've submitted a proposed fix 
at [2]. This doesn't fix the bug, but merely prevents the crash.
  
  [1] 
https://invent.kde.org/plasma/systemsettings/-/blob/master/core/ModuleView.cpp#L229
  [2] https://invent.kde.org/frameworks/kcmutils/-/merge_requests/1

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: ahmadsamir, rikmills, wbauer, kossebau, svuorela, cblack, 
kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Rik Mills]

rikmills added a comment.


  In D28765#673191 , @wbauer wrote:
  
  > This caused a crash in openSUSE:
  >  https://bugs.kde.org/show_bug.cgi?id=421566
  
  
  The same crash can been seen in Kubuntu when trying to launch the external 
software-properties app (driver manager) via systemsettings.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: rikmills, wbauer, kossebau, svuorela, cblack, kde-frameworks-devel, 
LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Wolfgang Bauer]

wbauer added a comment.


  This caused a crash in openSUSE:
  https://bugs.kde.org/show_bug.cgi?id=421566

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: wbauer, kossebau, svuorela, cblack, kde-frameworks-devel, LeGast00n, 
michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> dfaure wrote in kcmoduleinfo.h:131
> 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? :-)

As I just ran into this code, a short reminder: it would be a regression not to 
have a noDisplay check in the case of pure JSON metadata, as the current code 
of KService::;noDisplay() also asks KAuth whether the page should be shown 
(`KAuthorized::authorizeControlModule(storageId())`), which some KIOSK systems 
or admins might rely on, but also `showInCurrentDesktop()` & 
`showOnCurrentPlatform()`, which also still make sense, for what I can tell.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham, svuorela
Cc: kossebau, svuorela, cblack, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure closed this revision.

REPOSITORY
  R295 KCMUtils

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

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

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

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Sune Vuorela]

svuorela accepted this revision.
svuorela added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kcmoduleinfo.cpp:73
> +lib = pluginInfo.libraryPath();
> +keywords = 
> pluginInfo.property(QStringLiteral("Keywords")).toStringList();
>  }

At a later point, I*m not sure what the purpose is for these members are - but 
that's probably for another changeset.

> kcmoduleinfo.h:131
> + * @warning This will be null if this KCModuleInfo was created from a 
> KPluginInfo coming from KPluginMetaData.
> + * Prefer using pluginInfo() instead, which works for both kinds.
> + */

Can we mark it as deprecated?

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

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a comment.


  Any idea who could review this?

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a reviewer: ngraham.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson, ngraham
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a dependent revision: D28848: Port kontact plugin loading to 
KPluginLoader.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure added a comment.


  For KF6 we could also just replace KCModuleInfo with KPluginInfo. It really 
doesn't provide anything on top

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure updated this revision to Diff 79906.
dfaure added a comment.


  Use @warning. Thanks for the tip!

REPOSITORY
  R295 KCMUtils

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28765?vs=79893=79906

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28765

AFFECTED FILES
  src/kcmoduleinfo.cpp
  src/kcmoduleinfo.h
  src/kcmoduleloader.cpp
  src/kcmultidialog.cpp
  src/kpluginselector.cpp
  src/ksettings/dialog.cpp

To: dfaure, pino, broulik, mart, davidedmundson
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[Carson Black]

cblack added inline comments.

INLINE COMMENTS

> kcmoduleinfo.h:130
>   * @return a QExplicitlySharedDataPointer to KService created from the 
> modules .desktop file
> + * WARNING: this will be null if this KCModuleInfo was created from a 
> KPluginInfo coming from KPluginMetaData
> + * Prefer using pluginInfo() instead, which works for both kinds.

Use `@warning` instead of `WARNING:` for better markup

`WARNING:`
F8232033: image.png 
`@warning:`
F8232034: image.png 

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D28765

To: dfaure, pino, broulik, mart, davidedmundson
Cc: cblack, kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns

D28765: KSettings::Dialog: add support for KPluginInfos without a KService

[David Faure]

dfaure created this revision.
dfaure added reviewers: pino, broulik, mart, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  KPluginInfo evolved into an abstraction over old-style plugins
  (KService i.e. desktop files) and new-style plugins with JSON
  (KPluginMetaData/KPluginLoader).
  
  When porting kontact to new-style plugins, I hit the issue that
  KSettings::Dialog has a code path that creates a 'fake' KCM module
  just to get a checkable item in the config dialog. That fake module
  was using the plugininfo's KService, which was null, into the KCModuleInfo.
  
  So I added support for metadata-based plugins to KCModuleInfo.
  But rather than using KPluginMetaData and then turning KCModuleInfo into
  yet another abstraction over old and new, I just used KPluginInfo in
  there. One can still create a KCModuleInfo from a KService, but
  internally it'll use a KPluginInfo. This makes KCModuleInfo::service()
  dangerous now though, it can be null [but only in apps that start passing
  new-style KPluginInfos, so not in systemsettings5 yet].
  This commits ports kcmutils away from it as much as possible.

TEST PLAN
  kontact's configuration dialog works (with and without my
  not-yet-committed port to KPluginLoader). systemsettings5 still works.
  konqueror's configuration dialog still works.

REPOSITORY
  R295 KCMUtils

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D28765

AFFECTED FILES
  src/kcmoduleinfo.cpp
  src/kcmoduleinfo.h
  src/kcmoduleloader.cpp
  src/kcmultidialog.cpp
  src/kpluginselector.cpp
  src/ksettings/dialog.cpp

To: dfaure, pino, broulik, mart, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns