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

Authored by davidedmundson on Jun 5 2018, 9:57 AM.

Details

Reviewers
vkrause
Summary

In QML a lot of code is built up on list models.

One thing I found tedious 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

Diff Detail

Repository
R275 KItemModels
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
davidedmundson created this revision.Jun 5 2018, 9:57 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 5 2018, 9:57 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
davidedmundson requested review of this revision.Jun 5 2018, 9:57 AM
davidedmundson edited the summary of this revision. (Show Details)Jun 5 2018, 10:00 AM
markg added a subscriber: markg.Jun 5 2018, 12:18 PM

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.

src/knumbermodel.cpp
25

QDebug is unused.

35

Drop one space..

115

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().

122

Won't this give compile warnings? It's double and int foo mixed.
Also, the +1 should probably be "+ d->step"

141–144

This can be compile-time generated.

davidedmundson added a comment.EditedJun 5 2018, 12:33 PM

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 loops.

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.

src/knumbermodel.cpp
122

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

Just saw I forgot to submit those comments yesterday

autotests/knumbermodeltest.cpp
27

QVariant(QStringLiteral("3"))?

58

This probably fails in non-English locale?

src/knumbermodel.cpp
104

Only DisplayRole changes

118

Coding style, new line

141
return QHash<int, QByteArray>{
    {DisplayRole, QByteArrayLiteral("display")
...
src/knumbermodel.h
3

Missing email

38

@since

39

Coding style, space before colon

42

Should those be minimumValue, maximumValue, stepSize to match Qt's APIs?

45

Does this work, given the enum is defined elsewhere? I had huge trouble when I did time formatting in KFormat's QML proxy

49

override

52

Name those ...Role since it's not an enum class?

94

Coding style

95

int role = Qt::DisplayRole?

davidedmundson marked 11 inline comments as done.Jun 6 2018, 12:05 PM

In all the documentation blocks you miss the argument and return value documentation.

Are there any instances where that would provide any added value?

autotests/knumbermodeltest.cpp
27

Autotest has cast from ascii turned on as we don't care about minor performance here, it ends up being the same code.

src/knumbermodel.cpp
122

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.

davidedmundson marked an inline comment as done.

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.

vkrause accepted this revision.Jul 6 2018, 12:48 PM
This revision is now accepted and ready to land.Jul 6 2018, 12:48 PM

ping? this is needed for scratch/davidedmundson/kirigami-addons