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

[Konrad Materka]

kmaterka added a comment.


  Few comments from someone who was recently using `PlasmaCore.SortFilterModel` and had trouble understanding sorting :)

INLINE COMMENTS

> ksortfilterproxymodel_qml.cpp:111
> +
> +void tst_KSortFilterProxyModelQml::testSortRole()
> +{

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

> ksortfilterproxymodel.cpp:165
> +{
> +    sort(std::max(sortColumn(), 0), order);
> +    Q_EMIT sortOrderChanged();

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)?

> ksortfilterproxymodel.cpp:175
> +    sort(column, sortOrder());
> +    emit sortColumnChanged();
> +}

Which is preferred: `emit` or `Q_EMIT`?

> ksortfilterproxymodel.h:67
> +     */
> +    Q_PROPERTY(QJSValue filterColumnCallback READ filterColumnCallback WRITE setFilterColumnCallback NOTIFY filterColumnCallbackChanged)
> +

can we have something similar for sorting? `sortColumnCallback` and use it in overridden `lessThan`?

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/20200120/a387aa35/attachment-0001.html>