meven added inline comments.
INLINE COMMENTS
> ervin wrote in BlacklistedApplicationsModel.cpp:179
> 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).
It was just some copy/paste will update to the second option i tink
> ervin wrote in BlacklistedApplicationsModel.cpp:186
> 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.
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.
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
