ahiemstra added inline comments. INLINE COMMENTS
> kmaterka wrote in ksortfilterproxymodel_qml.cpp:111 > Can you add a test for sortColumn? It might be useful for newcomers (like me) > to understand how it works. I had real trouble understanding when it is > needed and when not (ConfigEntries.qml > <https://phabricator.kde.org/source/plasma-workspace/browse/master/applets/systemtray/package/contents/ui/ConfigEntries.qml;v5.17.90$83>) I don't think a unit test is the right place for example code. It's probably better to either improve the documentation of the property or add an example somewhere where it makes sense. > kmaterka wrote in ksortfilterproxymodel.cpp:165 > When using PlasmaCore.SortFilterModel sortColumn is sometimes set to -1 and > sorting is not working when `sortColumn` is not set. If I remember correctly, > -1 is the default in QSortFilterProxyModel. Is `std::max(sortColumn(), 0)` > added to fix that? What will happen in this situation: > > KSortFilterProxyModel { > sourceModel: testModel > sortColumn: -1 > sortRole: "user" > } > > Maybe is should be documented in `sortRole` and `sortOrder` properties that > these two set sortColumn to 0 (or -1 in case of empty role)? In that example, you'd be sorting on the "user" role of column 0, which seems like a reasonable default to me. I would actually suggest to make sortColumn always at least 0, I don't think there really is much of a use case for -1 as default. > kmaterka wrote in ksortfilterproxymodel.h:67 > can we have something similar for sorting? `sortColumnCallback` and use it in > overridden `lessThan`? Let's keep this a bit constrained, if we add a stub implementation of `lessThan()`, we can later on add a property to expose a JS callback for it somehow. REPOSITORY R275 KItemModels REVISION DETAIL https://phabricator.kde.org/D25326 To: davidedmundson Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
