sebas requested changes to this revision. sebas added a reviewer: sebas. sebas added a comment. This revision now requires changes to proceed.
Looks really nice already! Good job! Some comments inline. INLINE COMMENTS kcms/desktoptheme-qml/kcm.cpp:62 QStringLiteral / QLatin1String (I still have trouble deciding which to use when); also on the following lines kcms/desktoptheme-qml/kcm.cpp:70 items in m_theme are parented to this, so this line shouldn't be needed kcms/desktoptheme-qml/kcm.cpp:97 popup() or show() instead of exec() as to not block? 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? kcms/desktoptheme-qml/kcm.cpp:173 whitespace, space after Q_FOREACH (same on a couple of other lines) kcms/desktoptheme-qml/kcm.cpp:178 Can be simplified by adding QDir::NoDotAndDotDot to the entryList flags kcms/desktoptheme-qml/kcm.cpp:188 const kcms/desktoptheme-qml/kcm.cpp:190 const kcms/desktoptheme-qml/kcm.cpp:204 remove kcms/desktoptheme-qml/kcm.cpp:240 perhaps on one line? (Not really critical, if you prefer it like this, ignore this comment) kcms/desktoptheme-qml/kcm.cpp:245 const kcms/desktoptheme-qml/kcm.cpp:248 merge this and the following line, make it const kcms/desktoptheme-qml/kcm.cpp: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? kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml:103 units.smallSpacing? kcms/desktoptheme-qml/package/contents/ui/ThemePreview.qml:116 use units.smallSpacing kcms/desktoptheme-qml/package/contents/ui/main.qml:150 i18n() kcms/desktoptheme-qml/package/contents/ui/main.qml:156 i18n() kcms/desktoptheme-qml/package/contents/ui/main.qml:162 i18n() 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 kcms/desktoptheme-qml/package/metadata.desktop:5 Encoding key is deprecated, defaults to UTF-8 anyway kcms/desktoptheme-qml/package/metadata.desktop:6 should not be empty (it's used for search in systemsettings) kcms/desktoptheme-qml/package/metadata.desktop:19 can be removed REPOSITORY rPLASMADESKTOP Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D1113 EMAIL PREFERENCES https://phabricator.kde.org/settings/panel/emailpreferences/ To: drosca, Plasma, sebas Cc: sebas, plasma-devel
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel