Port the ui components to use KConfig XT features to reduce amount of boilerplate code.
Details
- Reviewers
ervin bport - Group Reviewers
Plasma - Commits
- R119:ec73805eb2bf: [KCM/Activities] Use KConfigXT in ui
kcmshell5 activities
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- arcpatch-D26398
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 20800 Build 20818: arc lint + arc unit
Handle BlacklistedApplicationsModel properly, prevent a warning 'duplicate connection name'
A few changes needed to make the code easier to understand in a few months time. Also there's a larger concern of a piece of code being prone to later bugs (although I'd expect it to work for now).
kcms/activities/BlacklistedApplicationsModel.cpp | ||
---|---|---|
176 | I personally like that construct, but AFAIK it's rather unusual in KDE code, so maybe for the sake of the future developer use something more "classic". const auto name = d->applications[i].name; if (d->applications[i].blocked) { blockedApplications << name; } else { allowedApplications << name; } Or: auto &list = (d->applications[i].blocked ? blockedApplications : allowedApplications); list << d->applications[i].name; Second option is closer to your initial intent (I think it's still less common than the first, but at least we guide the future reader understanding with the intermediate variable). | |
183 | It shall work as intended for now, but I think there's a potential caveat there in case of future refactoring and if the timing between save()/load() of the various settings object change. The isSaveNeeded()/isDefaults() state will be based on the whole settings object state. So we might claim here for saving being needed (currently unlikely AFAICT) or settings not being defaults (currently likely if load() brought values from the file which weren't defaults) based on config keys which are not managed by this class. I thus wonder if it wouldn't be wise to restrict that to only the items concerned. | |
kcms/activities/MainConfigurationWidget.cpp | ||
60–61 | You should be able to connect directly using the function pointer style connect and spare the existence of onChanged and onDefaulted. | |
kcms/activities/PrivacyTab.h | ||
57–58 | I think I'd make the name of those signals more explicit. The tab is mostly working as a regularly managed thing, except for the model. So maybe just prefix with "blacklist" like: |
kcms/activities/BlacklistedApplicationsModel.cpp | ||
---|---|---|
176 | It was just some copy/paste will update to the second option i tink | |
183 | I tride this, but I can't access here to the default value of blockedApplications and allowedApplications as the generated function defaultAllowedApplicationsValue_helper() is protected. |
kcms/activities/BlacklistedApplicationsModel.cpp | ||
---|---|---|
188 | To reduced code duplication and make sure we're not uncontrollably getting in null pointers land, I'd use intermediate variables for the item pointers and I'd throw a bunch of Q_ASSERT: auto blockedAppsItem = d->pluginConfig->findItem("blockedApplications"); Q_ASSERT(blockedAppsItem); auto allowedAppsItem = d->pluginConfig->findItem("allowedApplications"); Q_ASSERT(allowedAppsItem); |