From 0d4e7bd458b12d917623d5c87e8b888965ccfd13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Cie=C5=9Blak?= Date: Wed, 21 Feb 2024 14:45:09 +0100 Subject: [PATCH] StatusQ(MovableModel): layoutChanged handling fixed Closes: #13602 --- ui/StatusQ/include/StatusQ/movablemodel.h | 34 +++- ui/StatusQ/src/movablemodel.cpp | 218 ++++++++++++++-------- ui/StatusQ/tests/tst_MovableModel.cpp | 95 +++++++++- 3 files changed, 264 insertions(+), 83 deletions(-) diff --git a/ui/StatusQ/include/StatusQ/movablemodel.h b/ui/StatusQ/include/StatusQ/movablemodel.h index ccec69cbc6..5ff3b71bcd 100644 --- a/ui/StatusQ/include/StatusQ/movablemodel.h +++ b/ui/StatusQ/include/StatusQ/movablemodel.h @@ -13,13 +13,13 @@ class MovableModel : public QAbstractListModel Q_PROPERTY(bool synced READ synced NOTIFY syncedChanged) public: - explicit MovableModel(QObject *parent = nullptr); + explicit MovableModel(QObject* parent = nullptr); void setSourceModel(QAbstractItemModel *sourceModel); - QAbstractItemModel *sourceModel() const; + QAbstractItemModel* sourceModel() const; - int rowCount(const QModelIndex &parent = QModelIndex()) const override; - QVariant data(const QModelIndex &index, int role) const override; + int rowCount(const QModelIndex& parent = QModelIndex()) const override; + QVariant data(const QModelIndex& index, int role) const override; QHash roleNames() const override; Q_INVOKABLE void desyncOrder(); @@ -37,10 +37,30 @@ protected slots: void resetInternalData(); private: + // source signals handling for synced state + void syncedSourceDataChanged(const QModelIndex& topLeft, + const QModelIndex& bottomRight, + const QVector& roles); + void sourceLayoutAboutToBeChanged(const QList& parents, + QAbstractItemModel::LayoutChangeHint hint); + void sourceLayoutChanged(const QList& parents, + QAbstractItemModel::LayoutChangeHint hint); + + // source signals handling for desynced state + void desyncedSourceDataChanged(const QModelIndex& topLeft, + const QModelIndex& bottomRight, + const QVector& roles); + void sourceRowsInserted(const QModelIndex &parent, int first, int last); + void sourceRowsAboutToBeRemoved(const QModelIndex &parent, int first, int last); + + // other + void connectSignalsForSyncedState(); + QPointer m_sourceModel; - - void connectSignalsForAttachedState(); - bool m_synced = true; std::vector m_indexes; + + // helpers for handling layoutChanged from source when synced + QList m_layoutChangePersistentIndexes; + QModelIndexList m_proxyIndexes; }; diff --git a/ui/StatusQ/src/movablemodel.cpp b/ui/StatusQ/src/movablemodel.cpp index cd2f9fe8f0..0ae1af1bc4 100644 --- a/ui/StatusQ/src/movablemodel.cpp +++ b/ui/StatusQ/src/movablemodel.cpp @@ -20,7 +20,7 @@ void MovableModel::setSourceModel(QAbstractItemModel* sourceModel) disconnect(m_sourceModel, nullptr, this, nullptr); m_sourceModel = sourceModel; - connectSignalsForAttachedState(); + connectSignalsForSyncedState(); emit sourceModelChanged(); @@ -89,83 +89,22 @@ void MovableModel::desyncOrder() &MovableModel::endMoveRows); disconnect(m_sourceModel, &QAbstractItemModel::dataChanged, this, - &MovableModel::dataChanged); + &MovableModel::syncedSourceDataChanged); disconnect(m_sourceModel, &QAbstractItemModel::layoutAboutToBeChanged, this, - &MovableModel::layoutAboutToBeChanged); + &MovableModel::sourceLayoutAboutToBeChanged); disconnect(m_sourceModel, &QAbstractItemModel::layoutChanged, this, - &MovableModel::layoutChanged); + &MovableModel::sourceLayoutChanged); connect(m_sourceModel, &QAbstractItemModel::dataChanged, this, - [this](const QModelIndex& topLeft, const QModelIndex& bottomRight, - const QVector& roles) { - emit dataChanged(index(0), index(rowCount() - 1), roles); - }); + &MovableModel::desyncedSourceDataChanged); connect(m_sourceModel, &QAbstractItemModel::rowsInserted, this, - [this](const QModelIndex &parent, int first, int last) { - - beginInsertRows({}, first, last); - - int oldCount = m_indexes.size(); - int insertCount = last - first + 1; - - m_indexes.reserve(m_indexes.size() + insertCount); - - for (auto i = first; i <= last; i++) - m_indexes.emplace_back(m_sourceModel->index(i, 0)); - - std::rotate(m_indexes.begin() + first, m_indexes.begin() + oldCount, - m_indexes.end()); - - endInsertRows(); - }); + &MovableModel::sourceRowsInserted); connect(m_sourceModel, &QAbstractItemModel::rowsAboutToBeRemoved, this, - [this] (const QModelIndex &parent, int first, int last) { - std::vector indicesToRemove; - indicesToRemove.reserve(last - first + 1); - - for (auto i = 0; i < m_indexes.size(); i++) { - const QPersistentModelIndex& idx = m_indexes.at(i); - - if (idx.row() >= first && idx.row() <= last) - indicesToRemove.push_back(i); - } - - std::vector> sequences; - auto sequenceBegin = indicesToRemove.front(); - auto sequenceEnd = sequenceBegin; - - for (auto i = 1; i < indicesToRemove.size(); i++) { - auto idxToRemove = indicesToRemove[i]; - - auto idxDiff = idxToRemove - sequenceEnd; - if (idxDiff == 1) - sequenceEnd = idxToRemove; - - if (idxDiff != 1 || i == indicesToRemove.size() - 1) { - sequences.emplace_back(sequenceBegin, sequenceEnd); - sequenceBegin = idxToRemove; - sequenceEnd = idxToRemove; - } - } - - if (sequences.empty()) - sequences.emplace_back(sequenceBegin, sequenceEnd); - - auto end = sequences.crend(); - - for (auto it = sequences.crbegin(); it != end; it++) { - beginRemoveRows({}, it->first, it->second); - - m_indexes.erase(m_indexes.begin() + it->first, - m_indexes.begin() + it->second + 1); - - endRemoveRows(); - } - }); + &MovableModel::sourceRowsAboutToBeRemoved); auto count = m_sourceModel->rowCount(); @@ -189,10 +128,18 @@ void MovableModel::syncOrder() auto sourceModel = m_sourceModel; disconnect(m_sourceModel, nullptr, this, nullptr); - connectSignalsForAttachedState(); + connectSignalsForSyncedState(); + + for (int i = 0; i < m_indexes.size(); ++i) { + const QModelIndex idx = m_indexes[i]; + + if (i == idx.row()) + continue; + + changePersistentIndex(index(i, 0), index(idx.row(), 0)); + } resetInternalData(); - emit layoutChanged(); } @@ -262,7 +209,130 @@ void MovableModel::resetInternalData() } } -void MovableModel::connectSignalsForAttachedState() +void MovableModel::syncedSourceDataChanged(const QModelIndex& topLeft, + const QModelIndex& bottomRight, + const QVector& roles) +{ + emit dataChanged(index(topLeft.row(), topLeft.column()), + index(bottomRight.row(), bottomRight.column()), roles); +} + +void MovableModel::sourceLayoutAboutToBeChanged( + const QList& parents, + QAbstractItemModel::LayoutChangeHint hint) +{ + emit layoutAboutToBeChanged(); + + const auto persistentIndexes = persistentIndexList(); + + for (const QModelIndex& persistentIndex: persistentIndexes) { + m_proxyIndexes << persistentIndex; + Q_ASSERT(persistentIndex.isValid()); + const auto srcIndex = m_sourceModel->index( + persistentIndex.row(), + persistentIndex.column()); + + Q_ASSERT(srcIndex.isValid()); + m_layoutChangePersistentIndexes << srcIndex; + } +} + +void MovableModel::sourceLayoutChanged( + const QList& parents, + QAbstractItemModel::LayoutChangeHint hint) +{ + for (int i = 0; i < m_proxyIndexes.size(); ++i) { + auto p = m_layoutChangePersistentIndexes.at(i); + changePersistentIndex(m_proxyIndexes.at(i), index( + p.row(), p.column(), p.parent())); + } + + m_layoutChangePersistentIndexes.clear(); + m_proxyIndexes.clear(); + + emit layoutChanged(); +} + +void MovableModel::desyncedSourceDataChanged(const QModelIndex& topLeft, + const QModelIndex& bottomRight, + const QVector& roles) +{ + Q_UNUSED(topLeft) + Q_UNUSED(bottomRight) + + emit dataChanged(index(0), index(rowCount() - 1), roles); +} + +void MovableModel::sourceRowsInserted(const QModelIndex& parent, int first, + int last) +{ + Q_ASSERT(!parent.isValid()); + + beginInsertRows({}, first, last); + + int oldCount = m_indexes.size(); + int insertCount = last - first + 1; + + m_indexes.reserve(m_indexes.size() + insertCount); + + for (auto i = first; i <= last; i++) + m_indexes.emplace_back(m_sourceModel->index(i, 0)); + + std::rotate(m_indexes.begin() + first, m_indexes.begin() + oldCount, + m_indexes.end()); + + endInsertRows(); +} + +void MovableModel::sourceRowsAboutToBeRemoved(const QModelIndex& parent, + int first, int last) +{ + Q_ASSERT(!parent.isValid()); + + std::vector indicesToRemove; + indicesToRemove.reserve(last - first + 1); + + for (auto i = 0; i < m_indexes.size(); i++) { + const QPersistentModelIndex& idx = m_indexes.at(i); + + if (idx.row() >= first && idx.row() <= last) + indicesToRemove.push_back(i); + } + + std::vector> sequences; + auto sequenceBegin = indicesToRemove.front(); + auto sequenceEnd = sequenceBegin; + + for (auto i = 1; i < indicesToRemove.size(); i++) { + auto idxToRemove = indicesToRemove[i]; + + auto idxDiff = idxToRemove - sequenceEnd; + if (idxDiff == 1) + sequenceEnd = idxToRemove; + + if (idxDiff != 1 || i == indicesToRemove.size() - 1) { + sequences.emplace_back(sequenceBegin, sequenceEnd); + sequenceBegin = idxToRemove; + sequenceEnd = idxToRemove; + } + } + + if (sequences.empty()) + sequences.emplace_back(sequenceBegin, sequenceEnd); + + auto end = sequences.crend(); + + for (auto it = sequences.crbegin(); it != end; it++) { + beginRemoveRows({}, it->first, it->second); + + m_indexes.erase(m_indexes.begin() + it->first, + m_indexes.begin() + it->second + 1); + + endRemoveRows(); + } +} + +void MovableModel::connectSignalsForSyncedState() { if (m_sourceModel == nullptr) return; @@ -286,13 +356,13 @@ void MovableModel::connectSignalsForAttachedState() &MovableModel::endMoveRows); connect(m_sourceModel, &QAbstractItemModel::dataChanged, this, - &MovableModel::dataChanged); + &MovableModel::syncedSourceDataChanged); connect(m_sourceModel, &QAbstractItemModel::layoutAboutToBeChanged, this, - &MovableModel::layoutAboutToBeChanged); + &MovableModel::sourceLayoutAboutToBeChanged); connect(m_sourceModel, &QAbstractItemModel::layoutChanged, this, - &MovableModel::layoutChanged); + &MovableModel::sourceLayoutChanged); connect(m_sourceModel, &QAbstractItemModel::modelAboutToBeReset, this, &MovableModel::beginResetModel); diff --git a/ui/StatusQ/tests/tst_MovableModel.cpp b/ui/StatusQ/tests/tst_MovableModel.cpp index f049e686c6..1a25d2f7c7 100644 --- a/ui/StatusQ/tests/tst_MovableModel.cpp +++ b/ui/StatusQ/tests/tst_MovableModel.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include class TestMovableModel : public QObject @@ -305,6 +306,7 @@ private slots: sourceModelCopy.move(2, 1); ModelSignalsSpy signalsSpy(&model); + PersistentIndexesTester indexesTester(&model); sfpm.setSortRole(1); sfpm.sort(0, Qt::DescendingOrder); @@ -323,6 +325,7 @@ private slots: QCOMPARE(sfpm.roleNames().value(1), "subname"); QVERIFY(isSame(&sfpm, expectedSorted)); QVERIFY(isSame(&model, sourceModelCopy)); + QVERIFY(indexesTester.compare()); } void insertionTest() @@ -444,7 +447,48 @@ private slots: QCOMPARE(signalsSpy.rowsRemovedSpy.at(1).at(2), 1); } - void dataChangeTest() + void dataChangeWhenSyncedTest() + { + QQmlEngine engine; + + ListModelWrapper sourceModel(engine, R"([ + { "name": "A", "subname": "a1" }, + { "name": "A", "subname": "a2" }, + { "name": "B", "subname": "b1" }, + { "name": "C", "subname": "c1" }, + { "name": "C", "subname": "c2" }, + { "name": "C", "subname": "c3" } + ])"); + + MovableModel model; + model.setSourceModel(sourceModel); + + ModelSignalsSpy signalsSpy(&model); + + sourceModel.setProperty(1, "subname", "a2_"); + + ListModelWrapper expected(engine, R"([ + { "name": "A", "subname": "a1" }, + { "name": "A", "subname": "a2_" }, + { "name": "B", "subname": "b1" }, + { "name": "C", "subname": "c1" }, + { "name": "C", "subname": "c2" }, + { "name": "C", "subname": "c3" } + ])"); + + auto subnameRole = roleForName(model.roleNames(), "subname"); + + QCOMPARE(signalsSpy.count(), 1); + QCOMPARE(signalsSpy.dataChangedSpy.count(), 1); + QCOMPARE(signalsSpy.dataChangedSpy.first().at(0).toModelIndex(), model.index(1)); + QCOMPARE(signalsSpy.dataChangedSpy.first().at(1), model.index(1)); + QCOMPARE(signalsSpy.dataChangedSpy.first().at(2).value>(), + {subnameRole}); + + QVERIFY(isSame(&model, expected)); + } + + void dataChangeWhenDesyncedTest() { QQmlEngine engine; @@ -644,10 +688,12 @@ private slots: QCOMPARE(signalsSpy.count(), 0); } + PersistentIndexesTester indexesTester(&model); + model.desyncOrder(); sourceModel.move(0, 2, 2); - QVERIFY(!isSame(&model, sourceModel)); + QVERIFY(isNotSame(&model, sourceModel)); ModelSignalsSpy signalsSpy(&model); QSignalSpy syncedChangedSpy(&model, &MovableModel::syncedChanged); @@ -662,6 +708,51 @@ private slots: QCOMPARE(model.synced(), true); QVERIFY(isSame(&model, sourceModel)); + QVERIFY(indexesTester.compare()); + } + + void sortingSyncedTest() + { + QQmlEngine engine; + + auto source = R"([ + { "name": "A", "subname": "a1" }, + { "name": "A", "subname": "a2" }, + { "name": "B", "subname": "b1" }, + { "name": "C", "subname": "c1" }, + { "name": "C", "subname": "c2" }, + { "name": "C", "subname": "c3" } + ])"; + + ListModelWrapper sourceModel(engine, source); + ListModelWrapper sourceModelCopy(engine, source); + + QSortFilterProxyModel sfpm; + sfpm.setSourceModel(sourceModel); + + MovableModel model; + model.setSourceModel(&sfpm); + + ModelSignalsSpy signalsSpy(&model); + ModelSignalsSpy signalsSpySfpm(&sfpm); + + PersistentIndexesTester indexesTester(&model); + + sfpm.setSortRole(1); + sfpm.sort(0, Qt::DescendingOrder); + + ListModelWrapper expectedSorted(engine, R"([ + { "name": "C", "subname": "c3" }, + { "name": "C", "subname": "c2" }, + { "name": "C", "subname": "c1" }, + { "name": "B", "subname": "b1" }, + { "name": "A", "subname": "a2" }, + { "name": "A", "subname": "a1" } + ])"); + + QCOMPARE(model.synced(), true); + QCOMPARE(signalsSpy.count(), signalsSpySfpm.count()); + QVERIFY(indexesTester.compare()); } };