broulik added inline comments. INLINE COMMENTS
> Messages.sh:2 > #! /usr/bin/env bash > -$EXTRACTRC *.ui >> rc.cpp > -$XGETTEXT *.cpp -o $podir/kcmformats.pot > -rm -f rc.cpp > - > +$EXTRACTRC `find . -name "*.qml"` >> rc.qml || exit 11 > +$XGETTEXT `find . -name "*.qml"` -o $podir/kcm5_formats.pot I don't think this is necessary, at least it's the first time I have ever seen this sort of thing > kcm.cpp:37 > +{ > + qmlRegisterAnonymousType<FormatsSettings>("kcm.kde.org", 1); > + qmlRegisterAnonymousType<LocaleModel>("kcm.kde.org", 1); You're not importing this anywhere, and this still works? Makes me wonder why Qt went through the trouble of introducing this when it makes no diference over `qmlRegisterType<T>()` ... Please also use a KCM-specific import name for this, such as `org.kde.private.kcms.formats` > kcm.h:39 > + Formats(QObject* parent, const QVariantList& args); > + virtual ~Formats() override; > + `virtual` isn't necessary here > kcm.h:42 > + /* creates example strings with the specified Locale. */ > + void updateStringsExample(); > + Name this `updateExampleStrings` and where is it actually being called from? > kcm.h:48 > +private: > + > + FormatsSettings *m_settings; Remove whitespace > kcm_formats.desktop:14 > +Name=Formats > +Comment=What your kcm is all about? > +X-KDE-Keywords=Colors, Mouse, Monitor Yeah, what is it about? :) > localemodel.cpp:85 > +{ > + Q_UNUSED(parent); > + return m_data.size(); Ensure that it's not trying to make you a tree model: if (parent.isValid()) { return 0; } return m_data.count(); > localemodel.cpp:91 > +{ > + if (!idx.isValid()) { > + return {}; Use `checkIndex(idx)` which will also ensure it is within bounds iirc > localemodel.h:1 > +#ifndef LOCALEMODEL_H > +#define LOCALEMODEL_H Missing copyright header > localemodel.h:8 > +#include <QVector> > +#include <QPair> > +#include <QByteArray> Unused > localemodel.h:16 > + QString localeValue; > +}; > + declare as movable type > localemodel.h:18 > + > +class LocaleModel : public QAbstractListModel { > + Q_OBJECT `{` on new line > localemodel.h:21 > +public: > + enum {Flag = Qt::UserRole + 1, LocaleName, LocaleValue}; > + LocaleModel(QObject *parent = nullptr); Use `Qt::DisplayRole` for `LocaleName` Also, what is this ominous "value"? Please give it a clearer name. Also, coding style. > localemodel.h:22 > + enum {Flag = Qt::UserRole + 1, LocaleName, LocaleValue}; > + LocaleModel(QObject *parent = nullptr); > + `explicit` > main.qml:38 > + textRole: "localeName" > + currentIndex: kcm.model.indexFor(kcm.settings.defaultLanguage) > + `QAbstractItemModel::match` is usable from QML, then you don't need to roll your own `indexFor` method > main.qml:41 > + /* Kirigami basic list item does not work correctly with > ComboBoxex > + delegate: Kirigami.BasicListItem { > + icon: model.flag Have you tried `QQC2.ItemDelegate`? So annoying QQC to this day can't properly do icons for comboboxes :( > main.qml:46 > + */ > + onCurrentIndexChanged: { > + kcm.settings.defaultLanguage = > kcm.model.valueFor(currentIndex) Use `onActivated` > main.qml:47 > + onCurrentIndexChanged: { > + kcm.settings.defaultLanguage = > kcm.model.valueFor(currentIndex) > + } `QAbstractItemModel::data` is usable from QML: var idx = kcm.model.index(currentIndex, 0); kcm.settings.defaultLanguage = kcm.model.data(idx, YourModel.LocaleValueRole); If we're depending on Qt 5.14 there's now also a `valueRole` property you can set and then just use `currentValue` > main.qml:51 > + > + QQC2.CheckBox { > + id: detailedSettings Does this have to be section or something? Iirc FormLayout has a checkable section feature > main.qml:62 > + currentIndex: kcm.model.indexFor(kcm.settings.numeric) > + onCurrentIndexChanged: { > + kcm.settings.numeric = kcm.model.valueFor(currentIndex) Use `onActivated` and everywhere else REPOSITORY R119 Plasma Desktop REVISION DETAIL https://phabricator.kde.org/D25449 To: tcanabrava, ervin Cc: broulik, ervin, davidedmundson, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart