sortproxymodel: check if source model is a proxy model

[BLumia]

if the source model is a proxy model, the source model's source model might be changed and our SortProxyModel won't get notified. This might happen when we try to pass a QML-based proxy model as source model, like KDE Framework's KItemModel.KSortFilterProxyModel for example.

[dfaure-kdab]

Doesn't this rather mean that KSortFilterProxyModel forgets to emit modelReset in its own setSourceModel?

It's supposed to tell the world if its own source model changes....

[BLumia]

Doesn't this rather mean that KSortFilterProxyModel forgets to emit modelReset in its own setSourceModel?

I'm actually testing it with a custom proxy model that simply subclassed QSortFilterProxyModel and overrided filterAcceptsRow. modelReset was emitted and SortProxyModel simply didn't connect that signal to update its internal data. Currently SortProxyModel only connected to these three signals:

KDToolBox/qt/model_view/sortProxyModel/sortproxymodel.cpp

Lines 133 to 135 in e3fd152

	connect(model, &QAbstractItemModel::dataChanged, this, &SortProxyModel::handleDataChanged);
	connect(model, &QAbstractItemModel::rowsInserted, this, &SortProxyModel::handleRowsInserted);
	connect(model, &QAbstractItemModel::rowsRemoved, this, &SortProxyModel::handleRowsRemoved);

KSortFilterProxyModel (and likely all proxy models) can also be used to reproduce the issue. I only mentioned that one because it seems that's the one that most Qt developers know.

[dfaure-kdab]

Then the bug is that SortProxyModel doesn't connect to modelReset.
This would be a much cleaner fix than connecting to the source's source.

[BLumia]

Then the bug is that SortProxyModel doesn't connect to modelReset.

Currently, when using SortProxyModel, developer can still reset the model freely and the row move signals for QML is still emitted without any issue. I guess that might be one of the intended usages?

Documentation of QAbstractItemModel::beginResetModel():

When a model radically changes its data it can sometimes be easier to just call this function rather than emit dataChanged() to inform other components when the underlying data source, or its structure, has changed.

If we call resetInternalData everytime when the model gets reset, then the row move signals won't be there, thus no row move animation for QML, which (maybe) break the whole propose of using SortProxyModel. If it's intentional not to be used in such way, then indeed using modelReset is cleaner.

[BLumia]

Oh, just realized, did you mean treat modelReset as full range dataChanged? Will try to test it later and see if that work.

[dfaure-kdab]

I don't really know. I actually don't know anything about SortProxyModel myself, @AndreSomers will have final say in any case.
But as the maintainer of itemmodels in Qt, I know for sure that the job of a proxy is to react to modelReset (and layoutChanged), not to connect to the source's source directly.

[BLumia]

Will try to test it later and see if that work.

modelreset.zip

Above is a simple test program for the issue that I want to address. (the code is plainly for testing purpose so it's still sort of messy)

• If we compile and run it directly, the app window won't show any data, which is the bug.
• If we uncomment sortproxymodel.cpp line 143 to line 146, then recompile and run it, there will be data shown in the window, and when we drag the slider at the bottom side of the window, the items will moved with animation
• If we don't uncomment sortproxymodel.cpp line 143 to line 146, but uncomment line 136 to line 142 then recompile and run, the bug still exists
• If we keep sortproxymodel.cpp line 136 to 146 commented, and change main.qml line 38 to sourceModel: theModel, there will be no bug since theModel is not a proxy model.

[BLumia]

Patch updated, now we rely on modelReset instead and will only reset the internal data when necessary, I guess this approach will be fine in both cases.

[BLumia]

@dfaure-kdab sorry for the ping, could you check if the updated patch is fine to merge?

[dfaure-kdab]

As I said, @AndreSomers will be the one to approve this, as the maintainer of sortproxymodel.
He's on vacations though, so I'm afraid you'll have to wait a bit longer.

[AndreSomers]

Thank you for your contribution; sorry for not responding to your contribution sooner. Unfortunately, I think your approach is fundamentally flawed. I don't think it is reasonable to assume that after a model reset we conceptually have the same items in the model if we have the same number of rows.

Of course, the SortProxyModel should indeed listen to the modelReset signals, but it should do so with emiting it's own modelAboutToReset and then after the source model send modelReset, it should just completely re-build the internal model and not attempt to emit moves. I would welcome

If you are dealing with input data that doesn't fundamentally change but gets updated by just dumping a new batch (which is often handled by just doing a model reset), you may need to look at updateableModel. This model is designed to handle situations like that and will actually go over the data to figure out what changed instead of just resetting the model. Putting a SortProxyModel on top of that should then work.