From c2b6aa5e513a47939bad3703c6b0d984faff9d2a Mon Sep 17 00:00:00 2001 From: Alberto Mardegan Date: Thu, 24 Oct 2019 23:06:57 +0300 Subject: QQuickTreeModelAdaptor1: fix updating of ModelIndex role The decision to store the original QModelIndex as a role has the unfortunate consequence that we need to emit the dataChanged() signal whenever its value change. The previous implementation of the "move rows" operation failed in delivering this update in most cases. While addressing that, this commit also fixes some similar issues with the HasChildren role not being notified in certain scenarios. Also, when the last child of an expanded item gets moved away and later placed back in the original position, the item should not remember its expanded state, but rather accept the children while staying collapsed. Last but not least, it's important that the endMoveRows() protected method is called *after* the original tree model has completed its movement, because otherwise its elements might be accessed while in the wrong state. Similarly, we don't want the dataChanged() signals to be emitted while rows are moved or removed; we queue up these events and fire them only after the item indexes have settled. Fixes: QTBUG-59606 Change-Id: Ib981c912ea19908e1283cc463331c3053d4d6e7d Reviewed-by: Mitch Curtis --- src/controls/Private/qquicktreemodeladaptor.cpp | 175 +++++++++++++++++++----- src/controls/Private/qquicktreemodeladaptor_p.h | 27 ++++ 2 files changed, 168 insertions(+), 34 deletions(-) (limited to 'src') diff --git a/src/controls/Private/qquicktreemodeladaptor.cpp b/src/controls/Private/qquicktreemodeladaptor.cpp index c2c364945..f1c8e4130 100644 --- a/src/controls/Private/qquicktreemodeladaptor.cpp +++ b/src/controls/Private/qquicktreemodeladaptor.cpp @@ -52,7 +52,8 @@ QT_BEGIN_NAMESPACE #endif QQuickTreeModelAdaptor1::QQuickTreeModelAdaptor1(QObject *parent) : - QAbstractListModel(parent), m_model(0), m_lastItemIndex(0) + QAbstractListModel(parent), m_model(0), m_lastItemIndex(0), + m_visibleRowsMoved(false), m_signalAggregatorStack(0) { } @@ -490,11 +491,13 @@ void QQuickTreeModelAdaptor1::collapseRow(int n) if (!m_model || !isExpanded(n)) return; + SignalFreezer aggregator(this); + TreeItem &item = m_items[n]; item.expanded = false; m_expandedItems.remove(item.index); QVector changedRole(1, ExpandedRole); - emit dataChanged(index(n), index(n), changedRole); + queueDataChanged(index(n), index(n), changedRole); int childrenCount = m_model->rowCount(item.index); if ((item.index.flags() & Qt::ItemNeverHasChildren) || !m_model->hasChildren(item.index) || childrenCount == 0) return; @@ -530,8 +533,18 @@ void QQuickTreeModelAdaptor1::removeVisibleRows(int startIndex, int endIndex, bo if (doRemoveRows) beginRemoveRows(QModelIndex(), startIndex, endIndex); m_items.erase(m_items.begin() + startIndex, m_items.begin() + endIndex + 1); - if (doRemoveRows) + if (doRemoveRows) { endRemoveRows(); + + /* We need to update the model index for all the items below the removed ones */ + int lastIndex = m_items.count() - 1; + if (startIndex <= lastIndex) { + const QModelIndex &topLeft = index(startIndex, 0, QModelIndex()); + const QModelIndex &bottomRight = index(lastIndex, 0, QModelIndex()); + const QVector changedRole(1, ModelIndexRole); + queueDataChanged(topLeft, bottomRight, changedRole); + } + } } void QQuickTreeModelAdaptor1::modelHasBeenDestroyed() @@ -636,7 +649,7 @@ void QQuickTreeModelAdaptor1::modelRowsInserted(const QModelIndex & parent, int if (parentRow >= 0) { const QModelIndex& parentIndex = index(parentRow); QVector changedRole(1, HasChildrenRole); - emit dataChanged(parentIndex, parentIndex, changedRole); + queueDataChanged(parentIndex, parentIndex, changedRole); item = m_items.at(parentRow); if (!item.expanded) { ASSERT_CONSISTENCY(); @@ -655,6 +668,7 @@ void QQuickTreeModelAdaptor1::modelRowsInserted(const QModelIndex & parent, int void QQuickTreeModelAdaptor1::modelRowsAboutToBeRemoved(const QModelIndex & parent, int start, int end) { ASSERT_CONSISTENCY(); + enableSignalAggregation(); if (parent == m_rootIndex || childrenVisible(parent)) { const QModelIndex &smi = m_model->index(start, 0, parent); int startIndex = itemIndex(smi); @@ -687,19 +701,30 @@ void QQuickTreeModelAdaptor1::modelRowsRemoved(const QModelIndex & parent, int s if (parentRow >= 0) { const QModelIndex& parentIndex = index(parentRow); QVector changedRole(1, HasChildrenRole); - emit dataChanged(parentIndex, parentIndex, changedRole); + queueDataChanged(parentIndex, parentIndex, changedRole); } + disableSignalAggregation(); ASSERT_CONSISTENCY(); } void QQuickTreeModelAdaptor1::modelRowsAboutToBeMoved(const QModelIndex & sourceParent, int sourceStart, int sourceEnd, const QModelIndex & destinationParent, int destinationRow) { ASSERT_CONSISTENCY(); + enableSignalAggregation(); + m_visibleRowsMoved = false; if (!childrenVisible(sourceParent)) return; // Do nothing now. See modelRowsMoved() below. if (!childrenVisible(destinationParent)) { modelRowsAboutToBeRemoved(sourceParent, sourceStart, sourceEnd); + /* If the destination parent has no children, we'll need to + * report a change on the HasChildrenRole */ + if (isVisible(destinationParent) && m_model->rowCount(destinationParent) == 0) { + const QModelIndex &topLeft = index(itemIndex(destinationParent), 0, QModelIndex()); + const QModelIndex &bottomRight = topLeft; + const QVector changedRole(1, HasChildrenRole); + queueDataChanged(topLeft, bottomRight, changedRole); + } } else { int depthDifference = -1; if (destinationParent.isValid()) { @@ -734,7 +759,9 @@ void QQuickTreeModelAdaptor1::modelRowsAboutToBeMoved(const QModelIndex & source int totalMovedCount = endIndex - startIndex + 1; - const bool visibleRowsMoved = startIndex != destIndex && + /* This beginMoveRows() is matched by a endMoveRows() in the + * modelRowsMoved() method below. */ + m_visibleRowsMoved = startIndex != destIndex && beginMoveRows(QModelIndex(), startIndex, endIndex, QModelIndex(), destIndex); const QList &buffer = m_items.mid(startIndex, totalMovedCount); @@ -757,48 +784,62 @@ void QQuickTreeModelAdaptor1::modelRowsAboutToBeMoved(const QModelIndex & source m_items.replace(bufferCopyOffset + i, item); } - if (visibleRowsMoved) - endMoveRows(); + /* If both source and destination items are visible, the indexes of + * all the items in between will change. If they share the same + * parent, then this is all; however, if they belong to different + * parents, their bottom siblings will also get displaced, so their + * index also needs to be updated. + * Given that the bottom siblings of the top moved elements are + * already included in the update (since they lie between the + * source and the dest elements), we only need to worry about the + * siblings of the bottom moved element. + */ + const int top = qMin(startIndex, bufferCopyOffset); + int bottom = qMax(endIndex, bufferCopyOffset + totalMovedCount - 1); + if (sourceParent != destinationParent) { + const QModelIndex &bottomParent = + bottom == endIndex ? sourceParent : destinationParent; + + const int rowCount = m_model->rowCount(bottomParent); + if (rowCount > 0) + bottom = qMax(bottom, lastChildIndex(m_model->index(rowCount - 1, 0, bottomParent))); + } + const QModelIndex &topLeft = index(top, 0, QModelIndex()); + const QModelIndex &bottomRight = index(bottom, 0, QModelIndex()); + const QVector changedRole(1, ModelIndexRole); + queueDataChanged(topLeft, bottomRight, changedRole); if (depthDifference != 0) { const QModelIndex &topLeft = index(bufferCopyOffset, 0, QModelIndex()); const QModelIndex &bottomRight = index(bufferCopyOffset + totalMovedCount - 1, 0, QModelIndex()); const QVector changedRole(1, DepthRole); - emit dataChanged(topLeft, bottomRight, changedRole); + queueDataChanged(topLeft, bottomRight, changedRole); } } } void QQuickTreeModelAdaptor1::modelRowsMoved(const QModelIndex & sourceParent, int sourceStart, int sourceEnd, const QModelIndex & destinationParent, int destinationRow) { - if (childrenVisible(destinationParent)) { - if (!childrenVisible(sourceParent)) - modelRowsInserted(destinationParent, destinationRow, destinationRow + sourceEnd - sourceStart); - else { - int destIndex = -1; - if (destinationRow == m_model->rowCount(destinationParent)) { - const QModelIndex &emi = m_model->index(destinationRow - 1, 0, destinationParent); - destIndex = lastChildIndex(emi) + 1; - } else { - destIndex = itemIndex(m_model->index(destinationRow, 0, destinationParent)); - } + if (!childrenVisible(sourceParent)) { + modelRowsInserted(destinationParent, destinationRow, destinationRow + sourceEnd - sourceStart); + } else if (!childrenVisible(destinationParent)) { + modelRowsRemoved(sourceParent, sourceStart, sourceEnd); + } - const QModelIndex &emi = m_model->index(destinationRow + sourceEnd - sourceStart, 0, destinationParent); - int endIndex = -1; - if (isExpanded(emi)) { - int rowCount = m_model->rowCount(emi); - if (rowCount > 0) - endIndex = lastChildIndex(m_model->index(rowCount - 1, 0, emi)); - } - if (endIndex == -1) - endIndex = itemIndex(emi); + if (m_visibleRowsMoved) + endMoveRows(); - const QModelIndex &topLeft = index(destIndex, 0, QModelIndex()); - const QModelIndex &bottomRight = index(endIndex, 0, QModelIndex()); - const QVector changedRole(1, ModelIndexRole); - emit dataChanged(topLeft, bottomRight, changedRole); - } + if (isVisible(sourceParent) && m_model->rowCount(sourceParent) == 0) { + int parentRow = itemIndex(sourceParent); + collapseRow(parentRow); + const QModelIndex &topLeft = index(parentRow, 0, QModelIndex()); + const QModelIndex &bottomRight = topLeft; + const QVector changedRole { ExpandedRole, HasChildrenRole }; + queueDataChanged(topLeft, bottomRight, changedRole); } + + disableSignalAggregation(); + ASSERT_CONSISTENCY(); } @@ -887,4 +928,70 @@ bool QQuickTreeModelAdaptor1::testConsistency(bool dumpOnFail) const return true; } +void QQuickTreeModelAdaptor1::enableSignalAggregation() { + m_signalAggregatorStack++; +} + +void QQuickTreeModelAdaptor1::disableSignalAggregation() { + m_signalAggregatorStack--; + Q_ASSERT(m_signalAggregatorStack >= 0); + if (m_signalAggregatorStack == 0) { + emitQueuedSignals(); + } +} + +void QQuickTreeModelAdaptor1::queueDataChanged(const QModelIndex &topLeft, + const QModelIndex &bottomRight, + const QVector &roles) +{ + if (isAggregatingSignals()) { + m_queuedDataChanged.append(DataChangedParams { topLeft, bottomRight, roles }); + } else { + emit dataChanged(topLeft, bottomRight, roles); + } +} + +void QQuickTreeModelAdaptor1::emitQueuedSignals() +{ + QVector combinedUpdates; + /* First, iterate through the queued updates and merge the overlapping ones + * to reduce the number of updates. + * We don't merge adjacent updates, because they are typically filed with a + * different role (a parent row is next to its children). + */ + for (const DataChangedParams &dataChange : m_queuedDataChanged) { + int startRow = dataChange.topLeft.row(); + int endRow = dataChange.bottomRight.row(); + bool merged = false; + for (DataChangedParams &combined : combinedUpdates) { + int combinedStartRow = combined.topLeft.row(); + int combinedEndRow = combined.bottomRight.row(); + if ((startRow <= combinedStartRow && endRow >= combinedStartRow) || + (startRow <= combinedEndRow && endRow >= combinedEndRow)) { + if (startRow < combinedStartRow) { + combined.topLeft = dataChange.topLeft; + } + if (endRow > combinedEndRow) { + combined.bottomRight = dataChange.bottomRight; + } + for (int role : dataChange.roles) { + if (!combined.roles.contains(role)) + combined.roles.append(role); + } + merged = true; + break; + } + } + if (!merged) { + combinedUpdates.append(dataChange); + } + } + + /* Finally, emit the dataChanged signals */ + for (const DataChangedParams &dataChange : combinedUpdates) { + emit dataChanged(dataChange.topLeft, dataChange.bottomRight, dataChange.roles); + } + m_queuedDataChanged.clear(); +} + QT_END_NAMESPACE diff --git a/src/controls/Private/qquicktreemodeladaptor_p.h b/src/controls/Private/qquicktreemodeladaptor_p.h index e71923149..6e926db2e 100644 --- a/src/controls/Private/qquicktreemodeladaptor_p.h +++ b/src/controls/Private/qquicktreemodeladaptor_p.h @@ -157,12 +157,39 @@ private: } }; + struct DataChangedParams { + QModelIndex topLeft; + QModelIndex bottomRight; + QVector roles; + }; + + struct SignalFreezer { + SignalFreezer(QQuickTreeModelAdaptor1 *parent) : m_parent(parent) { + m_parent->enableSignalAggregation(); + } + ~SignalFreezer() { m_parent->disableSignalAggregation(); } + + private: + QQuickTreeModelAdaptor1 *m_parent; + }; + + void enableSignalAggregation(); + void disableSignalAggregation(); + bool isAggregatingSignals() const { return m_signalAggregatorStack > 0; } + void queueDataChanged(const QModelIndex &topLeft, + const QModelIndex &bottomRight, + const QVector &roles); + void emitQueuedSignals(); + QPointer m_model; QPersistentModelIndex m_rootIndex; QList m_items; QSet m_expandedItems; QList m_itemsToExpand; mutable int m_lastItemIndex; + bool m_visibleRowsMoved; + int m_signalAggregatorStack; + QVector m_queuedDataChanged; }; QT_END_NAMESPACE -- cgit v1.2.3