Introduce an Info Center mode
ClosedPublic

Authored by mart on Jan 24 2020, 12:52 PM.

Details

Summary

withthe command line switch -i, start
systemsettings loading the KInfoCenter modules and hyerarchy instead.

  • KAboutData and dbus service name will be the same of KInfoCenter
  • The startup page is not shown but the "System Information" kcm is loaded instead
  • KACtivityStats is disabled
  • The home button will load the "System Information" kcm as well
  • It will *always* be in sidebar mode regardless how systemsettings is configured
  • Config dialog can't be open.

BUG:412975

Test Plan
  • SystemSettings mode functionality didn't change
  • infocenter mode is fully functional

Diff Detail

Repository
R124 System Settings
Branch
mart/kinfoCenterMode
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21737
Build 21755: arc lint + arc unit
mart created this revision.Jan 24 2020, 12:52 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 24 2020, 12:52 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart requested review of this revision.Jan 24 2020, 12:52 PM
mart edited the test plan for this revision. (Show Details)Jan 24 2020, 12:56 PM
mart added a dependent revision: D26896: Kill the KinfoCenter binary.

Great idea!

app/SettingsBase.h
98

I would prefer this to be an enum

app/main.cpp
83

Can we not just create a different KAboutData instance?

88

i18n

davidedmundson added inline comments.
app/SettingsBase.h
48

why not an arg in the ctor then?

93

what happens to lost and found modules

app/main.cpp
76

As an alternative, we could switch based on our binary name, from arg[0] and then install a symlink.
Then we have 100% compatibility

meven added a subscriber: meven.Jan 24 2020, 4:17 PM
meven added inline comments.
core/MenuModel.cpp
26

not needed

core/ModuleView.cpp
349–353

We might want to have a different url scheme like "kinfo" insteat of "kcm" and "org.kde.kinfocenter" instead of "org.kde.systemsettings"

mart updated this revision to Diff 74422.Jan 27 2020, 1:59 PM
mart marked 6 inline comments as done.
  • base behavior on executable name
mart added inline comments.Jan 27 2020, 1:59 PM
core/ModuleView.cpp
349–353

that's a good point.
Tough, at the moment we don't do anything with the statistics in kinfocenter mode, so for now better not clutter the kastats database?

meven added inline comments.Jan 27 2020, 6:23 PM
core/BaseMode.cpp
32

Not needed

core/ModuleView.cpp
349–353

We can ignore this for now, for sure, since we don't have a use case indeed.
KFeedback may like to access kactivities stats though.

mart updated this revision to Diff 74481.Jan 28 2020, 10:07 AM
  • remove usekless qdebug
mart marked an inline comment as done.Jan 28 2020, 10:07 AM
meven added inline comments.Jan 28 2020, 10:45 AM
app/main.cpp
81–98

You can use mode here

mart updated this revision to Diff 74487.Jan 28 2020, 11:50 AM
  • use mode
mart marked an inline comment as done.Jan 28 2020, 11:50 AM
mart edited the summary of this revision. (Show Details)
davidedmundson accepted this revision.Jan 28 2020, 1:01 PM
davidedmundson added inline comments.
sidebar/SidebarMode.cpp
589–590

Yes! that would be a nicer design

This revision is now accepted and ready to land.Jan 28 2020, 1:01 PM
mart updated this revision to Diff 74491.Jan 28 2020, 1:07 PM
  • better todo message
This revision was automatically updated to reflect the committed changes.

Not sure it's my place to do so, but I'd like to make a small comment.
Instead of passing mode around and all the if statements, wouldn't it be better to have a single decision point in main.cpp, like a factory class?
Use polymorphism and create something like InfoCenter : CommonParent, SystemSettings : CommonParent, each knowing how to initialize their KAboutData, GUI elements, icons etc.?
And in main.cpp do something like CommonParent * commonParent = new InfoCenter() // or new SystemSettings() and use that instead of mode.
Further down the road, if you need to add something to system settings, you could do it in the SystemSettings class maybe and not have to add another if to remove it from the InfoCenter part. Just a thought, not sure if it fits.

ovichiro added inline comments.Feb 3 2020, 6:25 PM
app/main.cpp
68

The authors should maybe be moved outside if-else since they are the same for both branches.

ngraham added a subscriber: ngraham.Feb 3 2020, 9:10 PM

Not sure it's my place to do so, but I'd like to make a small comment.
Instead of passing mode around and all the if statements, wouldn't it be better to have a single decision point in main.cpp, like a factory class?
Use polymorphism and create something like InfoCenter : CommonParent, SystemSettings : CommonParent, each knowing how to initialize their KAboutData, GUI elements, icons etc.?
And in main.cpp do something like CommonParent * commonParent = new InfoCenter() // or new SystemSettings() and use that instead of mode.
Further down the road, if you need to add something to system settings, you could do it in the SystemSettings class maybe and not have to add another if to remove it from the InfoCenter part. Just a thought, not sure if it fits.

Sounds sensible. Would you like to submit a patch?

Sounds sensible. Would you like to submit a patch?

I don't know how yet. I need to brush up on C++ first. I'll see what I can do in the next few days or weeks.