This revision was automatically updated to reflect the committed changes.
Closed by commit R275:4fd7a211bb8d: Move Plasmas SortFilterProxyModel
into KItemModels QML plugin (authored by ahiemstra, committed by
davidedmundson).
CHANGED PRIOR TO COMMIT
broulik accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R275 KItemModels
BRANCH
arcpatch-D25326
REVISION DETAIL
https://phabricator.kde.org/D25326
To: davidedmundson, broulik
Cc: kmaterka, iasensio, ahmadsamir, broulik, ahiemstra, mart,
ahiemstra updated this revision to Diff 74601.
ahiemstra added a comment.
- Use QQmlParserStatus to slightly delay syncing of role names
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25326?vs=74592=74601
BRANCH
arcpatch-D25326
REVISION DETAIL
davidedmundson updated this revision to Diff 74592.
davidedmundson marked 14 inline comments as done.
davidedmundson added a comment.
a load of review comments
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25326?vs=71421=74592
BRANCH
master
ahiemstra added inline comments.
INLINE COMMENTS
> ksortfilterproxymodel.cpp:74
> +QJSValueList args = {QJSValue(source_row),
> engine->toScriptValue(source_parent)};
> +return const_cast *>(this)->m_filterCallback.call(args).toBool();
> +}
Suggestion: Instead of directly
ahiemstra added inline comments.
INLINE COMMENTS
> ksortfilterproxymodel.cpp:85
> +QJSValueList args = {QJSValue(source_column),
> engine->toScriptValue(source_parent)};
> +return const_cast *>(this)->m_filterCallback.call(args).toBool();
> +}
This is using the wrong
ahiemstra added a comment.
Something I noticed just now while trying to use this: the `filterString`
property has disappeared. PlasmaCore.SortFilterProxy does have this. I think we
should restore it, otherwise you need to manually call
QSFPM::setFilterFixedString or hope that you can set it
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.
ahiemstra added inline comments.
INLINE COMMENTS
> kmaterka wrote in ksortfilterproxymodel_qml.cpp:111
> 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
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
ahmadsamir added inline comments.
INLINE COMMENTS
> davidedmundson wrote in ksortfilterproxymodel.cpp:101
> I'm not sure your comments are still valid, we switched to using the
> superclass QSortFilterProxyModel's methods, and don't shadow that part here.
Right, I was looking at an old
davidedmundson added inline comments.
INLINE COMMENTS
> ahmadsamir wrote in ksortfilterproxymodel.cpp:101
> In setFilterRegExp(),
> QSortFilterProxyModel::setFilterRegularExpression(QRegularExpression &) is
> used, so IIUC here it should be :
>
ahmadsamir added inline comments.
INLINE COMMENTS
> ksortfilterproxymodel.cpp:101
> +{
> +return QSortFilterProxyModel::filterRegExp().pattern();
> +}
In setFilterRegExp(),
QSortFilterProxyModel::setFilterRegularExpression(QRegularExpression &) is
used, so IIUC here it should be :
davidedmundson updated this revision to Diff 71421.
davidedmundson edited the summary of this revision.
davidedmundson added a comment.
Huge cleanup
Drop all the filter properties
Implement other changes
Extend unit tests
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
davidedmundson added inline comments.
INLINE COMMENTS
> broulik wrote in ksortfilterproxymodel.h:44
> Did you try that `QRegularExpression` vs `QRegExp` thing?
We did some research
- Old Qt which we support doesn't convert JS to QRegularExpresssion
- We don't want to write new code with
broulik added inline comments.
INLINE COMMENTS
> ksortfilterproxymodel.h:2
> +/*
> + * Copyright 2010 by Marco MArtin
> +
*Martin
> ksortfilterproxymodel.h:44
> + */
> +Q_PROPERTY(QString filterRegExp READ filterRegExp WRITE setFilterRegExp
> NOTIFY filterRegExpChanged)
> +
Did
davidedmundson updated this revision to Diff 71329.
davidedmundson added a comment.
Implement new arguments for filterAcceptsRow passing source_row,
source_parent.
It makes for slightly heavier messier JS, but it also allows for super
flexibility
beyond just having the value retried
davidedmundson updated this revision to Diff 71327.
davidedmundson added a comment.
update
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25326?vs=71326=71327
BRANCH
master
REVISION DETAIL
https://phabricator.kde.org/D25326
AFFECTED FILES
davidedmundson updated this revision to Diff 71326.
davidedmundson added a comment.
All changes except the parameters of filterRowCallback
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25326?vs=69918=71326
BRANCH
master
REVISION DETAIL
davidedmundson added inline comments.
INLINE COMMENTS
> broulik wrote in sortfiltermodel.h:78
> > presumably for some reason.
>
> Either because someone was too lazy to use the respective property on the
> `Repeater` or `ListView` or before `rowCount()` was `Q_INVOKABLE`. Not sure
> it's
davidedmundson marked an inline comment as done.
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
davidedmundson updated this revision to Diff 69918.
davidedmundson marked 3 inline comments as done.
davidedmundson added a comment.
asdf
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D25326?vs=69917=69918
BRANCH
master
REVISION DETAIL
broulik added inline comments.
INLINE COMMENTS
> davidedmundson wrote in sortfiltermodel.h:43
> It's not listed in https://doc.qt.io/qt-5/qtqml-cppintegration-data.html
>
> We need to test
Bummer. I tried, a JS `RegExp` (both `new RegExp()` and `/literal syntax/`) get
turned into a `QRegExp`.
davidedmundson updated this revision to Diff 69917.
davidedmundson edited the summary of this revision.
davidedmundson added a comment.
Only some changes done, not all
Updating now to share added unit test
REPOSITORY
R275 KItemModels
CHANGES SINCE LAST UPDATE
davidedmundson marked 3 inline comments as done.
davidedmundson added inline comments.
INLINE COMMENTS
> broulik wrote in sortfiltermodel.cpp:84
> Can we also just have it send `source_row` and `source_parent` since we have
> proper `QModelIndex` handling now?
We could.
Though means everyone
broulik added inline comments.
INLINE COMMENTS
> sortfiltermodel.cpp:84
> +QQmlEngine *engine = QQmlEngine::contextForObject(this)->engine();
> +args <<
> engine->toScriptValue(idx.data(m_roleIds.value(m_filterRole)));
> +
Can we also just have it send `source_row` and
davidedmundson added inline comments.
INLINE COMMENTS
> ahiemstra wrote in sortfiltermodel.h:63
> This (and sortRole below) hides the properties from QSortFilterProxy with the
> same name. Maybe it is a better idea to use filterRoleName/sortRoleName?
Or potentially we could just kill our
ahiemstra added inline comments.
INLINE COMMENTS
> sortfiltermodel.h:63
> + */
> +Q_PROPERTY(QString filterRole READ filterRole WRITE setFilterRole)
> +
This (and sortRole below) hides the properties from QSortFilterProxy with the
same name. Maybe it is a better idea to use
davidedmundson edited the summary of this revision.
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D25326
To: davidedmundson
Cc: mart, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
mart added a comment.
+1: i found myself many times wanting to use this but not being able because
of plasma dep.
the code is used a lot in plasmoids and seems quite robust
REPOSITORY
R275 KItemModels
REVISION DETAIL
https://phabricator.kde.org/D25326
To: davidedmundson
Cc: mart,
davidedmundson created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
davidedmundson requested review of this revision.
REVISION SUMMARY
It exposes QSFPM in a usable way from QML, but also exposes a way to
perform JS callbacks as an
31 matches
Mail list logo
