ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.
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).
INLINE COMMENTS
> BlacklistedApplicationsModel.cpp:179
> + for (int i = 0; i < rowCount(); i++) {
> + (d->applications[i].blocked ? blockedApplications :
> allowedApplications)
> + << d->applications[i].name;
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).
> BlacklistedApplicationsModel.cpp:186
> +
> + emit changed(d->pluginConfig->isSaveNeeded());
> + emit defaulted(d->pluginConfig->isDefaults());
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.
> MainConfigurationWidget.cpp:63
> {
> - defaulted(d->tabSwitching->isDefault() && d->tabPrivacy->isDefault());
> + unmanagedWidgetChangeState(changed);
> }
You should be able to connect directly using the function pointer style connect
and spare the existence of onChanged and onDefaulted.
> PrivacyTab.h:57
> Q_SIGNALS:
> - void changed();
> + void changed(bool changed);
> + void defaulted(bool isDefault);
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) ?
REPOSITORY
R119 Plasma Desktop
REVISION DETAIL
https://phabricator.kde.org/D26398
To: meven, #plasma, ervin, bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas,
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed,
jensreuterberg, abetts, sebas, apol, ahiemstra, mart
