Details
-
Bug
-
Resolution: Unresolved
-
P3: Somewhat important
-
None
-
5.5.1, 5.6.1, 5.7.0, 5.8.0 Alpha
-
None
-
Windows 10 64bit with Qt 5.7 32bit
Description
Using QModelIndexes and QPersistentIndexes is unsafe. This is mostly visible with TreeView because it's one of the first controls who uses QModelIndexes.
I've attached a simple test application for showing the error but i've decided to fill this issue also for descussing how to solve this and proposing some ideas.
For seeing the bug:
1) Compile and execute the application
2) Remove the the first item with index(0,0,..)
Expected result
the second first item should disappear and the second item should update its index to index(0,0..)
Real result
The second item index is not updated. Clicking the info button shows an invalid modelindex in the program output.
The info button simply do
treeModel.index(styleData.index.row, styleData.index.column, styleData.index.parent)
In other words the TreeModel correctly say that there's no index at that position but the view hasn't updated
Explanation
This is caused by how the QQuickTreeModelAdaptor works but in the long run this could be also a design problem with QModelIndexes inside QML.
Before moving the discussion in how to safely exchange QModelndexes in QML i will explain the bug.
1) The TreeModel begin removing and item and emits rowsAboutToBeRemoved
2) The QQuickTreeModelAdaptor execute onModelRowsAboutToBeRemoved
3) This in turn calls the removeVisibleRows with doRemoveRows true (in order to execute beginRemoveRows and endRemoveRows)
4) Point #3 imply that the Adaptor emit endRemoveRows before its sourceModel. In fact the chain is sourceModel->begin, adaptor->begin, adaptor->end, sourceModel->end.
5) When the adaptor emit the rowsRemoved signal (inside the endRemoveRows) the ListView in qml updated its items and obviously remove the row (or recycle it) but for sure update the styleData.row field
6) This in turn causes the styleData.index property to be updated (since it depends on the styleData.row)
7) The TreeViewItemDelegateLoader ask to the adaptor to map the (row,column) to and index calling the mapRowToModelIndex
8) In a safe manner the Adaptor uses QPersistentModelIndexes in its internal datastructure. However the QPersistentModelIndexes are update AFTER beginRemoveRows. So here's the bug. The adaptor obtain an OLD index. This is somewhat correct from the source model indexe point of view. In fact it's notifying its observers that those indexes are "ABOUT" to be remove but still valid.
9) The qml obtain the old index
10) The Adaptor complete its removeVisibleRows
11) The Source model finishes to notify the observers of the rowsAboutToBeRemoved and effectly remove the rows
12) The QPersistentModelIndexes gets updated
13) The source model emit the rowsRemoved signal (inside the endRemoveRows)
Solutions:
Currently in my own clone i patched the TreeModelAdaptor in order to execute the endRemoveRows inside its onModelRowsRemoved handler. This causes the call chain to be
sourceModel->begin, adaptor->begin, sourceModel->end, adaptor->end
As this problem shows using QPersistentModelIndexes is still unsafe.
Storing a QPersistentModelIndex in qml could help but it's not enough.
In fact suppose someone writes this property in qml
property var persistentIndex: model.index(row, column)
This could be somewhat safe but has the drawback it gets not updated automatically when the QPersistenModelIndex change.
Thus a property dependeing on that it's not reevaluated i.e.:
property var persistentIndex: model.index(row, column) property var data: model.data(persistentIndex)
A solution is banning the of QModelIndexes from QML. Another could be the use (for the implementation of complex objects) of a QObject wrapper
that gets notified (but right know i don't know how) from the QPersistentModelIndex when it gets update. Something like
IndexWrapper { id: wrapper // property var index viewProxyModel: view.adaptor row: styleData.row column: styleData.column } property var data: wrapper.index
This IndexWrapper update its index when:
1) The row or column change
2) The PersistenModelIndex mapped by the adaptor change
Conclusion
I can quickly contribute a patch to the Adaptor for solving the problem explained in this bug report
but i would like to get some feedback before pushing it.
I can work also on a long term solution