Delegate KCM cursor theme config management to KConfigXT
ClosedPublic

Authored by bport on Oct 9 2019, 2:36 PM.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Oct 9 2019, 2:36 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 9 2019, 2:36 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Oct 9 2019, 2:36 PM
davidedmundson added inline comments.
kcms/cursortheme/package/contents/ui/main.qml
105

I think this will be broken if you change this combo box, then change theme.

When a user explicitly changes this combo box this binding gets broken with currentIndex now reflecting an explicit number.

On theme change cursorThemeSettings.cursorSize changes correctly but our binding is gone.

This might have to be a connection again

bport added inline comments.Oct 9 2019, 6:38 PM
kcms/cursortheme/package/contents/ui/main.qml
105

After testing seems to be not broken.
the binding is set to a method that transorm current size value to index (kcm.cursorSizeIndex do that)

ervin requested changes to this revision.Oct 9 2019, 8:29 PM
ervin added inline comments.
kcms/cursortheme/kcmcursortheme.cpp
344

int, not const inst

358

s/row/index/

kcms/cursortheme/kcmcursortheme.h
50

This is rather unusual, could we have a full fledged property here despite the boilerplate? I'm thinking of the future developer who might get bitten when changing the QML code and having a half working property.

80

int, not const int

81

s/row/index/ I'd say.

83

nitpick: Please add an empty line after that one.

kcms/cursortheme/package/contents/ui/main.qml
105

David has a point, the binding of currentIndex will be broken as soon as the user changes the entry in the combo box.

That being said, IIRC this shouldn't matter much in that context though, because we seem to be just after pre-selecting the right entry in the combo box when the module is displayed and not always enforcing a currentIndex value (which we can't in the context of combo box as David pointed out).

What matters is that the settings get properly updated when the user select a new entry in the combo box which is handled with onActivated just fine.

kcms/cursortheme/xcursor/sortproxymodel.h
25 ↗(On Diff #67561)

Why is that include added? This doesn't seem necessary to me.

This revision now requires changes to proceed.Oct 9 2019, 8:29 PM
bport updated this revision to Diff 67606.Oct 10 2019, 1:49 PM

Take in consideration feedbacks

bport marked 2 inline comments as done.Oct 10 2019, 1:53 PM
ervin requested changes to this revision.Oct 15 2019, 8:52 AM
ervin added inline comments.
kcms/cursortheme/kcmcursortheme.cpp
160–161

Once you got the NOTIFY part of the property it'll be emitted from here.

kcms/cursortheme/kcmcursortheme.h
50

Should also have the NOTIFY part.

This revision now requires changes to proceed.Oct 15 2019, 8:52 AM
bport marked 2 inline comments as done.Oct 15 2019, 9:23 AM
bport updated this revision to Diff 67949.Oct 15 2019, 9:26 AM

Add notify to setPreferredSize

ervin accepted this revision.Oct 15 2019, 9:28 AM
This revision is now accepted and ready to land.Oct 15 2019, 9:28 AM
This revision was automatically updated to reflect the committed changes.