This (and sortRole below) hides the properties from QSortFilterProxy with the same name. Maybe it is a better idea to use filterRoleName/sortRoleName?

Having a sortColumn property would also be useful, since that is apparently missing from QSortFilterProxy. I seem to recall that was added to the Plasma version a while ago.

Or potentially we could just kill our handling of it?

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

QRegExp is deprecated, use setFilterRegularExpression taking a QRegularExpression

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

Given this is a new class/import, remove REVISION

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

Is this needed?

Remove

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

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

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

We could.

Though means everyone doing a lookup immediately, handling columns and roles properly.

More powerful, but slightly more complex for the user

There's also the option of having two proxies with different behaviours?

@mart thoughts?

It's not listed in https://doc.qt.io/qt-5/qtqml-cppintegration-data.html

We need to test

Did some investigation we have our handling just to emit a signal when the property changes!

Another good thing to try to upstream.

Keeping the same name allows us to subtly drop the API in the future and keep clients working.

I think so. We've implemented it in a lot of plasma models, presumably for some reason.

Would be a good thing to try and upstream though.

I don't see why we would need a d pointer. We may as well self an allocation.

Perhaps rename the header to _p.h then?

We haven't done that universally in all frameworks, but sure I could.

Bummer. I tried, a JS RegExp (both new RegExp() and /literal syntax/) get turned into a QRegExp. It does *not* work with QRegularExpression :(

There's a RegularExpressionValidator which uses QRegularExpression but it's in Qt 5.14. Maybe they fixed that recently - doesn't really help us here, though.

Another good thing to try to upstream.

+1 given they're all QObjects already anyway

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 upstreamable since it assumes it's a flast list. Maybe something for QAbstract*List*Model rather than QAbstract*Item*Model

Was just wondering but if it's only to be used from QML and not exported, doesn't need one I suppose.

rowCount being invokable seems enough in the cases I've found in Plasma, will drop this.

*Martin

Did you try that QRegularExpression vs QRegExp thing?

"but you can set it to null" - I think this thing needs a RESET method

needs NOTIFY signals, as do the ones below

override?

Shouldn't this be an int and then you can drop all of that role names stuff?

We did some research

• Old Qt which we support doesn't convert JS to QRegularExpresssion
• We don't want to write new code with QRegExp
• I don't want to expose two properties and have QML doing ifdefs

That doesn't help draw a conclusion. Though is there any benefit to creating a reg exp in JS space rather than here?

what do we gain doing that?

You would use that with
filterRowCallback = undefined;

which is the same as now.

I think RESET is only usable if you can't represent null in the passed data type.

roleNames are always exported. Some people expose the roles as an enum, but not that many

I certainly don't want to encourage magic numbers being used in QML.

In setFilterRegExp(), QSortFilterProxyModel::setFilterRegularExpression(QRegularExpression &) is used, so IIUC here it should be :
QSortFilterProxyModel::filterRegularExpression().pattern()

Hello. Since you're using QRegularExpression, maybe rename to setFilterRegex() or setFitlerRegularExpression() (the latter matches QSortFilterProxyModel naming schema) to avoid QRegExp vs QRegularExpression confusion?

(Another pro for renaming setFilterRegExp() to setFilterRegularExpression()).

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 revision of this diff. Sorry about the noise.

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)

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

Which is preferred: emit or Q_EMIT?

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

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.

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.

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.

Sure, you are right, documentation first. Logic of sortRole uses sortColumn, unit test would be a nice addition :)

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.

Agree, you are right.

Please add:

The default value is -1. If \a sortRole is set, the default value is 0.

This is using the wrong callback, it should be using m_filterColumnCallback

Suggestion: Instead of directly returning the value, do this: (Also for filterAcceptsColumn below)

auto result = const_cast<KSortFilterProxyModel *>(this)->m_filterCallback.call(args);
if (result.isError()) {
    qCWarning(KITEMMODELS_LOG) << "Row filter callback produced an error:";
    qCWarning(KITEMMODELS_LOG) << result.toString();
} else {
    return result.toBool();
}

That way at least some error will be displayed when the callback fails.