D24846: Port kcm icons to kconfigxt

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  Looks like you didn't address a faire number of comments, or they ended up in the wrong commit (by the look of D24847 <https://phabricator.kde.org/D24847>).

INLINE COMMENTS

> main.cpp:137
>  {
> -    return m_iconSizes[group];
> -}
> -
> -void IconModule::setIconSize(int group, int size)
> -{
> -    if (iconSize(group) == size) {
> -        return;
> +    QString themeName(m_model->selectedTheme());
> +    if (!m_kicon_theme_map.contains(m_model->selectedTheme())) {

nitpick, I find = more readable in such a context (and less prone to the most vexing parse since you don't use curly braces init).

I'd write: const auto themeName = m_model->selectedTheme();

> ervin wrote in main.cpp:102
> Now that my ManagedConfigModule change landed, you should use a proper compile time check connect to settingsChanged here.

I still think those connects might not be necessary.

> ervin wrote in main.cpp:110
> Don't do foreach. Instead write:
> 
>   for (auto theme : qAsConst(m_iconThemeCache)) {
> 
> Or even better, just use qDeleteAll:
> 
>   qDeleteAll(m_iconThemeCache)
> 
> Or even better yet : try to use std::unique_ptr, std::shared_ptr or QScopedPointer as values in your associative container (I let you check which one fits best).

Still not addressed... mind your for loops...

> main.h:117
>  
> -    QVector<int> m_iconSizes;
> +    QMap<QString, KIconTheme*> m_kicon_theme_map;
>  

Still not addressed... s/m_kicon_theme_map/m_iconThemeMap/ (we do camel case here) and should be a QHash.

> ervin wrote in main.h:34
> s/QMap/QHash/

Still not addressed, use a QHash

> ervin wrote in main.h:83
> Killing the const here is semantically wrong IMO.

Still not addressed, put the const back

> ervin wrote in IconSizePopup.qml:81
> I think I'd have expected that logic on the C++ side actually. Others might disagree. :-)

Not on the C++ side still?

> ervin wrote in IconSizePopup.qml:91
> Urgh, I'd assert than silently swallow that I think.

Still not addressed

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24846

To: bport, ervin, mart, #plasma, crossi
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191129/ea3dfa37/attachment-0001.html>