[kcmkwin] Don't discard unsaved changes when reloading effects model
ClosedPublic

Authored by zzag on Feb 3 2019, 5:03 PM.

Details

Summary

If an effect is installed or removed, then all not yet committed changes
will be lost. This is undesired behaviour.

Diff Detail

Repository
R108 KWin
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8625
Build 8643: arc lint + arc unit
zzag created this revision.Feb 3 2019, 5:03 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 3 2019, 5:03 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
zzag requested review of this revision.Feb 3 2019, 5:03 PM
zzag updated this revision to Diff 50888.Feb 4 2019, 9:14 PM

Rebase.

davidedmundson requested changes to this revision.Feb 21 2019, 12:38 PM
davidedmundson added a subscriber: davidedmundson.
davidedmundson added inline comments.
kcmkwin/common/effectmodel.cpp
472–473

I think there's a problem here.

Previously:
m_effectsList is the current state that we think kwin has
m_effectsChanged is what the user has set

we're now losing what we think kwin has

In ::syncEffectsToKWin (line 484 here)
we compare the .effectsStatus of both

If a user makes a change, and then we call reload, and then the user saves:

m_effectsList[0]->effectStatus is the user set value
m_effectsList[0]->changed is true

so we'll write out the correct value in the config file all successfully

but m_effectsList.at(it).effectStatus == m_effectsChanged.at(it).effectStatus so we won't notify kwin.

This revision now requires changes to proceed.Feb 21 2019, 12:38 PM
zzag updated this revision to Diff 52201.Feb 21 2019, 1:01 PM

Check changed in syncEffectsToKWin

zzag updated this revision to Diff 52202.Feb 21 2019, 1:02 PM

wrong diff

zzag planned changes to this revision.Feb 21 2019, 1:03 PM

Argh, I wrecked my diffs.

zzag updated this revision to Diff 52203.Feb 21 2019, 1:07 PM

I hope the diff is now okay

Does this make m_effectsChanged completely redundant now?

zzag added a comment.EditedFeb 21 2019, 1:10 PM

Does this make m_effectsChanged completely redundant now?

Yeah, that was my intention. Should I squash D18706 and D18705? (I didn't put them in one commit because they looked different to me)

davidedmundson accepted this revision.Feb 21 2019, 1:16 PM

Oh, I forgot about D18706

You could have just told me my comment was superseded by another commit and ignored it.
You didn't need to fix this.

Squash, ship as two things, whatever is easiest for you.

This revision is now accepted and ready to land.Feb 21 2019, 1:16 PM
This revision was automatically updated to reflect the committed changes.