diff --git a/ui/StatusQ/include/StatusQ/concatmodel.h b/ui/StatusQ/include/StatusQ/concatmodel.h index 7a5bc40a8f..bfd49c248c 100644 --- a/ui/StatusQ/include/StatusQ/concatmodel.h +++ b/ui/StatusQ/include/StatusQ/concatmodel.h @@ -113,4 +113,8 @@ private: std::vector> m_rolesMappingToSource; std::vector m_rolesMappingInitializationFlags; std::vector m_rowCounts; + + // helpers for handling layoutChanged from source + QList m_layoutChangePersistentIndexes; + QModelIndexList m_proxyIndexes; }; diff --git a/ui/StatusQ/src/concatmodel.cpp b/ui/StatusQ/src/concatmodel.cpp index e592c8f528..197644c1f6 100644 --- a/ui/StatusQ/src/concatmodel.cpp +++ b/ui/StatusQ/src/concatmodel.cpp @@ -569,19 +569,71 @@ void ConcatModel::connectModelSlots(int index, QAbstractItemModel *model) {}, destinationRow + prefix); }); - connect(model, &QAbstractItemModel::rowsMoved, this, - [this, index] + connect(model, &QAbstractItemModel::rowsMoved, this, [this] { this->endMoveRows(); }); - connect(model, &QAbstractItemModel::layoutAboutToBeChanged, this, [this] + connect(model, &QAbstractItemModel::layoutAboutToBeChanged, this, + [this, model, index] { emit this->layoutAboutToBeChanged(); + + const auto persistentIndexes = persistentIndexList(); + auto prefix = this->countPrefix(index); + auto count = this->m_rowCounts[index]; + + for (const QModelIndex& persistentIndex : persistentIndexes) { + if (persistentIndex.row() < prefix || persistentIndex.row() >= count + prefix) + continue; + + m_proxyIndexes << persistentIndex; + Q_ASSERT(persistentIndex.isValid()); + const auto srcIndex = model->index( + persistentIndex.row() - prefix, + persistentIndex.column()); + + Q_ASSERT(srcIndex.isValid()); + m_layoutChangePersistentIndexes << srcIndex; + } }); - connect(model, &QAbstractItemModel::layoutChanged, this, [this] + connect(model, &QAbstractItemModel::layoutChanged, this, [this, model, index] { + auto prefix = this->countPrefix(index); + auto oldCount = m_rowCounts[index]; + auto newCount = model->rowCount(); + auto countDiff = oldCount - newCount; + + // Update row count if necessary as it can change along with layout + // change + if (countDiff != 0) + m_rowCounts[index] = newCount; + + for (int i = 0; i < m_proxyIndexes.size(); ++i) { + auto p = m_layoutChangePersistentIndexes.at(i); + + changePersistentIndex(m_proxyIndexes.at(i), this->index( + p.row() + prefix, p.column(), p.parent())); + } + + // If count has changed, it's necessary to update all indexes relating + // to the following source models (shift by countDiff). + if (countDiff != 0) { + const auto persistentIndexes = persistentIndexList(); + + for (const QModelIndex& p : persistentIndexes) { + if (p.row() < prefix + oldCount) + continue; + + changePersistentIndex(p, this->index(p.row() - countDiff, + p.column(), p.parent())); + } + } + + m_layoutChangePersistentIndexes.clear(); + m_proxyIndexes.clear(); + emit this->layoutChanged(); }); diff --git a/ui/StatusQ/tests/CMakeLists.txt b/ui/StatusQ/tests/CMakeLists.txt index 0b47391a91..097fd10cfb 100644 --- a/ui/StatusQ/tests/CMakeLists.txt +++ b/ui/StatusQ/tests/CMakeLists.txt @@ -83,7 +83,7 @@ target_link_libraries(SumAggregatorTest PRIVATE StatusQ StatusQTestLib) add_test(NAME SumAggregatorTest COMMAND SumAggregatorTest) add_executable(ConcatModelTest tst_ConcatModel.cpp) -target_link_libraries(ConcatModelTest PRIVATE StatusQ StatusQTestLib) +target_link_libraries(ConcatModelTest PRIVATE StatusQ StatusQTestLib SortFilterProxyModel) add_test(NAME ConcatModelTest COMMAND ConcatModelTest) add_executable(WritableProxyModelTest tst_WritableProxyModel.cpp) diff --git a/ui/StatusQ/tests/tst_ConcatModel.cpp b/ui/StatusQ/tests/tst_ConcatModel.cpp index 2e33520cda..27a93f6cb5 100644 --- a/ui/StatusQ/tests/tst_ConcatModel.cpp +++ b/ui/StatusQ/tests/tst_ConcatModel.cpp @@ -10,6 +10,12 @@ #include #include +#include +#include +#include + +#include +#include namespace { // Workaround for https://bugreports.qt.io/browse/QTBUG-57971 (ListModel doesn't @@ -2027,6 +2033,255 @@ private slots: QCOMPARE(dataChangedSpy.count(), 0); } } + + void sortingTest() { + QQmlEngine engine; + + auto source1 = R"([ + { "name": "D", "subname": "d1" }, + { "name": "A", "subname": "a1" }, + { "name": "B", "subname": "b1" }, + { "name": "C", "subname": "c1" }, + { "name": "B", "subname": "b2" }, + { "name": "C", "subname": "c2" } + ])"; + + auto source2 = R"([ + { "name": "A", "subname": "a1" }, + { "name": "G", "subname": "g1" }, + { "name": "F", "subname": "f1" }, + { "name": "C", "subname": "c1" } + ])"; + + ListModelWrapper sourceModel1(engine, source1); + ListModelWrapper sourceModel2(engine, source2); + + QSortFilterProxyModel sfpm1; + sfpm1.setSourceModel(sourceModel1); + + QSortFilterProxyModel sfpm2; + sfpm2.setSourceModel(sourceModel2); + + ConcatModel model; + + QQmlListProperty sources = model.sources(); + + SourceModel sm1, sm2; + sm1.setModel(&sfpm1); + sm2.setModel(&sfpm2); + + sources.append(&sources, &sm1); + sources.append(&sources, &sm2); + + model.componentComplete(); + + ModelSignalsSpy signalsSpy(&model); + PersistentIndexesTester indexesTester(&model); + + sfpm1.setSortRole(1); + sfpm1.sort(0, Qt::AscendingOrder); + + sfpm2.setSortRole(1); + sfpm2.sort(0, Qt::DescendingOrder); + + auto roles = model.roleNames(); + + QCOMPARE(model.data(model.index(0), roleForName(roles, "subname")), "a1"); + QCOMPARE(model.data(model.index(1), roleForName(roles, "subname")), "b1"); + QCOMPARE(model.data(model.index(2), roleForName(roles, "subname")), "b2"); + QCOMPARE(model.data(model.index(3), roleForName(roles, "subname")), "c1"); + QCOMPARE(model.data(model.index(4), roleForName(roles, "subname")), "c2"); + QCOMPARE(model.data(model.index(5), roleForName(roles, "subname")), "d1"); + QCOMPARE(model.data(model.index(6), roleForName(roles, "subname")), "g1"); + QCOMPARE(model.data(model.index(7), roleForName(roles, "subname")), "f1"); + QCOMPARE(model.data(model.index(8), roleForName(roles, "subname")), "c1"); + QCOMPARE(model.data(model.index(9), roleForName(roles, "subname")), "a1"); + + QCOMPARE(sfpm1.roleNames().value(1), "subname"); + QVERIFY(indexesTester.compare()); + } + + void layoutChangeWithRemovalTest() { + ConcatModel model; + + TestModel sourceModel1({ + { "name", { "name 1_1", "name 1_2", "name 1_3", "name 1_4" }} + }); + + TestModel sourceModel2({ + { "name", { "name 2_1", "name 2_2", "name 2_3" }} + }); + + QQmlListProperty sources = model.sources(); + + SourceModel source1, source2; + source1.setModel(&sourceModel1); + source2.setModel(&sourceModel2); + + sources.append(&sources, &source1); + sources.append(&sources, &source2); + + model.componentComplete(); + QCOMPARE(model.rowCount(), 7); + + QPersistentModelIndex pmi1 = model.index(0); + QPersistentModelIndex pmi2 = model.index(1); + QPersistentModelIndex pmi3 = model.index(2); + QPersistentModelIndex pmi4 = model.index(3); + QPersistentModelIndex pmi5 = model.index(4); + QPersistentModelIndex pmi6 = model.index(5); + QPersistentModelIndex pmi7 = model.index(6); + + sourceModel1.removeEverySecond(); + QCOMPARE(model.rowCount(), 5); + + QCOMPARE(pmi1.isValid(), false); + QCOMPARE(pmi2.isValid(), true); + QCOMPARE(pmi3.isValid(), false); + QCOMPARE(pmi4.isValid(), true); + QCOMPARE(pmi5.isValid(), true); + QCOMPARE(pmi6.isValid(), true); + QCOMPARE(pmi7.isValid(), true); + + QCOMPARE(pmi2.row(), 0); + QCOMPARE(pmi4.row(), 1); + QCOMPARE(pmi5.row(), 2); + QCOMPARE(pmi6.row(), 3); + QCOMPARE(pmi7.row(), 4); + + sourceModel2.removeEverySecond(); + QCOMPARE(model.rowCount(), 3); + + auto roles = model.roleNames(); + + QCOMPARE(model.data(model.index(0), roleForName(roles, "name")), "name 1_2"); + QCOMPARE(model.data(model.index(1), roleForName(roles, "name")), "name 1_4"); + QCOMPARE(model.data(model.index(2), roleForName(roles, "name")), "name 2_2"); + + QCOMPARE(pmi1.isValid(), false); + QCOMPARE(pmi2.isValid(), true); + QCOMPARE(pmi3.isValid(), false); + QCOMPARE(pmi4.isValid(), true); + QCOMPARE(pmi5.isValid(), false); + QCOMPARE(pmi6.isValid(), true); + QCOMPARE(pmi7.isValid(), false); + + QCOMPARE(pmi2.row(), 0); + QCOMPARE(pmi4.row(), 1); + QCOMPARE(pmi6.row(), 2); + } + + void sfpmTest() { + // This test reproduces in C++ situation that happens in the following + // QML code. The initial filtering is notified from a SFPM as + // a layoutAboutToBeChanged/layoutChanged signals with row count change. + // Concat model must manage properly row count update and persistent + // indexes. + // + // import QtQuick 2.15 + // import QtQuick.Controls 2.15 + + // import StatusQ 0.1 + // import SortFilterProxyModel 0.2 + + // Item { + // ListModel { + // id: src + // ListElement { name: "A" } + // ListElement { name: "D" } + // ListElement { name: "A" } + // ListElement { name: "Z" } + // ListElement { name: "A" } + // } + // SortFilterProxyModel { + // id: sfpm + // sourceModel: src + // filters: ValueFilter { + // roleName: "name" + // value: "A" + // } + // } + // ConcatModel { + // id: concat + // sources: SourceModel { model: sfpm } + // } + // Flow { + // Repeater { + // model: concat + // Button { text: model.name } + // } + // } + // } + + QQmlEngine engine; + + TestModel sourceModel({ + { "name", { "A", "B", "A" }}, + { "subname", { "a1", "b1", "a2" }} + }); + + qqsfpm::QQmlSortFilterProxyModel sfpm; + ConcatModel model; + + ModelSignalsSpy signalsSpy(&sfpm); + + sfpm.classBegin(); + model.classBegin(); + + qqsfpm::ValueFilter filter; + filter.setRoleName("name"); + filter.setValue("A"); + sfpm.appendFilter(&filter); + + QList layoutAboutToBeChangedSizes; + QList layoutChangedSizes; + + connect(&sfpm, &QAbstractItemModel::layoutAboutToBeChanged, this, + [&sfpm, &layoutAboutToBeChangedSizes]() { + layoutAboutToBeChangedSizes << sfpm.rowCount(); + }); + + connect(&sfpm, &QAbstractItemModel::layoutChanged, this, + [&sfpm, &layoutChangedSizes]() { + layoutChangedSizes << sfpm.rowCount(); + }); + + QQmlListProperty sources = model.sources(); + SourceModel sm; + sm.setModel(&sfpm); + sources.append(&sources, &sm); + sfpm.setSourceModel(&sourceModel); + + // The crucial part is the fact that concat model is completed before + // SFPM. It triggers filtering reported via + // layoutAboutToBeChanged/layoutChanged. The same behavior can be observed + // also for two SFPMs, it's not specific to ConcatModel. + model.componentComplete(); + sfpm.componentComplete(); + + QVERIFY(layoutAboutToBeChangedSizes.size() > 0); + QVERIFY(layoutAboutToBeChangedSizes.size() + == layoutChangedSizes.size()); + + // Checking whether the assumed circumstances have occurred. There should + // be at least one pair of layoutAboutToBeChanged/layoutChanged where + // on layoutAboutToBeChanged the count is smaller than in the following + // layoutChanged. + QList diffs; + + for (auto i = 0; i < layoutAboutToBeChangedSizes.size(); i++) + diffs << std::abs(layoutAboutToBeChangedSizes[i] + - layoutChangedSizes[i]); + + QVERIFY(std::accumulate(diffs.cbegin(), diffs.cend(), 0) != 0); + + QCOMPARE(model.rowCount(), 2); + + auto roles = model.roleNames(); + + QCOMPARE(model.data(model.index(0), roleForName(roles, "subname")), "a1"); + QCOMPARE(model.data(model.index(1), roleForName(roles, "subname")), "a2"); + } }; QTEST_MAIN(TestConcatModel)