D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button

[Kevin Ottens]

ervin requested changes to this revision.
ervin added inline comments.

INLINE COMMENTS

> kpluginselector.cpp:286
>  
> +    connect(this, &KPluginSelector::changed, [=]{ emit defaulted(this->isDefault()); });
> +

We generally don't "this->", also you're capturing too much with =, capturing this would be enough.

> kpluginselector.cpp:388
>  
> -    emit changed(true);
> +
> +    emit changed(isChanged);

No need for the extra blank line

> kpluginselector.cpp:389
> +
> +    emit changed(isChanged);
>  }

This logic looks wrong to me.

isChanged indicates if an entry state was changed during the course of the call to defaults(). It's very possible it's false when getting out of the loop.
If that's the case we will emit changed(false). But the changed() signal here indicates if we have a state different from the last load() call.

I think in such a case we'd loose the information of being in a "dirty" state while defaults() wouldn't have changed state at all.

> kpluginselector.h:241
>      Private *const d;
> +
>  };

nitpick: Shouldn't be here

> kpluginselector_p.h:205
>      void slotStateChanged(bool state);
> -    void emitChanged();
> +    void emitChanged(bool state);
>      void slotAboutClicked();

Either this is unused or this doesn't compile/run properly. Indeed, this signature changed but no other code has been adjusted to match it.

REPOSITORY
  R295 KCMUtils

REVISION DETAIL
  https://phabricator.kde.org/D26466

To: bport, #plasma, ervin, crossi, meven
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200106/226017ed/attachment-0001.html>