Reimplement QStringListModel::setItemData

Before this patch QStringListModel::setItemData emitted dataChanged
twice if the roles map contained both DisplayRole and EditRole.
This fixes the duplication.

Increased efficiency of QStringListModel::itemData

Task-number: QTBUG-67511
Change-Id: Ibaea17530f15627a3cb8003e5284e54001731ded
Reviewed-by: David Faure <david.faure@kdab.com>

src/corelib/itemmodels/qstringlistmodel.cpp

@@ -135,6 +135,42 @@ QModelIndex QStringListModel::sibling(int row, int column, const QModelIndex &id
     return createIndex(row, 0);
 }
 
+/*!
+  \reimp
+  \since 5.13
+*/
+QMap<int, QVariant> QStringListModel::itemData(const QModelIndex &index) const
+{
+    if (!checkIndex(index, CheckIndexOption::IndexIsValid | CheckIndexOption::ParentIsInvalid))
+        return QMap<int, QVariant>{};
+    const QVariant displayData = lst.at(index.row());
+    return QMap<int, QVariant>{{
+        std::make_pair<int>(Qt::DisplayRole, displayData),
+        std::make_pair<int>(Qt::EditRole, displayData)
+    }};
+}
+
+/*!
+  \reimp
+  \since 5.13
+  If \a roles contains both Qt::DisplayRole and Qt::EditRole, the latter will take precedence
+*/
+bool QStringListModel::setItemData(const QModelIndex &index, const QMap<int, QVariant> &roles)
+{
+    if (roles.isEmpty())
+        return false;
+    if (std::any_of(roles.keyBegin(), roles.keyEnd(), [](int role) -> bool {
+        return role != Qt::DisplayRole && role != Qt::EditRole;
+    })) {
+        return false;
+    }
+    auto roleIter = roles.constFind(Qt::EditRole);
+    if (roleIter == roles.constEnd())
+        roleIter = roles.constFind(Qt::DisplayRole);
+    Q_ASSERT(roleIter != roles.constEnd());
+    return setData(index, roleIter.value(), roleIter.key());
+}
+
 /*!
     Returns data for the specified \a role, from the item with the
     given \a index.
@@ -185,13 +221,7 @@ bool QStringListModel::setData(const QModelIndex &index, const QVariant &value,
     if (index.row() >= 0 && index.row() < lst.size()
         && (role == Qt::EditRole || role == Qt::DisplayRole)) {
         lst.replace(index.row(), value.toString());
-        QVector<int> roles;
+        emit dataChanged(index, index, {Qt::DisplayRole, Qt::EditRole});
-        roles.reserve(2);
-        roles.append(Qt::DisplayRole);
-        roles.append(Qt::EditRole);
-        emit dataChanged(index, index, roles);
-        // once Q_COMPILER_UNIFORM_INIT can be used, change to:
-        // emit dataChanged(index, index, {Qt::DisplayRole, Qt::EditRole});
         return true;
     }
     return false;

src/corelib/itemmodels/qstringlistmodel.h

@@ -69,6 +69,9 @@ public:
     bool removeRows(int row, int count, const QModelIndex &parent = QModelIndex()) override;
     bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild) override;
 
+    QMap<int, QVariant> itemData(const QModelIndex &index) const override;
+    bool setItemData(const QModelIndex &index, const QMap<int, QVariant> &roles) override;
+
     void sort(int column, Qt::SortOrder order = Qt::AscendingOrder) override;
 
     QStringList stringList() const;

tests/auto/corelib/itemmodels/qstringlistmodel/tst_qstringlistmodel.cpp

@@ -87,6 +87,9 @@ private slots:
     void moveRows();
     void moveRowsInvalid_data();
     void moveRowsInvalid();
+
+    void itemData();
+    void setItemData();
 };
 
 void tst_QStringListModel::moveRowsInvalid_data()
@@ -351,6 +354,74 @@ void tst_QStringListModel::setData_emits_both_roles()
              expected);
 }
 
+void tst_QStringListModel::itemData()
+{
+    QStringListModel testModel{ QStringList {
+        QStringLiteral("One"),
+        QStringLiteral("Two"),
+        QStringLiteral("Three"),
+        QStringLiteral("Four"),
+        QStringLiteral("Five")
+    }};
+    QMap<int, QVariant> compareMap;
+    QCOMPARE(testModel.itemData(QModelIndex()), compareMap);
+    compareMap.insert(Qt::DisplayRole, QStringLiteral("Two"));
+    compareMap.insert(Qt::EditRole, QStringLiteral("Two"));
+    QCOMPARE(testModel.itemData(testModel.index(1, 0)), compareMap);
+}
+
+void tst_QStringListModel::setItemData()
+{
+    QStringListModel testModel{ QStringList {
+        QStringLiteral("One"),
+        QStringLiteral("Two"),
+        QStringLiteral("Three"),
+        QStringLiteral("Four"),
+        QStringLiteral("Five")
+    }};
+    QSignalSpy dataChangedSpy(&testModel, &QAbstractItemModel::dataChanged);
+    QModelIndex changeIndex = testModel.index(1, 0);
+    const QVector<int> changeRoles{Qt::DisplayRole, Qt::EditRole};
+    const QString changedString("Changed");
+    QMap<int, QVariant> newItemData{std::make_pair<int>(Qt::DisplayRole, changedString)};
+    // invalid index does nothing and returns false
+    QVERIFY(!testModel.setItemData(QModelIndex(), newItemData));
+    // valid data is set, return value is true and dataChanged is emitted once
+    QVERIFY(testModel.setItemData(changeIndex, newItemData));
+    QCOMPARE(changeIndex.data(Qt::DisplayRole).toString(), changedString);
+    QCOMPARE(changeIndex.data(Qt::EditRole).toString(), changedString);
+    QCOMPARE(dataChangedSpy.size(), 1);
+    QVariantList dataChangedArguments = dataChangedSpy.takeFirst();
+    QCOMPARE(dataChangedArguments.at(0).value<QModelIndex>(), changeIndex);
+    QCOMPARE(dataChangedArguments.at(1).value<QModelIndex>(), changeIndex);
+    QCOMPARE(dataChangedArguments.at(2).value<QVector<int> >(), changeRoles);
+    // Unsupported roles do nothing return false
+    newItemData.clear();
+    newItemData.insert(Qt::UserRole, changedString);
+    QVERIFY(!testModel.setItemData(changeIndex, newItemData));
+    QCOMPARE(dataChangedSpy.size(), 0);
+    // If some but not all the roles are supported it returns false and does nothing
+    newItemData.insert(Qt::EditRole, changedString);
+    changeIndex = testModel.index(2, 0);
+    QVERIFY(!testModel.setItemData(changeIndex, newItemData));
+    QCOMPARE(changeIndex.data(Qt::DisplayRole).toString(), QStringLiteral("Three"));
+    QCOMPARE(changeIndex.data(Qt::EditRole).toString(), QStringLiteral("Three"));
+    QCOMPARE(dataChangedSpy.size(), 0);
+    // Qt::EditRole and Qt::DisplayRole are both set, Qt::EditRole takes precedence
+    newItemData.clear();
+    newItemData.insert(Qt::EditRole, changedString);
+    newItemData.insert(Qt::DisplayRole, QStringLiteral("Ignored"));
+    changeIndex = testModel.index(3, 0);
+    QVERIFY(testModel.setItemData(changeIndex, newItemData));
+    QCOMPARE(changeIndex.data(Qt::DisplayRole).toString(), changedString);
+    QCOMPARE(changeIndex.data(Qt::EditRole).toString(), changedString);
+    QCOMPARE(dataChangedSpy.size(), 1);
+    dataChangedArguments = dataChangedSpy.takeFirst();
+    QCOMPARE(dataChangedArguments.at(0).value<QModelIndex>(), changeIndex);
+    QCOMPARE(dataChangedArguments.at(1).value<QModelIndex>(), changeIndex);
+    QCOMPARE(dataChangedArguments.at(2).value<QVector<int> >(), changeRoles);
+}
+
 void tst_QStringListModel::supportedDragDropActions()
 {
     QStringListModel model;