From 6fe2067c224ea8c87a95f2470f163b98fe128e7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Cie=C5=9Blak?= Date: Thu, 21 Dec 2023 09:59:19 +0100 Subject: [PATCH] fix(ConcatModel): Changed behavior on submodel destruction to not emit signals The general rule should be to avoid any actions on destruction different than setting relevant pointers to null. Other actions like emiting signals are potentially dangerous when the whole objects hierarchy is under destruction. Closes: #13065 --- ui/StatusQ/include/StatusQ/concatmodel.h | 7 ++-- ui/StatusQ/src/concatmodel.cpp | 22 +---------- ui/StatusQ/tests/tst_ConcatModel.cpp | 49 ++++++++---------------- 3 files changed, 21 insertions(+), 57 deletions(-) diff --git a/ui/StatusQ/include/StatusQ/concatmodel.h b/ui/StatusQ/include/StatusQ/concatmodel.h index bc4081722f..7a5bc40a8f 100644 --- a/ui/StatusQ/include/StatusQ/concatmodel.h +++ b/ui/StatusQ/include/StatusQ/concatmodel.h @@ -3,6 +3,7 @@ #include #include #include +#include #include @@ -27,13 +28,11 @@ public: signals: void modelAboutToBeChanged(); - void modelChanged(bool deleted); + void modelChanged(); void markerRoleValueChanged(); private: - void onModelDestroyed(); - - QAbstractItemModel* m_model = nullptr; + QPointer m_model; QString m_markerRoleValue; }; diff --git a/ui/StatusQ/src/concatmodel.cpp b/ui/StatusQ/src/concatmodel.cpp index 305923fc9b..4505af34a7 100644 --- a/ui/StatusQ/src/concatmodel.cpp +++ b/ui/StatusQ/src/concatmodel.cpp @@ -90,18 +90,11 @@ void SourceModel::setModel(QAbstractItemModel* model) if (m_model == model) return; - if (model) - connect(model, &QObject::destroyed, this, &SourceModel::onModelDestroyed); - - if (m_model) - disconnect(m_model, &QObject::destroyed, this, &SourceModel::onModelDestroyed); - emit modelAboutToBeChanged(); m_model = model; - emit modelChanged(false); + emit modelChanged(); } - /*! \qmlproperty any StatusQ::SourceModel::model @@ -133,12 +126,6 @@ const QString& SourceModel::markerRoleValue() const return m_markerRoleValue; } -void SourceModel::onModelDestroyed() -{ - m_model = nullptr; - emit modelChanged(true); -} - ConcatModel::ConcatModel(QObject* parent) : QAbstractListModel{parent} @@ -350,15 +337,10 @@ void ConcatModel::componentComplete() }); connect(source, &SourceModel::modelChanged, this, - [this, source, i](bool deleted) + [this, source, i]() { auto previousRowCount = m_rowCounts[i]; - if (deleted) { - auto prefix = countPrefix(i); - beginRemoveRows({}, prefix, prefix + previousRowCount - 1); - } - if (previousRowCount) { m_rowCounts[i] = 0; endRemoveRows(); diff --git a/ui/StatusQ/tests/tst_ConcatModel.cpp b/ui/StatusQ/tests/tst_ConcatModel.cpp index f2f75d8430..f4d3523d57 100644 --- a/ui/StatusQ/tests/tst_ConcatModel.cpp +++ b/ui/StatusQ/tests/tst_ConcatModel.cpp @@ -1379,43 +1379,26 @@ private slots: QSignalSpy rowsAboutToBeRemovedSpy(&model, &ConcatModel::rowsAboutToBeRemoved); QSignalSpy rowsRemovedSpy(&model, &ConcatModel::rowsRemoved); - // checking validity inside rowsAboutToBeRemoved signal - { - QObject context; - connect(&model, &ConcatModel::rowsAboutToBeRemoved, &context, - [this, &model, &roles] { - QCOMPARE(model.rowCount(), 6); + sourceModel2.reset(); - QCOMPARE(model.data(model.index(0, 0), roleForName(roles, "key")), 1); - QCOMPARE(model.data(model.index(1, 0), roleForName(roles, "key")), 2); - QCOMPARE(model.data(model.index(2, 0), roleForName(roles, "key")), {}); - QCOMPARE(model.data(model.index(3, 0), roleForName(roles, "key")), {}); - QCOMPARE(model.data(model.index(4, 0), roleForName(roles, "key")), 5); - QCOMPARE(model.data(model.index(5, 0), roleForName(roles, "key")), 6); + QCOMPARE(model.rowCount(), 6); - QCOMPARE(model.data(model.index(0, 0), roleForName(roles, "whichModel")), ""); - QCOMPARE(model.data(model.index(1, 0), roleForName(roles, "whichModel")), ""); - QCOMPARE(model.data(model.index(2, 0), roleForName(roles, "whichModel")), ""); - QCOMPARE(model.data(model.index(3, 0), roleForName(roles, "whichModel")), ""); - QCOMPARE(model.data(model.index(4, 0), roleForName(roles, "whichModel")), ""); - QCOMPARE(model.data(model.index(5, 0), roleForName(roles, "whichModel")), ""); - }); + QCOMPARE(rowsAboutToBeRemovedSpy.count(), 0); + QCOMPARE(rowsRemovedSpy.count(), 0); - sourceModel2.reset(); - } + QCOMPARE(model.data(model.index(0, 0), roleForName(roles, "key")), 1); + QCOMPARE(model.data(model.index(1, 0), roleForName(roles, "key")), 2); + QCOMPARE(model.data(model.index(2, 0), roleForName(roles, "key")), {}); + QCOMPARE(model.data(model.index(3, 0), roleForName(roles, "key")), {}); + QCOMPARE(model.data(model.index(4, 0), roleForName(roles, "key")), 5); + QCOMPARE(model.data(model.index(5, 0), roleForName(roles, "key")), 6); - QCOMPARE(model.rowCount(), 4); - - QCOMPARE(rowsAboutToBeRemovedSpy.count(), 1); - QCOMPARE(rowsRemovedSpy.count(), 1); - - QCOMPARE(rowsAboutToBeRemovedSpy.at(0).at(0), QModelIndex{}); - QCOMPARE(rowsAboutToBeRemovedSpy.at(0).at(1), 2); - QCOMPARE(rowsAboutToBeRemovedSpy.at(0).at(2), 3); - - QCOMPARE(rowsRemovedSpy.at(0).at(0), QModelIndex{}); - QCOMPARE(rowsRemovedSpy.at(0).at(1), 2); - QCOMPARE(rowsRemovedSpy.at(0).at(2), 3); + QCOMPARE(model.data(model.index(0, 0), roleForName(roles, "whichModel")), ""); + QCOMPARE(model.data(model.index(1, 0), roleForName(roles, "whichModel")), ""); + QCOMPARE(model.data(model.index(2, 0), roleForName(roles, "whichModel")), ""); + QCOMPARE(model.data(model.index(3, 0), roleForName(roles, "whichModel")), ""); + QCOMPARE(model.data(model.index(4, 0), roleForName(roles, "whichModel")), ""); + QCOMPARE(model.data(model.index(5, 0), roleForName(roles, "whichModel")), ""); } void removalTest()