ervin requested changes to this revision.
ervin added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> main.cpp:101
>
> - connect(m_model, &IconsModel::selectedThemeChanged, this, [this] {
> - m_selectedThemeDirty = true;
> - setNeedsSave(true);
> - });
> - connect(m_model, &IconsModel::pendingDeletionsChanged, this, [this] {
> - setNeedsSave(true);
> - });
> + connect(m_model, SIGNAL(selectedThemeChanged()), this,
> SLOT(_k_settingsChanged()));
> + connect(m_model, SIGNAL(pendingDeletionsChanged()), this,
> SLOT(_k_settingsChanged()));
Shouldn't you be able to remove that connect? I'd expect this signal trickling
down to changing the settings object, wouldn't it?
> main.cpp:102
> + connect(m_model, SIGNAL(selectedThemeChanged()), this,
> SLOT(_k_settingsChanged()));
> + connect(m_model, SIGNAL(pendingDeletionsChanged()), this,
> SLOT(_k_settingsChanged()));
>
Now that my ManagedConfigModule change landed, you should use a proper compile
time check connect to settingsChanged here.
> main.cpp:110
> {
> + foreach (KIconTheme* theme, m_kicon_theme_map) {
> + delete theme;
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).
> 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();`
> main.cpp:138
> + QString themeName(m_model->selectedTheme());
> + if (!m_kicon_theme_map.contains(m_model->selectedTheme())) {
> + m_kicon_theme_map.insert(themeName, new KIconTheme(themeName));
Couldn't you fill the cache as soon as the selected theme changes instead? This
way you wouldn't need to modify your cache here.
> main.h:34
> #include <QScopedPointer>
> +#include <QMap>
>
s/QMap/QHash/
> main.h:83
>
> - Q_INVOKABLE int iconSize(int group) const;
> - Q_INVOKABLE void setIconSize(int group, int size);
> - Q_INVOKABLE QList<int> availableIconSizes(int group) const;
> + Q_INVOKABLE QList<int> availableIconSizes(int group);
>
Killing the const here is semantically wrong IMO.
> main.h:117
>
> - QVector<int> m_iconSizes;
> + QMap<QString, KIconTheme*> m_kicon_theme_map;
>
You don't need the keys to be sorted, so please use QHash instead.
Also we do camel case here, not snake case. :-)
> IconSizePopup.qml:81
>
> + property var sizesMap: [
> + {"settingName":"desktopSize", "displayName":
> i18n("Desktop")},
I think I'd have expected that logic on the C++ side actually. Others might
disagree. :-)
> IconSizePopup.qml:91
> + function sizeDisplayName(index) {
> + index = index <0 ? 0 : index;
> + return iconTypeList.sizesMap[index].displayName;
Urgh, I'd assert than silently swallow that I think.
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
