It works for the effect
Details
- Reviewers
crossi ervin zzag - Group Reviewers
KWin - Commits
- R108:cc4d191a949c: [KCM/Effects] Allow the user to know when the settings are set to default
kcmshell5 kcm_kwin_effects
Change some settings, the "Restore defaults" button is enabled when the state is not the default state.
Open an effect configuration, the "Restore defaults" button is enabled when the settings are not default.
Diff Detail
- Repository
- R108 KWin
- Branch
- kcm-effects-isDefault
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 20002 Build 20020: arc lint + arc unit
Other than issues pointed out in inline comments, this patch looks good to me.
kcmkwin/common/effectsmodel.cpp | ||
---|---|---|
571 | No else after a return statement. | |
663 | God I wish we had good support for clang-format. Could you please remove whitespace between [=] and (bool defaulted)? Sorry for annoying you with whitespace issues. :/ | |
kcmkwin/kwineffects/kcm.cpp | ||
55 | Do we actually need this connect? I assume that signal dataChanged is emitted when the model is loaded. |
kcmkwin/kwineffects/kcm.cpp | ||
---|---|---|
55 | dataChanged signal is only emitted in EffectsModel::setData |
kcmkwin/common/effectsmodel.cpp | ||
---|---|---|
571 | This inline comment hasn't been addressed. I asked you to remove a else after a return statement, i.e. if (effect.enabledByDefaultFunction && effect.status != Status::EnabledUndeterminded) { return false; } if ((bool)effect.status != effect.enabledByDefault) { return false; } return true; | |
kcmkwin/kwineffects/kcm.cpp | ||
55 | Oh, you're right! You could also consider using one of QAbstractItemModel's signals instead, e.g. modelReset. Perhaps EffectsModel::loaded() should not exist in the first place. |
kcmkwin/common/effectsmodel.cpp | ||
---|---|---|
571 | I misunderstood what you meant, in my last change I added : } else { return true; } Should be good now |
kcmkwin/common/effectsmodel.cpp | ||
---|---|---|
572 | Agreed, using it in modern C++ code is not good but on the other hand we have (bool)effect.status != effect.enabledByDefault just right above this method. If my memory serves me well, the c-style cast is leftover from the monolithic compositing kcm. This minor issue can be addressed in another patch though... |
kcmkwin/common/effectsmodel.cpp | ||
---|---|---|
572 | Sure, can only be in another patch anyway, that one got merged anyway. ;-) |