StatusQ(WritableProxyModel): Handling of layoutChanged/rowsMoved from source models fixed

Closes: #13601

ui/StatusQ/include/StatusQ/writableproxymodel.h

@@ -88,8 +88,9 @@ private:
     void onModelReset();
     void onLayoutAboutToBeChanged(const QList<QPersistentModelIndex>& sourceParents, QAbstractItemModel::LayoutChangeHint hint);
     void onLayoutChanged(const QList<QPersistentModelIndex>& sourceParents, QAbstractItemModel::LayoutChangeHint hint);
+    void onRowsAboutToBeMoved(const QModelIndex& sourceParent, int sourceStart, int sourceEnd, const QModelIndex& destinationParent, int destinationRow);
     void onRowsMoved(const QModelIndex& sourceParent, int sourceStart, int sourceEnd, const QModelIndex& destinationParent, int destinationRow);
-    
+
     QScopedPointer<WritableProxyModelPrivate> d;
     friend class WritableProxyModelPrivate;
 };

ui/StatusQ/src/writableproxymodel.cpp

@@ -5,6 +5,7 @@
 #endif
 #include <memory>
 
+
 template <typename T>
 using IndexedValues = QHash<T /*key*/, QMap<int/*role*/, QVariant/*value*/>>;
 
@@ -35,6 +36,13 @@ public:
     int sourceToProxyRow(int row) const;
     QVector<QPair<int, int>> sourceRowRangesBetween(int start, int end) const;
 
+    // helpers for handling layoutChanged from source
+    QList<QPersistentModelIndex> layoutChangePersistentIndexes;
+    QModelIndexList proxyIndexes;
+
+    void storePersitentIndexes();
+    void updatePersistentIndexes();
+
     //Simple mapping. No sorting, no moving
     //TODO: add mapping for temporarily moved rows
     void createProxyToSourceRowMap();
@@ -126,6 +134,33 @@ int WritableProxyModelPrivate::sourceToProxyRow(int row) const
     return -1;
 }
 
+void WritableProxyModelPrivate::storePersitentIndexes()
+{
+    const auto persistentIndexes = q.persistentIndexList();
+
+    for (const QModelIndex& persistentIndex: persistentIndexes) {
+
+        Q_ASSERT(persistentIndex.isValid());
+        const auto srcIndex = q.mapToSource(persistentIndex);
+
+        if (srcIndex.isValid()) {
+            proxyIndexes << persistentIndex;
+            layoutChangePersistentIndexes << srcIndex;
+        }
+    }
+}
+
+void WritableProxyModelPrivate::updatePersistentIndexes()
+{
+    for (int i = 0; i < proxyIndexes.size(); ++i) {
+        q.changePersistentIndex(proxyIndexes.at(i),
+                                q.mapFromSource(layoutChangePersistentIndexes.at(i)));
+    }
+
+    layoutChangePersistentIndexes.clear();
+    proxyIndexes.clear();
+}
+
 void WritableProxyModelPrivate::createProxyToSourceRowMap()
 {
     if (!q.sourceModel())
@@ -142,7 +177,8 @@ void WritableProxyModelPrivate::createProxyToSourceRowMap()
             continue;
         }
 
-        while(removedRows.contains(sourceModel->index(sourceIter, 0)) && sourceIter < sourceModel->rowCount())
+        while(removedRows.contains(sourceModel->index(sourceIter, 0))
+              && sourceIter < sourceModel->rowCount())
             ++sourceIter;
 
         proxyToSourceRowMapping.append(sourceIter);
@@ -620,6 +656,7 @@ void WritableProxyModel::setSourceModel(QAbstractItemModel* sourceModel)
     connect(sourceModel, &QAbstractItemModel::rowsRemoved, this, &WritableProxyModel::onRowsRemoved);
     connect(sourceModel, &QAbstractItemModel::modelAboutToBeReset, this, &WritableProxyModel::onModelAboutToBeReset);
     connect(sourceModel, &QAbstractItemModel::modelReset, this, &WritableProxyModel::onModelReset);
