Uploaded image for project: 'Qt'
  1. Qt
  2. QTBUG-55864

QModelIndexes and QPersistentModelIndexes are unsafe in Qml

    XMLWordPrintable

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

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            qt.team.quick.subscriptions Qt Quick and Widgets Team
            cuke Filippo Cucchetto
            Votes:
            3 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

              Created:
              Updated:

              Gerrit Reviews

                There are no open Gerrit changes