D12459: [Icon KCM] Port to new design

[Eike Hein]

hein added a comment.


  In general: This code would be cleaner if it wasn't using QStandardItemModel but just a QAbstractListModel subclass. Then stuff like the "selected theme index" could use either a role or a QItemSelectionModel, and the role enum wouldn't need to live outside of the model. :)
  
  Otherwise it looks quite good.

INLINE COMMENTS

> main.cpp:68
> +    : KQuickAddons::ConfigModule(parent, args)
> +    , m_iconGroups{
> +        QStringLiteral("Desktop"),

Do we have these strings somewhere in KIcon* so we don't need to duplicate them?

> main.cpp:96
> +    m_model->setItemRoleNames({
> +        {Qt::DisplayRole, QByteArrayLiteral("display")},
> +        {DescriptionRole, QByteArrayLiteral("description")},

DisplayRole is usually already defined, are you sure you need to duplicate it here?

In Plasma models we usually use uppercase role names btw.

> main.h:26
>  
> +#pragma once
>  

Is this legal in KDE code?

> IconSizePopup.qml:64
> +                Layout.fillHeight: true
> +                Layout.preferredHeight: iconTypeList.count * measureDelegate.height + 4
> +                activeFocusOnTab: false

Can you explain this better? Measure items and fixed pixel values raise red flags :)

> IconSizePopup.qml:90
> +                        highlighted: ListView.isCurrentItem
> +                        text: [
> +                            i18n("Desktop"),

Can we get rid of these copies too?

> IconSizePopup.qml:127
> +
> +                        // I have no idea what this code does but it works and is just copied from the old KCM
> +                        var index = -1;

I think it's trying to correct sizes being off by a fixed offset. But why is this necessary? What breaks if you remove this? This might not be a problem in 2018.

> main.qml:83
> +            Timer {
> +                id: thumbTimer
> +                interval: 1000

This timer seems to be unused?

> main.qml:121
> +
> +                Component.onCompleted: {
> +                    // avoid reloading it when icon sizes or dpr changes on startup

We usually have these at the bottom (this confused me while reading because I thought the Repeater was outside the Flow)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, #vdg
Cc: hein, zzag, abetts, ngraham, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, sebas, apol, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20180425/13e98626/attachment-0001.html>