D24846: Port kcm icons to kconfigxt

[Kevin Ottens]

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