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

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> sortfiltermodel.cpp:84
> +        QQmlEngine *engine = QQmlEngine::contextForObject(this)->engine();
> +        args << engine->toScriptValue<QVariant>(idx.data(m_roleIds.value(m_filterRole)));
> +

Can we also just have it send `source_row` and `source_parent` since we have proper `QModelIndex` handling now?

> sortfiltermodel.cpp:97
> +    }
> +    QSortFilterProxyModel::setFilterRegExp(QRegExp(exp, Qt::CaseInsensitive));
> +    Q_EMIT filterRegExpChanged(exp);

`QRegExp` is deprecated, use `setFilterRegularExpression` taking a `QRegularExpression`

> sortfiltermodel.h:43
> +     */
> +    Q_PROPERTY(QString filterRegExp READ filterRegExp WRITE setFilterRegExp NOTIFY filterRegExpChanged)
> +

Can this be a `QRegularExpression`?
I would assume a JS `RegExp` object (or regexp literal `/syntax/`) was supported (didn't test)

> sortfiltermodel.h:48
> +     */
> +    Q_PROPERTY(QString filterString READ filterString WRITE setFilterString NOTIFY filterStringChanged REVISION 1)
> +

Given this is a new class/import, remove `REVISION`

> sortfiltermodel.h:56
> +     * ignored. Attempts to write a non-callable to this property are silently ignored, but you can set
> +     * it to null.
> +     */

Could this use a `RESET` method instead of telling people to assign `null`?

> sortfiltermodel.h:78
> +     */
> +    Q_PROPERTY(int count READ count NOTIFY countChanged)
> +

Is this needed?

> sortfiltermodel.h:80
> +
> +    friend class DataModel;
> +

Remove

> sortfiltermodel.h:117
> +     */
> +    Q_INVOKABLE QVariantMap get(int i) const;
> +

I don't like this. We have proper `QModelIndex` handling in QML now, this is a hack imho

> sortfiltermodel.h:119
> +
> +    Q_INVOKABLE int mapRowToSource(int i) const;
> +

Remove, `QAbstractProxyModel::mapToSource` (and `fromSource`) is `Q_INVOKABLE` and we have proper `QModelIndex` handling in QML now

> sortfiltermodel.h:138
> +private:
> +    QString m_filterRole;
> +    QString m_sortRole;

Does this need a `d` pointer or is it just for use in QML? Perhaps rename the header to `_p.h` then?

REPOSITORY
  R275 KItemModels

REVISION DETAIL
  https://phabricator.kde.org/D25326

To: davidedmundson
Cc: 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/20191115/d5736dc3/attachment-0001.html>