Differential D29711

Create kcmshell.openSystemSettings() and kcmshell.openInfoCenter() functions
ClosedPublic
Actions

Authored by ngraham on May 13 2020, 3:28 PM.

Details

Reviewers
mart
Group Reviewers
Plasma
Commits
R296:1aefe2abc2fc: Create kcmshell.openSystemSettings() and kcmshell.openInfoCenter() functions
Summary

Many Plasma applets use kcmshell.open to open the appropriate KCM. However
now that we're de-emphasizing kcmshell, we want these to open in System
Settings or Info Center instead.

We can't just change the existing function for a few reasons:

  1. This is a frameworks; changing functionality like that isn't nice.
  2. The existing function takes a string list, which KCMShell supports to open multiple KCMs, but System Settings does not support this.
  3. There's still an occasional need to open KCMs in KCMShell (e.g. from another KCM open in System Settings).
  4. We need to distinguish between opening in System Settings and opening in Info Center

Accordingly, two new functions are created: one to open a KCM in System
Settings, and another to open in Info Center. Applets will be ported to
use these.

Test Plan

Test dependent patches (see Stack)

Diff Detail

Repository
R296 KDeclarative
Branch
create-kcmshell-openSystemSettings (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26971
Build 26989: arc lint + arc unit
ngraham created this revision.May 13 2020, 3:28 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 13 2020, 3:28 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
ngraham requested review of this revision.May 13 2020, 3:28 PM
cfeck added a subscriber: cfeck.May 13 2020, 6:48 PM

Sorry if I don't understand the scope, but does this mean I am forced to install systemsettings to be able to use KCMs?

ngraham added a comment.May 13 2020, 7:06 PM
In D29711#670458, @cfeck wrote:

Sorry if I don't understand the scope, but does this mean I am forced to install systemsettings to be able to use KCMs?

Why, is the "I'm using Plasma but I don't have System Settings installed" use case something that you think we should handle?

The context is that users were requesting for KCMs to be opened in System Settings, not KCMShell (https://bugs.kde.org/show_bug.cgi?id=402790) and we implemented that for the KCM runner for Plasma 5.19. The issue fixed here is an inconsistency in that opening KCMs from Plasma applet context menu would still open it in KCMShell, not System Settings or Info Center (which in 5.19 is now just System Settings; the same app shows both).

ltoscano added a subscriber: ltoscano.May 13 2020, 7:46 PM
In D29711#670470, @ngraham wrote:
In D29711#670458, @cfeck wrote:

Sorry if I don't understand the scope, but does this mean I am forced to install systemsettings to be able to use KCMs?

Why, is the "I'm using Plasma but I don't have System Settings installed" use case something that you think we should handle?

The context is that users were requesting for KCMs to be opened in System Settings, not KCMShell (https://bugs.kde.org/show_bug.cgi?id=402790) and we implemented that for the KCM runner for Plasma 5.19. The issue fixed here is an inconsistency in that opening KCMs from Plasma applet context menu would still open it in KCMShell, not System Settings or Info Center (which in 5.19 is now just System Settings; the same app shows both).

If the scope of this change is just about Plasma applet, of course systemsettings is going to be around.
But are you sure you want to kill kcmshell? KCMs are not for Plasma only. Opening them outside Plasma would still require a minimal shell.

Regarding this change, I guess you may have discussed it already, but wouldn't it be possible to find out whether systemsettings is available (and then kinfocenter) and use it, without adding a new function?

ngraham added a comment.May 15 2020, 4:57 AM
In D29711#670503, @ltoscano wrote:

If the scope of this change is just about Plasma applet, of course systemsettings is going to be around.

Yes the scope of this change is purely for Plasma applets; look at the dependent patches.

But are you sure you want to kill kcmshell? KCMs are not for Plasma only. Opening them outside Plasma would still require a minimal shell.

Regarding this change, I guess you may have discussed it already, but wouldn't it be possible to find out whether systemsettings is available (and then kinfocenter) and use it, without adding a new function?

We didn't kill kcmshell, it's explicitly still around.

I can't add new behaviors to the existing function because of the reasons listed in the description section of this patch.

mart added a comment.May 15 2020, 12:20 PM

hmm, should probably check before whether systemsettings5 and kinfocenter exist beforehand and fallback to kcmshell if not?

ngraham added a comment.May 15 2020, 5:03 PM
In D29711#671616, @mart wrote:

hmm, should probably check before whether systemsettings5 and kinfocenter exist beforehand and fallback to kcmshell if not?

Yeah Christoph and Luigi brought that up too. Will do.

ngraham updated this revision to Diff 82971.May 15 2020, 6:13 PM

Only open in System Settings or Info Center if systemsettings is installed

mart accepted this revision.May 19 2020, 8:19 AM
This revision is now accepted and ready to land.May 19 2020, 8:19 AM
ngraham closed this revision.May 19 2020, 1:03 PM

Revision Contents
Changeset List

Diff 82971

View Options

src/qmlcontrols/kquickcontrolsaddons/CMakeLists.txt

Loading...
View Options

src/qmlcontrols/kquickcontrolsaddons/kcmshell.h

Loading...
View Options

src/qmlcontrols/kquickcontrolsaddons/kcmshell.cpp

Loading...