+    connect(sourceModel, &QAbstractItemModel::rowsAboutToBeMoved, this, &WritableProxyModel::onRowsAboutToBeMoved);
     connect(sourceModel, &QAbstractItemModel::rowsMoved, this, &WritableProxyModel::onRowsMoved);
     connect(sourceModel, &QAbstractItemModel::layoutAboutToBeChanged, this, &WritableProxyModel::onLayoutAboutToBeChanged);
     connect(sourceModel, &QAbstractItemModel::layoutChanged, this, &WritableProxyModel::onModelReset);
@@ -927,10 +964,7 @@ void WritableProxyModel::onRowsAboutToBeRemoved(const QModelIndex& parent, int s
         auto sourceIndex = sourceModel()->index(row, 0);
 
         if (d->cache.contains(sourceIndex))
-        {
             d->moveFromCacheToInserted(sourceIndex);
-            continue;
-        }
     }
 
     auto sourceRemoveRanges = d->sourceRowRangesBetween(start, end);
@@ -975,16 +1009,25 @@ void WritableProxyModel::onModelReset()
     endResetModel();
 }
 
+
+void WritableProxyModel::onRowsAboutToBeMoved(const QModelIndex &sourceParent, int sourceStart, int sourceEnd, const QModelIndex &destinationParent, int destinationRow)
+{
+    if(sourceParent.isValid() || destinationParent.isValid())
+        return;
+
+    emit layoutAboutToBeChanged();
+
+    d->storePersitentIndexes();
+}
+
 void WritableProxyModel::onRowsMoved(const QModelIndex& sourceParent, int sourceStart, int sourceEnd, const QModelIndex& destinationParent, int destinationRow)
 {
     if(sourceParent.isValid() || destinationParent.isValid())
-    {
         return;
-    }
 
-    emit layoutAboutToBeChanged();
-    d->clearInvalidatedCache();
     d->createProxyToSourceRowMap();
+    d->updatePersistentIndexes();
+
     emit layoutChanged();
 }
 
@@ -994,11 +1037,14 @@ void WritableProxyModel::onLayoutAboutToBeChanged(const QList<QPersistentModelIn
         return;
 
     emit layoutAboutToBeChanged();
+
+    d->storePersitentIndexes();
 }
 
 void WritableProxyModel::onLayoutChanged(const QList<QPersistentModelIndex>& sourceParents, QAbstractItemModel::LayoutChangeHint hint)
 {
-    d->clearInvalidatedCache();
     d->createProxyToSourceRowMap();
+    d->updatePersistentIndexes();
+
     emit layoutChanged();
 }

ui/StatusQ/tests/tst_WritableProxyModel.cpp

@@ -5,6 +5,10 @@
 
 #include "StatusQ/writableproxymodel.h"
 
