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
