KCM/Compositing: Use KConfig XT to store values
ClosedPublic

Authored by meven on Mar 9 2020, 6:05 PM.

Details

Summary
  • Clean a little the code
  • Improve a little the UI
Test Plan

Before:


After:

Diff Detail

Repository
R108 KWin
Branch
kwincompositing
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23544
Build 23562: arc lint + arc unit
meven created this revision.Mar 9 2020, 6:05 PM
Restricted Application added a project: KWin. · View Herald TranscriptMar 9 2020, 6:05 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Mar 9 2020, 6:05 PM
meven updated this revision to Diff 77308.Mar 9 2020, 6:06 PM

Make default button work

meven edited the test plan for this revision. (Show Details)Mar 9 2020, 6:08 PM
zzag added a subscriber: zzag.Mar 10 2020, 5:00 PM
zzag added inline comments.
kcmkwin/kwincompositing/compositing.cpp
55

Naming nitpick: onChanged isn't really a great name for a signal. Usually, "on" is used with slots.

What about connecting Compositing::animationSpeedChanged to Compositing::updateSettings? e.g.

connect(this, &Compositing::animationSpeedChanged,       this, &Compositing::updateSettings);
285–294

Given that m_changed is not set to true when anything has been changed, there is a chance that we won't send a reinit request.

We probably need to save the return value of m_settings->isSaveNeeded() before calling m_settings->save() and check it here.

zzag requested changes to this revision.Mar 10 2020, 5:00 PM
This revision now requires changes to proceed.Mar 10 2020, 5:00 PM
meven updated this revision to Diff 77394.Mar 11 2020, 8:56 AM
meven marked 2 inline comments as done.

Make updateSettings a slot, remove m_changed

zzag added inline comments.Mar 11 2020, 9:12 AM
kcmkwin/kwincompositing/compositing.h
93

We probably don't need this signal anymore.

meven updated this revision to Diff 77395.Mar 11 2020, 9:13 AM

Remove unneeded signal

meven marked an inline comment as done.Mar 11 2020, 9:13 AM
meven updated this revision to Diff 77397.Mar 11 2020, 9:53 AM

Squash commits

meven updated this revision to Diff 77398.Mar 11 2020, 9:58 AM

Clean bad connect

meven updated this revision to Diff 77434.Mar 11 2020, 4:53 PM

KCM/Compositing: Use KConfig XT in UI

meven updated this revision to Diff 77435.Mar 11 2020, 4:54 PM

Clean bad commit

ervin requested changes to this revision.Mar 16 2020, 4:50 PM

It's a first step toward using addConfig later on,right?

kcmkwin/kwincompositing/compositing.cpp
80

That - 4 is very obscure here

81

Would be nice to use key + name in the kcfg to have proper capitalization for "gl"

84

ditto

102

ditto

263

would be nice to initialize it

This revision now requires changes to proceed.Mar 16 2020, 4:50 PM
zzag added inline comments.Mar 19 2020, 1:24 PM
kcmkwin/kwincompositing/compositing.cpp
80

Heh, - 4 and + 4 were obscure before so this patch doesn't make things better or worse. Just for context, we add and subtract 4 because long time ago HiddenPreviews had values with different meaning than what we have now. In order to break backwards compatibility, the new values were shifted by 4.

meven updated this revision to Diff 78077.Mar 20 2020, 10:07 AM
meven marked 6 inline comments as done.

Use key attribute from item prefixed by gl, add a comment about magic -4 HiddenPreviews

zzag added a comment.Mar 20 2020, 10:27 AM

Overall, +1.

kcmkwin/kwincompositing/compositing.cpp
53–64

Shouldn't the parent be this?

kcmkwin/kwincompositing/compositing.h
104–107

naming nit: could you please rename these methods to something else? It's unclear what values applyValues applies.

What about these?

void updateSettingsFromUi();
void updateUiFromSettings();

Feel free to ignore this comment because it looks like these methods will go away in D27988

meven updated this revision to Diff 78099.Mar 20 2020, 2:49 PM
meven marked 2 inline comments as done.

Use correct parent for KWinCompositingSetting

zzag accepted this revision.Mar 23 2020, 8:56 AM

Looks good to me. Please wait for feedback from other reviewers.

ervin accepted this revision.Mar 27 2020, 4:44 PM
ervin added inline comments.
kcmkwin/kwincompositing/compositing.cpp
80

And we didn't wrap that in conversion functions? :-)

This revision is now accepted and ready to land.Mar 27 2020, 4:44 PM
This revision was automatically updated to reflect the committed changes.
meven marked an inline comment as done.