+#include <TestHelpers/persistentindexestester.h>
+#include <TestHelpers/snapshotmodel.h>
+#include <TestHelpers/modeltestutils.h>
+
 namespace {
 
 class TestSourceModel : public QAbstractListModel {
@@ -73,6 +77,10 @@ public:
         if(!beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1, destinationParent, destinationChild))
             return false;
 
+        if (destinationChild > sourceRow) {
+            destinationChild -= 1;
+        }
+
         for (int i = 0; i < count; i++) {
             for (int j = 0; j < m_data.size(); j++) {
                 auto& roleVariantList = m_data[j].second;
@@ -765,7 +773,21 @@ private slots:
         QCOMPARE(rowsInsertedSpy.count(), 0);
         QCOMPARE(layoutChangedSpy.count(), 0);
 
-        sourceModel.moveRows({}, 1, 1, {}, 0);
+        PersistentIndexesTester indexesTester(&model);
+
+        {
+            SnapshotModel snapshot(model);
+
+            QObject context;
+            connect(&model, &WritableProxyModel::layoutAboutToBeChanged, &context,
+                    [&snapshot, &model] {
+                QVERIFY(isSame(snapshot, model));
+            });
+
+            sourceModel.moveRows({}, 1, 1, {}, 0);
+        }
+
+        QVERIFY(indexesTester.compare());
 
         QCOMPARE(model.dirty(), true);
         QCOMPARE(model.rowCount(), 2);
@@ -801,9 +823,11 @@ private slots:
         model.setData(model.index(2, 0), "Token 5.1", 0);
         model.setData(model.index(2, 0), "community_5.1", 1);
 
+        PersistentIndexesTester indexesTester(&model);
         bool success = sourceModel.moveRows({}, 1, 2, {}, 0);
 
         QVERIFY(success);
+        QVERIFY(indexesTester.compare());
 
         QCOMPARE(sourceModel.data(sourceModel.index(0, 0), 0), "Token 2");
         QCOMPARE(sourceModel.data(sourceModel.index(1, 0), 0), "Token 3");
@@ -835,11 +859,18 @@ private slots:
         model.removeRows(2, 1);
 
         QCOMPARE(model.dirty(), true);
+        QCOMPARE(model.rowCount(), 2);
         QCOMPARE(model.data(model.index(2, 0), 0), {});
         QCOMPARE(model.data(model.index(1, 0), 0), "Token 2");
         QCOMPARE(model.data(model.index(0, 0), 0), "Token 1");
 
-        sourceModel.moveRows({}, 2, 1, {}, 0);
+        PersistentIndexesTester indexesTester(&model);
+        PersistentIndexesTester sourceIndexesTester(&sourceModel);
+
+        QVERIFY(sourceModel.moveRows({}, 2, 1, {}, 0));
+        QVERIFY(sourceIndexesTester.compare());
+        QVERIFY(indexesTester.compare());
+        QCOMPARE(model.rowCount(), 2);
 
         QCOMPARE(sourceModel.data(sourceModel.index(0, 0), 0), "Token 3");
         QCOMPARE(sourceModel.data(sourceModel.index(1, 0), 0), "Token 1");
@@ -849,7 +880,10 @@ private slots:
         QCOMPARE(model.data(model.index(1, 0), 0), "Token 2");
         QCOMPARE(model.data(model.index(0, 0), 0), "Token 1");
 
-        sourceModel.moveRows({}, 1, 1, {}, 0);
+        QVERIFY(sourceModel.moveRows({}, 1, 1, {}, 0));
+        QVERIFY(sourceIndexesTester.compare());
+        QVERIFY(indexesTester.compare());
+        QCOMPARE(model.rowCount(), 2);
 
         QCOMPARE(sourceModel.data(sourceModel.index(0, 0), 0), "Token 1");
         QCOMPARE(sourceModel.data(sourceModel.index(1, 0), 0), "Token 3");
@@ -859,15 +893,20 @@ private slots:
         QCOMPARE(model.data(model.index(1, 0), 0), "Token 2");
         QCOMPARE(model.data(model.index(0, 0), 0), "Token 1");
 
-        sourceModel.moveRows({}, 0, 1, {}, 2);
+        indexesTester.storeIndexesAndData();
+        sourceIndexesTester.storeIndexesAndData();
+        QVERIFY(sourceModel.moveRows({}, 0, 1, {}, 3));
+        QVERIFY(sourceIndexesTester.compare());
+        QVERIFY(indexesTester.compare());
+        QCOMPARE(model.rowCount(), 2);
 
-        QCOMPARE(sourceModel.data(sourceModel.index(0, 0), 0), "Token 3");
+       QCOMPARE(sourceModel.data(sourceModel.index(0, 0), 0), "Token 3");
-        QCOMPARE(sourceModel.data(sourceModel.index(1, 0), 0), "Token 2");
+       QCOMPARE(sourceModel.data(sourceModel.index(1, 0), 0), "Token 2");
-        QCOMPARE(sourceModel.data(sourceModel.index(2, 0), 0), "Token 1");
+       QCOMPARE(sourceModel.data(sourceModel.index(2, 0), 0), "Token 1");
 
-        QCOMPARE(model.data(model.index(2, 0), 0), {});
+       QCOMPARE(model.data(model.index(2, 0), 0), {});
-        QCOMPARE(model.data(model.index(1, 0), 0), "Token 1");
+       QCOMPARE(model.data(model.index(1, 0), 0), "Token 1");
-        QCOMPARE(model.data(model.index(0, 0), 0), "Token 2");
+       QCOMPARE(model.data(model.index(0, 0), 0), "Token 2");
     }
 
     void proxyInsertedButSourceMovesRows()