D13358: Add new class that is a model of numbers between two values

[Kai Uwe Broulik]

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