kmaterka added inline comments. INLINE COMMENTS
> ahiemstra wrote in ksortfilterproxymodel_qml.cpp:111 > 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. Sure, you are right, documentation first. Logic of sortRole uses sortColumn, unit test would be a nice addition :) > ahiemstra wrote in ksortfilterproxymodel.cpp:165 > 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. sortColumn = -1 comes directly from QSortFilterProxyModel and it has use case (disables sorting). In `PlasmaCore.SortFilterModel` this worked incorrectly (or confusingly), even if you set sortRole it is not sorting. This is good now and I like it. I added separate comment for documenting this. > ahiemstra wrote in ksortfilterproxymodel.h:67 > 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. Agree, you are right. > ksortfilterproxymodel.h:86 > + * Specify which column shoud be used for sorting > + */ > + Q_PROPERTY(int sortColumn READ sortColumn WRITE setSortColumn NOTIFY > sortColumnChanged) Please add: The default value is -1. If \a sortRole is set, the default value is 0. 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
