Improve handling of QTBUG-57971 (ListModel initializing roles on first insertion) #5
Conversation
| &QQmlSortFilterProxyModel::initRoles, Qt::UniqueConnection); | ||
|
|
||
| if (sourceModel) { | ||
| connect(sourceModel, &QAbstractItemModel::modelReset, this, [sourceModel, this]() { |
There was a problem hiding this comment.
We should also disconnect this after the first insertion, right?
There was a problem hiding this comment.
No, this connection must stay here forever, otherwise cases like status-im/status-desktop#16310 (comment) won't work. The reset may be emitted because the source model has own source changed. And source of source may be e.g. ListModel with not initialized roles. So we must assume that after every source reset the workaround procedure must be connected again if source is empty.
There was a problem hiding this comment.
Got it, thanks! In this case I think we should disconnect on sourceModel change
There was a problem hiding this comment.
Ahh, you are right! Your statement is true even for the original implementation. The connection to rowsInserted should be also disconnected on source change.
There was a problem hiding this comment.
Thank you @alexjba for your discovery, there is excerpt exploiting that problem:
import QtQuick 2.15
import QtQuick.Controls 2.15
import StatusQ.Core.Utils 0.1
import SortFilterProxyModel 0.2
Item {
id: root
ListModel {
id: emptyModel
}
ListModel {
id: emptyModel2
}
SortFilterProxyModel {
id: sfpm
sourceModel: emptyModel
proxyRoles: ExpressionRole {
name: "customRole"
expression: "X" + model.index
}
}
ListView {
id: lv
anchors.fill: parent
model: sfpm
delegate: Text {
text: model.text + " | " + model.customRole
}
}
Button {
anchors.centerIn: parent
text: "ADD"
onClicked: {
sfpm.sourceModel = emptyModel2
emptyModel.append({ text: "d" })
emptyModel2.append({ text: "d" })
}
}
}Inserting into model that was a source (but is not currently) triggers the initRoles and disconnects the slot from currently used model 😱. So when inserting into current source, roles are not initialized properly... 🤦♂️
There was a problem hiding this comment.
Another example, this one exploiting lack of disconnection from modelReset:
import QtQuick 2.15
import QtQuick.Controls 2.15
import StatusQ.Core.Utils 0.1
import SortFilterProxyModel 0.2
Item {
id: root
ListModel {
id: emptyModel
}
ListModel {
id: emptyModel2
}
ListModel {
id: emptyModel3
}
SortFilterProxyModel {
id: sfpm1
sourceModel: emptyModel
proxyRoles: ExpressionRole {
name: "customRole"
expression: "X" + model.index
}
}
SortFilterProxyModel {
id: sfpm3
sourceModel: emptyModel3
proxyRoles: ExpressionRole {
name: "customRole"
expression: "X-" + model.index
}
}
SortFilterProxyModel {
id: sfpm2
sourceModel: sfpm1
}
ListView {
id: lv
anchors.fill: parent
model: sfpm2
delegate: Text {
text: model.text + " | " + model.customRole
}
}
Button {
anchors.centerIn: parent
text: "ADD"
onClicked: {
sfpm2.sourceModel = sfpm3
sfpm1.sourceModel = emptyModel2
emptyModel2.append({ text: "d" })
emptyModel3.append({ text: "d" })
}
}
}
Once it's merged we can test the app in a status-im/status-desktop#16321 where the |
Yeah sure, realized too late this is a different repo 😊 |
…t insertion) So far the provided workaround was missing some corner cases when there is more than one SFPM (or other proxies) in a chain, providing some custom roles like proxy roles in SFPM. Detailed explanation provided as a comment directly in the source.
micieslak
force-pushed
the
fix/improved-QTBUG-57971-handling
branch
from
d6df62f
to
40daec0
Compare
|
Ideally we should add proper unit tests for those corner cases. However now it's not easy as all helpers we are using for proxy models testing are located in status-im/status-desktop#16310 (comment) |
There already some tests under |
Good point, I created a ticket: status-im/status-desktop#16343 |
Description
So far the provided workaround was missing some corner cases when there is more than one SFPM (or other proxies) in a chain, providing some custom roles like proxy roles in SFPM. Detailed explanation provided as a comment directly in the source.
Additional information about failing corner cases can be found here: status-im/status-desktop#16310