search

oKcerG / SortFilterProxyModel

A nicely exposed QSortFilterProxyModel for QML
MIT License
299 stars 101 forks source link

Add tests infrastructure #18

Closed keithel closed 7 years ago

keithel commented 7 years ago
oKcerG commented 7 years ago

Thanks a lot for the PR, I'm looking at it now. Looks clean and really well written.

keithel commented 7 years ago

I was thinking -1 for unset value, but what if someone wanted a default value? min 0 max 0 as defaults seem reasonable - but need to make sure the filter gets applied.

oKcerG commented 7 years ago

What do you mean by default value ? For me by default it should not filter anything if you don't bind a value in qml. So IndexFilter {} doesn't filter anything out, but IndexFilter { maximumIndex: 0 } would only keep the first element. If we set it to 0 and keep a maximumIndexIsSet bool to know if we should filter on it, we can't differentiate a set value from an unset value in QML.

keithel commented 7 years ago

ahh I see what you mean-- yeah I guess that makes sense - then default values of -1 for both min and max, and special logic to know that scenario

oKcerG commented 7 years ago

I was thinking about using a QVariant so it could be invalid instead of -1, this leaves the possibility to use negative values for indexes starting from the end.

oKcerG commented 7 years ago

I would prefer that a Filter is created before hand for each test if needed rather than created dynamically with Qt.createQmlObject. It would help with the readability. Tests for ListModel are done like that : https://github.com/qt/qtdeclarative/blob/dev/tests/auto/qmltest/listmodel/tst_listmodel.qml

Apart from this point, I'm ready to accept this PR. Sorry for the delay in reviewing it.

oKcerG commented 7 years ago

Thanks for the time taken on this PR and sorry to nitpick again, but I believe we should only use one SortFilterProxyModel and multiple Filters that we assign to the proxy instead. The Filters should be the main focus point of those tests here, bundling each of them in a proxy model kind of hide that.

I'm also not a fan of the Array.prototype.slice.call, it looks cryptic. I would do a property list<Foo> test_foos instead and just return that property.

oKcerG commented 7 years ago

Thanks for the hard work. I rebased your commits to have a clean history. I'll use this as a base for the future tests that needs to be written.