D25449: Start of the new Formats KCM

[Kai Uwe Broulik]

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