New Desktop Theme KCM
It is in new directory (desktoptheme-qml) for easier review.
Details
- Reviewers
sebas - Group Reviewers
Plasma - Commits
- R119:cf0f5b5e141c: New Desktop Theme KCM
Diff Detail
- Repository
- R119 Plasma Desktop
- Branch
- desktoptheme-kcm (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
Looks really nice already! Good job!
Some comments inline.
kcms/desktoptheme-qml/kcm.cpp | ||
---|---|---|
62 | QStringLiteral / QLatin1String (I still have trouble deciding which to use when); also on the following lines | |
70 | items in m_theme are parented to this, so this line shouldn't be needed | |
97 | popup() or show() instead of exec() as to not block? | |
124 | Showing the exit code reminds me a lot of windows 98, can we show a meaningful error message instead? | |
173 | whitespace, space after Q_FOREACH (same on a couple of other lines) | |
178 | Can be simplified by adding QDir::NoDotAndDotDot to the entryList flags | |
188 | const | |
190 | const | |
204 | remove | |
240 | perhaps on one line? (Not really critical, if you prefer it like this, ignore this comment) | |
245 | const | |
248 | merge this and the following line, make it const | |
260 | exit code should be user-readable (see also above) | |
kcms/desktoptheme-qml/kcm.h | ||
40 | I understand that enum class makes this type-safe (i.e. will forbid casting to/from int) | |
kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml | ||
30 | Why's that? | |
103 | units.smallSpacing? | |
116 | use units.smallSpacing | |
kcms/desktoptheme-qml/package/contents/ui/main.qml | ||
150 | i18n() | |
156 | i18n() | |
162 | i18n() | |
191 | we have a couple of those, would be good to at least define this interval somewhere centrally, so we get less hardcoded values all over the code | |
kcms/desktoptheme-qml/package/metadata.desktop | ||
5 | Encoding key is deprecated, defaults to UTF-8 anyway | |
6 | should not be empty (it's used for search in systemsettings) | |
19 | can be removed |
Fix issues
Remove shadow from clock hands + add center screw
Always show vertical scrollbar to fix binding loop on width
kcms/desktoptheme-qml/kcm.h | ||
---|---|---|
42 ↗ | (On Diff #2682) | It is actually needed to cast to int in model::roleNames() |
kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml | ||
32 ↗ | (On Diff #2682) | Because air's background svg has huge margins. |
kcms/desktoptheme-qml/package/contents/ui/main.qml | ||
193 ↗ | (On Diff #2682) | Good idea, I think it would make sense to have it in kdeclarative. |
kcms/desktoptheme-qml/package/metadata.desktop | ||
8 ↗ | (On Diff #2682) | Actually, this is metadata for kpackage and not used from KCM. So I think this line can just be removed. |
Good stuff, David! :)
kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml | ||
---|---|---|
32 ↗ | (On Diff #2682) | Please add a note in the code for that, then |