Port KCMSplashScreen to KConfigXT
ClosedPublic

Authored by crossi on Oct 17 2019, 1:10 PM.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
crossi requested review of this revision.Oct 17 2019, 1:10 PM
crossi created this revision.
apol added a subscriber: apol.Oct 17 2019, 3:21 PM
apol added inline comments.
kcms/ksplash/CMakeLists.txt
29

You don't need to install the kcfg file if you're generating it.

crossi added inline comments.Oct 18 2019, 7:55 AM
kcms/ksplash/CMakeLists.txt
29

kcfg file is not generated, it is used to generate class SplashScreenSettings

It was requested by davidedmundson on another review, thought I should add it as well.

bport accepted this revision.Oct 21 2019, 7:01 AM
This revision is now accepted and ready to land.Oct 21 2019, 7:01 AM
ervin added a comment.Oct 21 2019, 8:42 AM

Bunch of comments, not formally requesting changes since for a couple of them I'm unsure if I missed something specific to the module being at play.

kcms/ksplash/CMakeLists.txt
29

I'm still not quite sure why it was requested to be honest. The reusability potential of those kcfgs is rather low in most cases.

kcms/ksplash/kcm.cpp
138

I'd expect this to be unnecessary since load() will emit ThemeChanged or EngineChanged if said config keys really changed.

145

I'd expect either setNeedsSave(false) here, or configChanged() to be connected from the ctor. Are we sure that state is reset properly on apply being clicked?

kcms/ksplash/splashscreensettings.kcfg
12

What about using the name elements for both entries to avoid ending up with signals starting with an uppercase? (although one could argue that kconfig_compiler should be fixed instead... it somehow lowercases properly the property and getter but not the signal).

crossi added inline comments.Oct 21 2019, 3:38 PM
kcms/ksplash/kcm.cpp
138

Yes, this one is not necessary.

145

When clicking on "apply", the button is disabled.

crossi updated this revision to Diff 68450.Oct 21 2019, 3:42 PM

remove unnecessary emit

Restricted Application added a project: Plasma. · View Herald TranscriptOct 21 2019, 3:42 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
crossi updated this revision to Diff 68532.Oct 22 2019, 12:35 PM

Use both key and name tag in entry to generate pretty signals' name.

ervin accepted this revision.Oct 22 2019, 12:37 PM
This revision was automatically updated to reflect the committed changes.