Details
- Reviewers
mart ervin - Group Reviewers
Plasma - Commits
- R119:fec538fb02e0: Delegate KCM cursor theme config management to KConfigXT
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.
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 |
kcms/cursortheme/package/contents/ui/main.qml | ||
---|---|---|
105 | After testing seems to be not broken. |
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. |