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

Authored by bport on Jan 6 2020, 1:31 PM.

Details

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

Diff Detail

Repository
R295 KCMUtils
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Jan 6 2020, 1:31 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 6 2020, 1:31 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bport requested review of this revision.Jan 6 2020, 1:31 PM
meven requested changes to this revision.Jan 6 2020, 1:38 PM

Apart from @since version, seems fine to me

src/kpluginselector.h
234

5.67, 5.66 was released saturday

This revision now requires changes to proceed.Jan 6 2020, 1:38 PM
ervin requested changes to this revision.Jan 6 2020, 1:51 PM
ervin added inline comments.
src/kpluginselector.cpp
286

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

388

No need for the extra blank line

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.

src/kpluginselector.h
241

nitpick: Shouldn't be here

src/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.

bport added inline comments.Jan 6 2020, 3:11 PM
src/kpluginselector.cpp
389

isPluginEnabled represent the current state (i.e. after load) so we compare the file value with default value

src/kpluginselector_p.h
205

The method declaration changed and the connected slot already provide the value

ervin added inline comments.Jan 6 2020, 3:13 PM
src/kpluginselector.cpp
389

I thought that was the current GUI state not the current storage state and got confused. I stand corrected.

ervin added inline comments.Jan 6 2020, 3:14 PM
src/kpluginselector_p.h
205

Right, I missed the relevant connect.

bport updated this revision to Diff 72906.Jan 6 2020, 4:09 PM

Increment version signal was introduced and remove uneeded blank line

bport marked 9 inline comments as done.Jan 6 2020, 4:10 PM
ervin accepted this revision.Jan 6 2020, 4:19 PM
bport marked an inline comment as done.Jan 6 2020, 5:04 PM
meven accepted this revision.Jan 7 2020, 9:18 AM
This revision is now accepted and ready to land.Jan 7 2020, 9:18 AM
This revision was automatically updated to reflect the committed changes.