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