[KCM/Activities] Use KConfigXT in ui
ClosedPublic

Authored by meven on Jan 3 2020, 4:11 PM.

Details

Summary

Port the ui components to use KConfig XT features to reduce amount of boilerplate code.

Test Plan

kcmshell5 activities

Diff Detail

Repository
R119 Plasma Desktop
Branch
arcpatch-D26398
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20799
Build 20817: arc lint + arc unit
meven created this revision.Jan 3 2020, 4:11 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 3 2020, 4:11 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
meven requested review of this revision.Jan 3 2020, 4:11 PM
meven updated this revision to Diff 72849.Jan 6 2020, 8:46 AM

KCM/Component clean unused file

meven updated this revision to Diff 72850.Jan 6 2020, 8:46 AM

Remove unwanted change

meven planned changes to this revision.Jan 6 2020, 12:37 PM
meven updated this revision to Diff 72964.Jan 7 2020, 11:51 AM

Handle BlacklistedApplicationsModel properly, prevent a warning 'duplicate connection name'

ervin requested changes to this revision.Jan 8 2020, 10:02 AM

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".
Either:

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:
blacklistChanged(bool) and blacklistDefaulted(bool) ?

This revision now requires changes to proceed.Jan 8 2020, 10:02 AM
meven added inline comments.Jan 8 2020, 11:16 AM
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.
I was missing an alternative.

meven updated this revision to Diff 73054.Jan 8 2020, 11:19 AM
meven marked 2 inline comments as done.

Review, code style

meven updated this revision to Diff 73055.Jan 8 2020, 11:24 AM
meven marked 2 inline comments as done.

In BlackListModel only acces concerned items isSaveNeeded and isDefault

meven updated this revision to Diff 73056.Jan 8 2020, 11:29 AM
meven marked 2 inline comments as done.

Improve signals naming, and remove redundant ones

ervin requested changes to this revision.Jan 8 2020, 11:31 AM
ervin added inline comments.
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);
This revision now requires changes to proceed.Jan 8 2020, 11:31 AM
meven updated this revision to Diff 73062.Jan 8 2020, 2:01 PM
meven marked an inline comment as done.

Add Q_ASSERTs

meven updated this revision to Diff 73063.Jan 8 2020, 2:03 PM

Only once commit

ervin accepted this revision.Jan 20 2020, 12:40 PM
This revision is now accepted and ready to land.Jan 20 2020, 12:40 PM
This revision was automatically updated to reflect the committed changes.