broulik added a comment.
Just saw I forgot to submit those comments yesterday
INLINE COMMENTS
> knumbermodeltest.cpp:26
> + QCOMPARE(m.rowCount(), 3);
> + QCOMPARE(m.data(m.index(0, 0), Qt::DisplayRole), QVariant("3"));
> + QCOMPARE(m.data(m.index(1, 0), Qt::DisplayRole), QVariant("4"));
`QVariant(QStringLiteral("3"))`?
> knumbermodeltest.cpp:57
> + QCOMPARE(m.rowCount(), 1);
> + QCOMPARE(m.data(m.index(0, 0), Qt::DisplayRole), QVariant("1,000"));
> +
This probably fails in non-English locale?
> knumbermodel.cpp:103
> + }
> + dataChanged(index(0, 0, QModelIndex()), index(rowCount(), 0,
> QModelIndex()));
> + emit formattingOptionsChanged();
Only `DisplayRole` changes
> knumbermodel.cpp:117
> +
> +int KNumberModel::rowCount(const QModelIndex &index) const {
> + if (index.parent().isValid()) {
Coding style, new line
> knumbermodel.cpp:140
> +{
> + QHash<int, QByteArray> roleNames;
> + roleNames[Display] = QByteArrayLiteral("display");
return QHash<int, QByteArray>{
{DisplayRole, QByteArrayLiteral("display")
...
> knumbermodel.h:2
> +/*
> + * Copyright (C) 2018 David Edmundson
> + *
Missing email
> knumbermodel.h:37
> + * Value, the actual value as a number
> + */
> +class KITEMMODELS_EXPORT KNumberModel: public QAbstractListModel
`@since`
> knumbermodel.h:38
> + */
> +class KITEMMODELS_EXPORT KNumberModel: public QAbstractListModel
> +{
Coding style, space before colon
> knumbermodel.h:41
> + Q_OBJECT
> + Q_PROPERTY (qreal min READ min WRITE setMin NOTIFY minChanged)
> + Q_PROPERTY (qreal max READ max WRITE setMax NOTIFY maxChanged)
Should those be `minimumValue`, `maximumValue`, `stepSize` to match Qt's APIs?
> knumbermodel.h:44
> + Q_PROPERTY (qreal step READ step WRITE setStep NOTIFY stepChanged)
> + Q_PROPERTY (QLocale::NumberOptions formattingOptions READ
> formattingOptions WRITE setFormattingOptions NOTIFY formattingOptionsChanged)
> +
Does this work, given the enum is defined elsewhere? I had huge trouble when I
did time formatting in `KFormat`'s QML proxy
> knumbermodel.h:48
> + KNumberModel(QObject *parent = nullptr);
> + ~KNumberModel();
> +
`override`
> knumbermodel.h:51
> + enum Roles {
> + Display = Qt::DisplayRole,
> + Value = Qt::UserRole
Name those `...Role` since it's not an `enum class`?
> knumbermodel.h:93
> +
> + int rowCount(const QModelIndex &index=QModelIndex()) const override;
> + QVariant data(const QModelIndex &index, int role) const override;
Coding style
> knumbermodel.h:94
> + int rowCount(const QModelIndex &index=QModelIndex()) const override;
> + QVariant data(const QModelIndex &index, int role) const override;
> + QHash<int, QByteArray> roleNames() const override;
`int role = Qt::DisplayRole`?
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D13358
To: davidedmundson
Cc: broulik, markg, kde-frameworks-devel, michaelh, ngraham, bruns
