D25326: Move Plasma's SortFilterProxyModel into KItemModel's QML plugin

[Konrad Materka]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200129/628590ca/attachment-0001.html>