<div>sebas requested changes to this revision.<br />
sebas added a reviewer: sebas.<br />
sebas added a comment.<br />
This revision now requires changes to proceed.</div><br /><div><div><p>Looks really nice already! Good job!</p>

<p>Some comments inline.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div>kcms/desktoptheme-qml/kcm.cpp:62 QStringLiteral / QLatin1String (I still have trouble deciding which to use when); also on the following lines</div><div>kcms/desktoptheme-qml/kcm.cpp:70 items in m_theme are parented to this, so this line shouldn't be needed</div><div>kcms/desktoptheme-qml/kcm.cpp:97 popup() or show() instead of exec() as to not block?</div><div>kcms/desktoptheme-qml/kcm.cpp:124 Showing the exit code reminds me a lot of windows 98, can we show a meaningful error message instead?</div><div>kcms/desktoptheme-qml/kcm.cpp:173 whitespace, space after Q_FOREACH (same on a couple of other lines)</div><div>kcms/desktoptheme-qml/kcm.cpp:178 Can be simplified by adding QDir::NoDotAndDotDot to the entryList flags</div><div>kcms/desktoptheme-qml/kcm.cpp:188 const </div><div>kcms/desktoptheme-qml/kcm.cpp:190 const</div><div>kcms/desktoptheme-qml/kcm.cpp:204 remove</div><div>kcms/desktoptheme-qml/kcm.cpp:240 perhaps on one line? (Not really critical, if you prefer it like this, ignore this comment)</div><div>kcms/desktoptheme-qml/kcm.cpp:245 const</div><div>kcms/desktoptheme-qml/kcm.cpp:248 merge this and the following line, make it const</div><div>kcms/desktoptheme-qml/kcm.cpp:260 exit code should be user-readable (see also above)</div><div>kcms/desktoptheme-qml/kcm.h:40 I understand that enum class makes this type-safe (i.e. will forbid casting to/from int)</div><div>kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml:30 Why's that?</div><div>kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml:103 units.smallSpacing?</div><div>kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml:116 use units.smallSpacing</div><div>kcms/desktoptheme-qml/package/contents/ui/main.qml:150 i18n()</div><div>kcms/desktoptheme-qml/package/contents/ui/main.qml:156 i18n()</div><div>kcms/desktoptheme-qml/package/contents/ui/main.qml:162 i18n()</div><div>kcms/desktoptheme-qml/package/contents/ui/main.qml: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</div><div>kcms/desktoptheme-qml/package/metadata.desktop:5 Encoding key is deprecated, defaults to UTF-8 anyway</div><div>kcms/desktoptheme-qml/package/metadata.desktop:6 should not be empty (it's used for search in systemsettings)</div><div>kcms/desktoptheme-qml/package/metadata.desktop:19 can be removed</div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rPLASMADESKTOP Plasma Desktop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D1113" rel="noreferrer">https://phabricator.kde.org/D1113</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>drosca, Plasma, sebas<br /><strong>Cc: </strong>sebas, plasma-devel<br /></div>