New Desktop Theme KCM
ClosedPublic

Authored by drosca on Mar 10 2016, 9:20 PM.

Details

Reviewers
sebas
Group Reviewers
Plasma
Commits
R119:cf0f5b5e141c: New Desktop Theme KCM
Summary

New Desktop Theme KCM
It is in new directory (desktoptheme-qml) for easier review.

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.
drosca updated this revision to Diff 2682.Mar 10 2016, 9:20 PM
drosca retitled this revision from to New Desktop Theme KCM.
drosca updated this object.
drosca edited the test plan for this revision. (Show Details)
drosca added a reviewer: Plasma.
Restricted Application added a project: Plasma. · View Herald TranscriptMar 10 2016, 9:20 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
drosca updated this revision to Diff 2721.Mar 11 2016, 7:01 PM

Keep the Plasma::Theme in cache
Fix logic of needs save

sebas requested changes to this revision.Mar 12 2016, 5:20 PM
sebas added a reviewer: sebas.
sebas added a subscriber: sebas.

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

This revision now requires changes to proceed.Mar 12 2016, 5:20 PM
drosca updated this revision to Diff 2751.Mar 12 2016, 8:43 PM
drosca edited edge metadata.

Fix issues
Remove shadow from clock hands + add center screw
Always show vertical scrollbar to fix binding loop on width

drosca added inline comments.Mar 12 2016, 8:51 PM
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.
This special case could be removed when we can get transparent margins size from FrameSvg.

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.

sebas accepted this revision.Mar 25 2016, 12:29 PM
sebas edited edge metadata.

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

This revision is now accepted and ready to land.Mar 25 2016, 12:29 PM
This revision was automatically updated to reflect the committed changes.