Support module commandline paramenter
ClosedPublic

Authored by mart on Apr 21 2020, 4:47 PM.

Details

Summary

in prevision of more or less replacing kcmshell with systemsettings
itself, suppo loading a module as a paramenter as well as a --list
paramenter

Test Plan

systemsettings5 kwindecoration loads the decoration kcm and the proper
category selected on the sidebar

Diff Detail

Repository
R124 System Settings
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Apr 21 2020, 4:47 PM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 21 2020, 4:47 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Apr 21 2020, 4:47 PM
mart retitled this revision from support module commandline paramenter to [WIP] Support module commandline paramenter.Apr 21 2020, 4:47 PM
ngraham added inline comments.
icons/IconMode.cpp
148

aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaahhhhhh!

sidebar/SidebarMode.cpp
529

?

mart updated this revision to Diff 80903.Apr 22 2020, 3:35 PM
  • support for startup module arguments
mart retitled this revision from [WIP] Support module commandline paramenter to Support module commandline paramenter.Apr 22 2020, 3:36 PM
mart updated this revision to Diff 80904.Apr 22 2020, 3:38 PM
mart marked 2 inline comments as done.
  • remove leftovers
bport added a subscriber: bport.Apr 22 2020, 3:47 PM
bport added inline comments.
icons/IconMode.cpp
204

Indentation is not correct

This doesn't build for me:

/home/nate/kde/src/systemsettings/app/main.cpp: In function ‘int main(int, char**)’:
/home/nate/kde/src/systemsettings/app/main.cpp:115:91: error: ‘endl’ was not declared in this scope; did you mean ‘std::endl’?
  115 |         std::cout << i18n("The following modules are available:").toLocal8Bit().data() << endl;
mart updated this revision to Diff 80913.Apr 22 2020, 4:47 PM
  • use namespace
ngraham accepted this revision.Apr 22 2020, 4:58 PM

Works great, and I fully support the overall goal of getting rid of kcmshell. Doing so will fix a ton of bugs (all the systemsettings | kcmshell bugs as well as all the bug reports about weird sizing in standalone KCMs, plus a bunch more about inconsistent behavior of KCMs in systemsettings vs kcmshell)

Foor fod thought: we might want to also think about some of the consequences of removing kcmshell; we could need to add programmatic cross-KCM navigation into system settings itself. For example, in D29080, I'm calling kcmshell from inside a KCM to pop-up the KScreen KCM on top of the fonts KCM. Without kcmshell, that won't work as currently implemented, and it would need to navigate you to the KScreen KCM in the same systemsettings instance.

Also a new way to get the information from kcmshell5 --list would be nice.

Material for follow-up patches, I suppose.

This revision is now accepted and ready to land.Apr 22 2020, 4:58 PM
mart added a comment.Apr 23 2020, 8:47 AM

Foor fod thought: we might want to also think about some of the consequences of removing kcmshell; we could need to add programmatic cross-KCM navigation into system settings itself. For example, in D29080, I'm calling kcmshell from inside a KCM to pop-up the KScreen KCM on top of the fonts KCM.

i guess it doesn't have to be removed completely (kcmshell itself is so little code that isn't really a big maintainance overhead) one thing that could be done probably is just changing krunner to launch systemsettings instead and leaving the rest unchanged.

the *really* annoying thing is that every single kcm has a line like

Exec=kcmshell5 kcm_lookandfeel

in its desktop file. Which would call something along the lines of what we did for kinfocenter, to still have kcmshell5 as a symlink, but in this case i don't think it would cover all cases?

Without kcmshell, that won't work as currently implemented, and it would need to navigate you to the KScreen KCM in the same systemsettings instance.

Also a new way to get the information from kcmshell5 --list would be nice.

systemsettings5 --list works ;)

In D29064#655349, @mart wrote:

the *really* annoying thing is that every single kcm has a line like

Exec=kcmshell5 kcm_lookandfeel

in its desktop file. Which would call something along the lines of what we did for kinfocenter, to still have kcmshell5 as a symlink, but in this case i don't think it would cover all cases?

Without kcmshell, that won't work as currently implemented, and it would need to navigate you to the KScreen KCM in the same systemsettings instance.

Yeah I guess a compatibility symlink that opens systemsettings5 will be necessary.

Also a new way to get the information from kcmshell5 --list would be nice.

systemsettings5 --list works ;)

Excellent!

Note you can pass multiple arguments to kcmshell5 to open a dedicated save window with random KCMs inside, e.g. done by battery monitor:

kcmshell5 powerdevilprofilesconfig powerdevilactivitiesconfig powerdevilglobalconfig

Not sure how this could be translated 1-on-1 :/
Though power management is its own group, so maybe we could port that over to passing the parent category in or something. But reimplementing that in systemsettings looks like a rabbit hole we don't want to do down into.

mart updated this revision to Diff 81319.Apr 27 2020, 9:45 AM
  • load a new module when a new instance is invoked
ngraham accepted this revision.Apr 27 2020, 3:11 PM
ngraham requested changes to this revision.May 8 2020, 4:16 PM
This revision now requires changes to proceed.May 8 2020, 4:16 PM
ngraham accepted this revision.May 8 2020, 4:20 PM
This revision is now accepted and ready to land.May 8 2020, 4:20 PM
This revision was automatically updated to reflect the committed changes.