D13358: Add new class that is a model of numbers between two values
This revision was automatically updated to reflect the committed changes. Closed by commit R275:4b1e1b44ef9f: Add new class that is a model of numbers between two values (authored by davidedmundson). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D13358?vs=35681=69812#toc REPOSITORY R275 KItemModels CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13358?vs=35681=69812 REVISION DETAIL https://phabricator.kde.org/D13358 AFFECTED FILES autotests/CMakeLists.txt autotests/knumbermodeltest.cpp src/core/CMakeLists.txt src/core/knumbermodel.cpp src/core/knumbermodel.h To: davidedmundson, vkrause Cc: broulik, markg, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
vkrause added a comment. ping? this is needed for scratch/davidedmundson/kirigami-addons REPOSITORY R275 KItemModels BRANCH master REVISION DETAIL https://phabricator.kde.org/D13358 To: davidedmundson, vkrause Cc: broulik, markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
vkrause accepted this revision. This revision is now accepted and ready to land. REPOSITORY R275 KItemModels BRANCH master REVISION DETAIL https://phabricator.kde.org/D13358 To: davidedmundson, vkrause Cc: broulik, markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
davidedmundson updated this revision to Diff 35681. davidedmundson marked an inline comment as done. davidedmundson added a comment. Most review comments As for the locale enum, I'm not sure. I'm working on a QML module and I'm trying to resolve any QML specific issues there rather than complicate these classes. I'll get that onto phab and we can see what ends up being cleanest overall. REPOSITORY R275 KItemModels CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D13358?vs=35599=35681 BRANCH master REVISION DETAIL https://phabricator.kde.org/D13358 AFFECTED FILES autotests/CMakeLists.txt autotests/knumbermodeltest.cpp src/CMakeLists.txt src/knumbermodel.cpp src/knumbermodel.h To: davidedmundson Cc: broulik, markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
davidedmundson marked 11 inline comments as done.
davidedmundson added a comment.
> In all the documentation blocks you miss the argument and return value
documentation.
Are there any instances where that would provide any added value?
INLINE COMMENTS
> broulik wrote in knumbermodeltest.cpp:26
> `QVariant(QStringLiteral("3"))`?
Autotest has cast from ascii turned on as we don't care about minor performance
here, it ends up being the same code.
> davidedmundson wrote in knumbermodel.cpp:121
> right, the max should be 0.
>
> The +1 is for the first entry
>
> i.e
> min=5 max=5 = 1 starting point
> min=5 max=7 = 1 starting poing + 2 complete steps == 3 rows
Edit, remembered why I did this
surprisingly std::floor(double) returns a double.
so I can either leave as-is with the implicit cast of an int , or use qFloor.
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D13358
To: davidedmundson
Cc: broulik, markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
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 ) const {
> +if (index.parent().isValid()) {
Coding style, new line
> knumbermodel.cpp:140
> +{
> +QHash roleNames;
> +roleNames[Display] = QByteArrayLiteral("display");
return QHash{
{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 =QModelIndex()) const override;
> +QVariant data(const QModelIndex , int role) const override;
Coding style
> knumbermodel.h:94
> +int rowCount(const QModelIndex =QModelIndex()) const override;
> +QVariant data(const QModelIndex , int role) const override;
> +QHash 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
D13358: Add new class that is a model of numbers between two values
davidedmundson added a comment.
For any C++ project you're right that you'd use that or even a basic for loop
instead of a model.
But when writing declaratively means we shouldn't be doing programatic lopos.
For a KDE project I wanted a QtQuickControls2 Tumbler to show year selection
something like 1970 -> 2100 or whatever.
You can do Tumbler {model: 130 delegate: Text{text:1970+model.value}}
but then you have to do a whole fuss getting the value back, it's totally
doable but not very clean.
For a home project I had a repeater with a bunch of buttons 1->4 which again
required the whole "text: index + 1" or a JS for loop creating items.
INLINE COMMENTS
> markg wrote in knumbermodel.cpp:121
> Won't this give compile warnings? It's double and int foo mixed.
> Also, the +1 should probably be "+ d->step"
right, the max should be 0.
The +1 is for the first entry
i.e
min=5 max=5 = 1 starting point
min=5 max=7 = 1 starting poing + 2 complete steps == 3 rows
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D13358
To: davidedmundson
Cc: markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
markg added a comment.
This basically is the equivalent of the C++ range iterator
(https://github.com/ryanhaining/cppitertools#range, but more libraries have a
"range" iterator like that).
In all the documentation blocks you miss the argument and return value
documentation.
I'm curious though, in which situation did you need a model like this?
A situation i can think of is where you would always want to show x number of
items (say for instance a top 10 downloads) and just leve the entries blank
(but have the index numbers) if there isn't enough to fill the top 10.
INLINE COMMENTS
> knumbermodel.cpp:24
> +#include
> +#include
> +
QDebug is unused.
> knumbermodel.cpp:34
> +qreal step = 1.0;
> +QLocale::NumberOptions formattingOptions =
> QLocale::DefaultNumberOptions;
> +};
Drop one space..
> knumbermodel.cpp:114
> +{
> +return d->min + d->step * index.row();
> +}
This is only guaranteed to be a valid index if the callers call it with a valid
index.
Better be safe then sorry and check for isValid().
> knumbermodel.cpp:121
> +}
> +return std::max(0.0, std::floor((d->max - d->min) / d->step)) + 1;
> +}
Won't this give compile warnings? It's double and int foo mixed.
Also, the +1 should probably be "+ d->step"
> knumbermodel.cpp:140-143
> +QHash roleNames;
> +roleNames[Display] = QByteArrayLiteral("display");
> +roleNames[Value] = QByteArrayLiteral("value");
> +return roleNames;
This can be compile-time generated.
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D13358
To: davidedmundson
Cc: markg, kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
davidedmundson edited the summary of this revision. REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D13358 To: davidedmundson Cc: kde-frameworks-devel, michaelh, ngraham, bruns
D13358: Add new class that is a model of numbers between two values
davidedmundson created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: kde-frameworks-devel. davidedmundson requested review of this revision. REVISION SUMMARY In QML a lot of code is built up on list models. One thing I found is a model of integers that goes N...M, especially for use in Tumblers and Repeaters. I've needed it on more than 2 occasions so tidied it up for frameworks. TEST PLAN Unit test REPOSITORY R275 KItemModels BRANCH master REVISION DETAIL https://phabricator.kde.org/D13358 AFFECTED FILES autotests/CMakeLists.txt autotests/knumbermodeltest.cpp src/CMakeLists.txt src/knumbermodel.cpp src/knumbermodel.h To: davidedmundson Cc: kde-frameworks-devel, michaelh, ngraham, bruns
