From 1382374deaa4a854aeb542e6c8f7e1841f2abb10 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Thu, 5 Oct 2017 16:20:36 +0200 Subject: Correct QStandardItemModel::setItemData to follow QAbstractItemModel QStandardItemModel::setItemData replaces the content of an item data with the new values rather than updating/inserting which is the behavior for QAbstractItemModel. This patch aims to unify the behavior. [ChangeLog][QtWidgets][QStandardItemModel] Fixed setItemData() incorrectly deleting unmodified data. That behavior is not following QAbstractItemModel's documented behavior which is no modification of data not provided in parameter. Task-number: QTBUG-45114 Task-number: QTBUG-10872 Change-Id: I2be40cee372b68d9f71c976548ecda6dc3011241 Reviewed-by: David Faure --- src/gui/itemmodels/qstandarditemmodel.cpp | 93 +++++++++++++++++++--- src/gui/itemmodels/qstandarditemmodel_p.h | 11 +++ .../qstandarditemmodel/tst_qstandarditemmodel.cpp | 62 +++++++++++++++ 3 files changed, 156 insertions(+), 10 deletions(-) diff --git a/src/gui/itemmodels/qstandarditemmodel.cpp b/src/gui/itemmodels/qstandarditemmodel.cpp index 07e372b1ae..519995e82a 100644 --- a/src/gui/itemmodels/qstandarditemmodel.cpp +++ b/src/gui/itemmodels/qstandarditemmodel.cpp @@ -185,6 +185,72 @@ void QStandardItemPrivate::childDeleted(QStandardItem *child) emit model->dataChanged(modelIndex, modelIndex); } +namespace { + + struct ByNormalizedRole + { + static int normalizedRole(int role) + { + return role == Qt::EditRole ? Qt::DisplayRole : role; + } + + bool operator()(const QStandardItemData& standardItemData, const std::pair& roleMapIt) const + { + return standardItemData.role < normalizedRole(roleMapIt.first); + } + bool operator()(const std::pair& roleMapIt, const QStandardItemData& standardItemData) const + { + return normalizedRole(roleMapIt.first) < standardItemData.role; + } + + }; + + /* + Based on std::transform with a twist. The inputs are iterators of pair. + The variant is checked for validity and if not valid, that element is not taken into account + which means that the resulting output might be shorter than the input. + */ + template + OutputIt roleMapStandardItemDataTransform(Input first1, Input last1, OutputIt d_first) + { + while (first1 != last1) { + if ((*first1).second.isValid()) + *d_first++ = QStandardItemData(*first1); + ++first1; + } + return d_first; + } + + + /* + Based on std::set_union with a twist. The idea is to create a union of both inputs + with an additional constraint: if an input contains an invalid variant, it means + that this one should not be taken into account for generating the output. + */ + template + OutputIt roleMapStandardItemDataUnion(Input1 first1, Input1 last1, + Input2 first2, Input2 last2, + OutputIt d_first, Compare comp) + { + for (; first1 != last1; ++d_first) { + if (first2 == last2) { + return roleMapStandardItemDataTransform(first1, last1, d_first); + } + if (comp(*first2, *first1)) { + *d_first = *first2++; + } else { + if ((*first1).second.isValid()) + *d_first = QStandardItemData(*first1); + if (!comp(*first1, *first2)) + ++first2; + ++first1; + } + } + return std::copy(first2, last2, d_first); + } +} + /*! \internal */ @@ -192,18 +258,25 @@ void QStandardItemPrivate::setItemData(const QMap &roles) { Q_Q(QStandardItem); - //let's build the vector of new values + auto byRole = [](const QStandardItemData& item1, const QStandardItemData& item2) { + return item1.role < item2.role; + }; + + std::sort(values.begin(), values.end(), byRole); + + /* + Create a vector of QStandardItemData that will contain the original values + if the matching role is not contained in roles, the new value if it is and + if the new value is an invalid QVariant, it will be removed. + */ QVector newValues; - for (auto it = roles.begin(), end = roles.end(); it != end; ++it) { - const QVariant &value = it.value(); - if (value.isValid()) { - int role = it.key(); - role = (role == Qt::EditRole) ? Qt::DisplayRole : role; - newValues.append(QStandardItemData(role, value)); - } - } + newValues.reserve(values.size()); + roleMapStandardItemDataUnion(roles.keyValueBegin(), + roles.keyValueEnd(), + values.cbegin(), values.cend(), + std::back_inserter(newValues), ByNormalizedRole()); - if (values!=newValues) { + if (newValues != values) { values.swap(newValues); if (model) model->d_func()->itemChanged(q); diff --git a/src/gui/itemmodels/qstandarditemmodel_p.h b/src/gui/itemmodels/qstandarditemmodel_p.h index 516cce8613..caee3ea34c 100644 --- a/src/gui/itemmodels/qstandarditemmodel_p.h +++ b/src/gui/itemmodels/qstandarditemmodel_p.h @@ -61,6 +61,7 @@ #include #include #include +#include QT_BEGIN_NAMESPACE @@ -69,6 +70,7 @@ class QStandardItemData public: inline QStandardItemData() : role(-1) {} inline QStandardItemData(int r, const QVariant &v) : role(r), value(v) {} + inline QStandardItemData(const std::pair &p) : role(p.first), value(p.second) {} int role; QVariant value; inline bool operator==(const QStandardItemData &other) const { return role == other.role && value == other.value; } @@ -91,6 +93,15 @@ inline QDataStream &operator<<(QDataStream &out, const QStandardItemData &data) return out; } +inline QDebug &operator<<(QDebug &debug, const QStandardItemData &data) +{ + QDebugStateSaver saver(debug); + debug.nospace() << data.role + << " " + << data.value; + return debug.space(); +} + #endif // QT_NO_DATASTREAM class QStandardItemPrivate diff --git a/tests/auto/gui/itemmodels/qstandarditemmodel/tst_qstandarditemmodel.cpp b/tests/auto/gui/itemmodels/qstandarditemmodel/tst_qstandarditemmodel.cpp index 2f5537adfe..a07f45ef1d 100644 --- a/tests/auto/gui/itemmodels/qstandarditemmodel/tst_qstandarditemmodel.cpp +++ b/tests/auto/gui/itemmodels/qstandarditemmodel/tst_qstandarditemmodel.cpp @@ -130,6 +130,8 @@ private slots: void getMimeDataWithInvalidModelIndex(); void supportedDragDropActions(); + void taskQTBUG_45114_setItemData(); + private: QAbstractItemModel *m_model; QPersistentModelIndex persistent; @@ -1701,5 +1703,65 @@ void tst_QStandardItemModel::supportedDragDropActions() QCOMPARE(model.supportedDropActions(), Qt::CopyAction | Qt::MoveAction); } +void tst_QStandardItemModel::taskQTBUG_45114_setItemData() +{ + QStandardItemModel model; + QSignalSpy spy(&model, &QStandardItemModel::itemChanged); + + QStandardItem *item = new QStandardItem("item"); + item->setData(1, Qt::UserRole + 1); + item->setData(2, Qt::UserRole + 2); + model.appendRow(item); + + QModelIndex index = item->index(); + QCOMPARE(model.itemData(index).size(), 3); + + QCOMPARE(spy.count(), 0); + + QMap roles; + + roles.insert(Qt::UserRole + 1, 1); + roles.insert(Qt::UserRole + 2, 2); + model.setItemData(index, roles); + + QCOMPARE(spy.count(), 0); + + roles.insert(Qt::UserRole + 1, 1); + roles.insert(Qt::UserRole + 2, 2); + roles.insert(Qt::UserRole + 3, QVariant()); + model.setItemData(index, roles); + + QCOMPARE(spy.count(), 0); + + roles.clear(); + roles.insert(Qt::UserRole + 1, 10); + roles.insert(Qt::UserRole + 3, 12); + model.setItemData(index, roles); + + QCOMPARE(spy.count(), 1); + QMap itemRoles = model.itemData(index); + + QCOMPARE(itemRoles.size(), 4); + QCOMPARE(itemRoles[Qt::UserRole + 1].toInt(), 10); + QCOMPARE(itemRoles[Qt::UserRole + 2].toInt(), 2); + QCOMPARE(itemRoles[Qt::UserRole + 3].toInt(), 12); + + roles.clear(); + roles.insert(Qt::UserRole + 3, 1); + model.setItemData(index, roles); + + QCOMPARE(spy.count(), 2); + + roles.clear(); + roles.insert(Qt::UserRole + 3, QVariant()); + model.setItemData(index, roles); + + QCOMPARE(spy.count(), 3); + + itemRoles = model.itemData(index); + QCOMPARE(itemRoles.size(), 3); + QVERIFY(!itemRoles.keys().contains(Qt::UserRole + 3)); +} + QTEST_MAIN(tst_QStandardItemModel) #include "tst_qstandarditemmodel.moc" -- cgit v1.2.3