[Differential] [Requested Changes To] D1113: New Desktop Theme KCM

[sebas (Sebastian Kügler)]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160312/0b9b4b50/attachment.html>