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
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Looks really nice already! Good job!
Some comments inline.
kcms/desktoptheme-qml/kcm.cpp | ||
---|---|---|
62 ↗ | (On Diff #2721) | QStringLiteral / QLatin1String (I still have trouble deciding which to use when); also on the following lines |
70 ↗ | (On Diff #2721) | items in m_theme are parented to this, so this line shouldn't be needed |
97 ↗ | (On Diff #2721) | popup() or show() instead of exec() as to not block? |
124 ↗ | (On Diff #2721) | Showing the exit code reminds me a lot of windows 98, can we show a meaningful error message instead? |
173 ↗ | (On Diff #2721) | whitespace, space after Q_FOREACH (same on a couple of other lines) |
178 ↗ | (On Diff #2721) | Can be simplified by adding QDir::NoDotAndDotDot to the entryList flags |
188 ↗ | (On Diff #2721) | const |
190 ↗ | (On Diff #2721) | const |
204 ↗ | (On Diff #2721) | remove |
240 ↗ | (On Diff #2721) | perhaps on one line? (Not really critical, if you prefer it like this, ignore this comment) |
245 ↗ | (On Diff #2721) | const |
248 ↗ | (On Diff #2721) | merge this and the following line, make it const |
260 ↗ | (On Diff #2721) | exit code should be user-readable (see also above) |
kcms/desktoptheme-qml/kcm.h | ||
40 ↗ | (On Diff #2721) | 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 ↗ | (On Diff #2721) | Why's that? |
103 ↗ | (On Diff #2721) | units.smallSpacing? |
116 ↗ | (On Diff #2721) | use units.smallSpacing |
kcms/desktoptheme-qml/package/contents/ui/main.qml | ||
150 ↗ | (On Diff #2721) | i18n() |
156 ↗ | (On Diff #2721) | i18n() |
162 ↗ | (On Diff #2721) | i18n() |
191 ↗ | (On Diff #2721) | 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 ↗ | (On Diff #2721) | Encoding key is deprecated, defaults to UTF-8 anyway |
6 ↗ | (On Diff #2721) | should not be empty (it's used for search in systemsettings) |
19 ↗ | (On Diff #2721) | 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 | ||
---|---|---|
41 ↗ | (On Diff #2751) | It is actually needed to cast to int in model::roleNames() |
kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml | ||
31 ↗ | (On Diff #2751) | Because air's background svg has huge margins. |
kcms/desktoptheme-qml/package/contents/ui/main.qml | ||
192 ↗ | (On Diff #2751) | Good idea, I think it would make sense to have it in kdeclarative. |
kcms/desktoptheme-qml/package/metadata.desktop | ||
7 ↗ | (On Diff #2751) | 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 | ||
---|---|---|
31 ↗ | (On Diff #2751) | Please add a note in the code for that, then |