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
Branch
desktoptheme-kcm (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
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

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

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
42

It is actually needed to cast to int in model::roleNames()

kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml
32

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
193

Good idea, I think it would make sense to have it in kdeclarative.

kcms/desktoptheme-qml/package/metadata.desktop
8

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
32

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.