D26398: [KCM/Activities] Use KConfigXT in ui

[Kevin Ottens]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200108/4e50da4c/attachment-0001.html>