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
