[KCM/Desktop] Port from KQuickAddons::ConfigModule to ManagedConfigModule
ClosedPublic

Authored by meven on Dec 11 2019, 12:42 PM.

Details

Test Plan

kcmshell5 kcm_kwin_virtualdesktops
kcm works as before

Diff Detail

Repository
R108 KWin
Branch
use-kconfigxt-in-qml
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 19924
Build 19942: arc lint + arc unit
meven created this revision.Dec 11 2019, 12:42 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 11 2019, 12:42 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
meven requested review of this revision.Dec 11 2019, 12:42 PM
ervin requested changes to this revision.Dec 11 2019, 12:46 PM
ervin added inline comments.
kcmkwin/kwindesktop/virtualdesktops.cpp
76

The opening curly braces should be on their own line.

93–94

Likely not needed anymore.

168

The whole function is likely unneeded now

This revision now requires changes to proceed.Dec 11 2019, 12:46 PM
meven updated this revision to Diff 71272.Dec 11 2019, 1:21 PM
meven marked 2 inline comments as done.

Remove uneeded setNeedsSave, code style

meven added inline comments.Dec 11 2019, 1:22 PM
kcmkwin/kwindesktop/virtualdesktops.cpp
168

It is needed m_animationsModel and m_desktopsModel are not handled by ManagedConfigModule (they are not KCoreConfigSkeleton but QAbstractItemModel and QAbstractListModel).
EffectsModel (base class of AnimationsModel) could be ported to KConfigXT I think.

crossi added a subscriber: crossi.Dec 11 2019, 1:31 PM
ervin added inline comments.Dec 11 2019, 1:48 PM
kcmkwin/kwindesktop/virtualdesktops.cpp
168

This is fragile still, please try to override isDefaults and isSaveNeeded instead.

Some qml nitpicks on signal handler

kcmkwin/kwindesktop/package/contents/ui/main.qml
177

Consider using onToggled, like other checkbox below.

254

Consider using onValueModified instead which fires when modified by user.

meven updated this revision to Diff 71652.Dec 16 2019, 8:30 AM
meven marked 2 inline comments as done.

Fix AnimationModel and DesktopModel to have defaults/isDefault methods, add isDefaults and isSaveNeeded to VirtualDesktops, use onToggled in qml

Much better with isDefaults and isSaveNeeded override.

kcmkwin/kwindesktop/virtualdesktops.cpp
170

Could provide more simpler and nicer implementation like isDefaults() above, something like :

return m_desktopsModel->needsSave() || m_animationsModel->needsSave();
bport added a subscriber: bport.Dec 16 2019, 8:53 AM
bport added inline comments.
kcmkwin/kwindesktop/virtualdesktops.cpp
170

I think we can simplify the whole method by

return m_desktopsModel->needsSave() || m_animationsModel->needsSave();

meven updated this revision to Diff 71655.Dec 16 2019, 8:54 AM
meven marked an inline comment as done.

Simplify isSaveNeeded

meven marked 4 inline comments as done.Dec 16 2019, 8:55 AM
meven updated this revision to Diff 71663.Dec 16 2019, 12:05 PM

Lower case first letter of entries names in kcfg, fix immutable state read for rollOverDesktops

Still some minor issue, looking good otherwise.

kcmkwin/kwindesktop/desktopsmodel.h
96

should be const

99

should be const

meven updated this revision to Diff 71705.Dec 17 2019, 8:50 AM
meven marked 2 inline comments as done.

const a couple functions

zzag added a subscriber: zzag.Dec 17 2019, 7:06 PM
zzag added inline comments.
kcmkwin/kwindesktop/desktopsmodel.cpp
379

Put the opening brace on the next line.

meven updated this revision to Diff 71755.Dec 18 2019, 8:05 AM
meven marked an inline comment as done.

A couple opening braces on new lines

zzag added a comment.Dec 20 2019, 3:17 PM

This patch looks good to me.

kcmkwin/kwindesktop/animationsmodel.cpp
140

Heh, it took a couple of minutes to decipher what this method is doing. Could you please add an explanatory comment why it's okay to check only EnabledByDefaultRole and not StatusRole?

meven updated this revision to Diff 71899.Dec 20 2019, 3:41 PM

Add comment

ervin requested changes to this revision.Dec 23 2019, 10:23 AM

Just a minor stylistic issue I missed the last time

kcmkwin/kwindesktop/animationsmodel.cpp
141

Missing space after the comma.

This revision now requires changes to proceed.Dec 23 2019, 10:23 AM
meven updated this revision to Diff 72064.Dec 23 2019, 11:18 AM
meven marked an inline comment as done.

Add space after comma

ervin accepted this revision.Dec 23 2019, 11:25 AM
This revision is now accepted and ready to land.Dec 23 2019, 11:25 AM
zzag accepted this revision.Dec 23 2019, 11:29 AM
This revision was automatically updated to reflect the committed changes.