D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
This revision was automatically updated to reflect the committed changes. Closed by commit R295:6d6e2427f8e2: Update KPluginSelector to allow KCM to show good state for reset, apply and… (authored by bport). REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26466?vs=72906=72954 REVISION DETAIL https://phabricator.kde.org/D26466 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
meven accepted this revision. This revision is now accepted and ready to land. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport marked an inline comment as done. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
ervin accepted this revision. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport marked 9 inline comments as done. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport updated this revision to Diff 72906. bport added a comment. Increment version signal was introduced and remove uneeded blank line REPOSITORY R295 KCMUtils CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D26466?vs=72885=72906 REVISION DETAIL https://phabricator.kde.org/D26466 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
ervin added inline comments. INLINE COMMENTS > bport wrote in kpluginselector_p.h:205 > The method declaration changed and the connected slot already provide the > value Right, I missed the relevant connect. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
ervin added inline comments. INLINE COMMENTS > bport wrote in kpluginselector.cpp:389 > isPluginEnabled represent the current state (i.e. after load) so we compare > the file value with default value I thought that was the current GUI state not the current storage state and got confused. I stand corrected. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport added inline comments. INLINE COMMENTS > ervin wrote in kpluginselector.cpp:389 > 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. isPluginEnabled represent the current state (i.e. after load) so we compare the file value with default value > ervin wrote in kpluginselector_p.h:205 > 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. The method declaration changed and the connected slot already provide the value 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
ervin requested changes to this revision.
ervin added inline comments.
INLINE COMMENTS
> kpluginselector.cpp:286
>
> +connect(this, ::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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
meven requested changes to this revision. meven added a comment. This revision now requires changes to proceed. Apart from @since version, seems fine to me INLINE COMMENTS > kpluginselector.h:234 > + * Emitted after configuration is changed, tell if configuration > represent default or not > + * @since 5.66 > + */ 5.67, 5.66 was released saturday 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport added a dependent revision: D26467: KCM runners: fix default button. 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
D26466: Update KPluginSelector to allow KCM to show good state for reset, apply and default button
bport created this revision. bport added reviewers: Plasma, ervin, crossi, meven. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. bport requested review of this revision. REVISION SUMMARY - emit changed with good value to ensure reset button is activated only when needed - add defaulted signal to activate dafault button only when needed REPOSITORY R295 KCMUtils REVISION DETAIL https://phabricator.kde.org/D26466 AFFECTED FILES src/kpluginselector.cpp src/kpluginselector.h src/kpluginselector_p.h To: bport, #plasma, ervin, crossi, meven Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
