KCM Style : kcm state is handled by ManagedConfigModule, properties are bound to settings, making it a bit simpler.
ClosedPublic

Authored by crossi on Nov 8 2019, 4:49 PM.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 18715
Build 18733: arc lint + arc unit
crossi created this revision.Nov 8 2019, 4:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptNov 8 2019, 4:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
crossi requested review of this revision.Nov 8 2019, 4:49 PM
crossi updated this revision to Diff 69467.Nov 8 2019, 4:59 PM

make access to selectedStyle more consistent

crossi updated this revision to Diff 69468.Nov 8 2019, 5:02 PM

missed that one

ervin added inline comments.Nov 12 2019, 10:54 AM
kcms/style/kcmstyle.cpp
98–102

Space before &, not after.

245

AFAICT this is the only place where m_effectsDirty is read. So I think you can remove that flag altogether, which should bring further simplifications.

247–248

Are you sure you need this m_previousStyle? I'd expect it to be exactly m_settings->widgetStyle().

crossi added inline comments.Nov 12 2019, 1:08 PM
kcms/style/kcmstyle.cpp
98–102

Will fix

245

m_effectsDirty is checked line 291, to trigger or not a refresh of all applications.

This check before save is now useless, since if we can save, it means a modification has occured, style or effects.

247–248

m_model->selectedStyle() is bound to m_settings->widgetStyle() and vice versa, so I need to save previous state to allow a rollback.
I should have used m_settings->widgetStyle() here and after for consistency.

ervin added inline comments.Nov 12 2019, 1:45 PM
kcms/style/kcmstyle.cpp
247–248

As far as rollback goes (might not be enough in your case), wouldn't checking for isSaveNeeded() on the settings and rollbacking it all be enough?

We can't rollback a single item though, I guess that would be the main limitation in your case... Maybe that's something to add to KConfigSkeletonItem?

Answer might be no to both, I'm just looking for opportunities to simplify code in modules. :-)

crossi updated this revision to Diff 69635.Nov 12 2019, 2:02 PM

style + consistency

ervin accepted this revision.Nov 15 2019, 7:06 AM
ervin added inline comments.
kcms/style/kcmstyle.cpp
245

Not for this patch, but I still wonder if we could kill it. I think it'd be better to have it as a local variable whose value would be computed from the state of the relevant items in the settings object. This would remove the logic of maintaining an extra state exploited only in save(). Also "effectsDirty" is a weird name I think, it doesn't seem related to any concept of effects.

This revision is now accepted and ready to land.Nov 15 2019, 7:06 AM
This revision was automatically updated to reflect the committed changes.