Implement QTableModel::moveRows

Implemented the virtual method moveRows to allow row movement.
Used it for the case of sorted insertion.

Heavily based on QListModel::moveRows and its unittest.

[ChangeLog][QtWidgets][QTableWidget] Implemented moveRows in model.
When sorting is enabled, setItem() now moves the row to its
sorted position, emitting rowsMoved rather than layoutChanged.

Task-number: QTBUG-69807
Change-Id: I62a150cca4e5b7d982f2359a6d8c248494528cac
Reviewed-by: Christian Ehrlicher <ch.ehrlicher@gmx.de>
(cherry picked from commit f951c11586081efea108ea5b6a7028c672e12c2a)

src/widgets/itemviews/qtablewidget.cpp

@@ -114,6 +114,46 @@ bool QTableModel::removeColumns(int column, int count, const QModelIndex &)
     return true;
 }
 
+bool QTableModel::moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild)
+{
+    if (sourceRow < 0
+        || sourceRow + count - 1 >= rowCount(sourceParent)
+        || destinationChild < 0
+        || destinationChild > rowCount(destinationParent)
+        || sourceRow == destinationChild
+        || sourceRow == destinationChild - 1
+        || count <= 0
+        || sourceParent.isValid()
+        || destinationParent.isValid()) {
+        return false;
+    }
+    if (!beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1, destinationParent, destinationChild))
+        return false;
+
+    // Table items
+    int numItems = count * columnCount();
+    int fromIndex = tableIndex(sourceRow, 0);
+    int destinationIndex = tableIndex(destinationChild, 0);
+    if (destinationChild < sourceRow)
+        fromIndex += numItems - 1;
+    else
+        destinationIndex--;
+    while (numItems--)
+        tableItems.move(fromIndex, destinationIndex);
+
+    // Header items
+    int fromRow = sourceRow;
+    if (destinationChild < sourceRow)
+        fromRow += count - 1;
+    else
+        destinationChild--;
+    while (count--)
+        verticalHeaderItems.move(fromRow, destinationChild);
+
+    endMoveRows();
+    return true;
+}
+
 void QTableModel::setItem(int row, int column, QTableWidgetItem *item)
 {
     int i = tableIndex(row, column);
@@ -152,27 +192,8 @@ void QTableModel::setItem(int row, int column, QTableWidgetItem *item)
             sortedRow = qMax((int)(it - colItems.begin()), 0);
         }
         if (sortedRow != row) {
-            emit layoutAboutToBeChanged({}, QAbstractItemModel::VerticalSortHint);
+            const int destinationChild = sortedRow > row ? sortedRow + 1 : sortedRow;
-            // move the items @ row to sortedRow
+            moveRows(QModelIndex(), row, 1, QModelIndex(), destinationChild);
-            int cc = columnCount();
-            QList<QTableWidgetItem *> rowItems(cc);
-            for (int j = 0; j < cc; ++j)
-                rowItems[j] = tableItems.at(tableIndex(row, j));
-            tableItems.remove(tableIndex(row, 0), cc);
-            tableItems.insert(tableIndex(sortedRow, 0), cc, 0);
-            for (int j = 0; j < cc; ++j)
-                tableItems[tableIndex(sortedRow, j)] = rowItems.at(j);
-            QTableWidgetItem *header = verticalHeaderItems.at(row);
-            verticalHeaderItems.remove(row);
-            verticalHeaderItems.insert(sortedRow, header);
-            // update persistent indexes
-            QModelIndexList oldPersistentIndexes = persistentIndexList();
-            QModelIndexList newPersistentIndexes = oldPersistentIndexes;
-            updateRowIndexes(newPersistentIndexes, row, sortedRow);
-            changePersistentIndexList(oldPersistentIndexes,
-                                      newPersistentIndexes);
-
-            emit layoutChanged({}, QAbstractItemModel::VerticalSortHint);
             return;
         }
     }

src/widgets/itemviews/qtablewidget_p.h

@@ -64,6 +64,8 @@ public:
     bool removeRows(int row, int count = 1, const QModelIndex &parent = QModelIndex()) override;
     bool removeColumns(int column, int count = 1, const QModelIndex &parent = QModelIndex()) override;
 
+    bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count, const QModelIndex &destinationParent, int destinationChild) override;
+
     void setItem(int row, int column, QTableWidgetItem *item);
     QTableWidgetItem *takeItem(int row, int column);
     QTableWidgetItem *item(int row, int column) const;

tests/auto/widgets/itemviews/qtablewidget/tst_qtablewidget.cpp

@@ -83,6 +83,10 @@ private slots:
 #endif
     void createPersistentOnLayoutAboutToBeChanged();
     void createPersistentOnLayoutAboutToBeChangedAutoSort();
+    void moveRows_data();
+    void moveRows();
+    void moveRowsInvalid_data();
+    void moveRowsInvalid();
 
 private:
     std::unique_ptr<QTableWidget> testWidget;
@@ -1390,6 +1394,7 @@ void tst_QTableWidget::setItemWithSorting()
 
         QSignalSpy dataChangedSpy(model, &QAbstractItemModel::dataChanged);
         QSignalSpy layoutChangedSpy(model, &QAbstractItemModel::layoutChanged);
+        QSignalSpy rowsMovedSpy(model, &QAbstractItemModel::rowsMoved);
 
         if (i == 0) {
             // set a new item
@@ -1415,12 +1420,14 @@ void tst_QTableWidget::setItemWithSorting()
             QCOMPARE(persistent.at(k).data().toString(), expectedValues.at(i));
         }
 
-        if (i == 0)
+        if (i == 0) {
             QCOMPARE(dataChangedSpy.size(), reorderingExpected ? 0 : 1);
-        else
+            QCOMPARE(rowsMovedSpy.size(), reorderingExpected ? 1 : 0);
+        } else {
             QCOMPARE(dataChangedSpy.size(), 1);
+            QCOMPARE(layoutChangedSpy.size(), reorderingExpected ? 1 : 0);
+        }
 
-        QCOMPARE(layoutChangedSpy.size(), reorderingExpected ? 1 : 0);
     }
 }
 
@@ -1927,5 +1934,118 @@ void tst_QTableWidget::createPersistentOnLayoutAboutToBeChangedAutoSort() // QTB
     QCOMPARE(layoutChangedSpy.size(), 1);
 }
 
+void tst_QTableWidget::moveRows_data()
+{
+    QTest::addColumn<int>("startRow");
+    QTest::addColumn<int>("count");
+    QTest::addColumn<int>("destination");
+    QTest::addColumn<QStringList>("expected");
+    QTest::newRow("1_Item_from_top_to_middle") << 0 << 1 << 3 << QStringList{"B", "C", "A", "D", "E", "F"};
+    QTest::newRow("1_Item_from_top_to_bottom") << 0 << 1 << 6 << QStringList{"B", "C", "D", "E", "F", "A"};
+    QTest::newRow("1_Item_from_middle_to_top") << 2 << 1 << 0 << QStringList{"C", "A", "B", "D", "E", "F"};
+    QTest::newRow("1_Item_from_bottom_to_middle") << 5 << 1 << 2 << QStringList{"A", "B", "F", "C", "D", "E"};
+    QTest::newRow("1_Item_from_bottom to_top") << 5 << 1 << 0 << QStringList{"F", "A", "B", "C", "D", "E"};
+    QTest::newRow("1_Item_from_middle_to_bottom") << 2 << 1 << 6 << QStringList{"A", "B", "D", "E", "F", "C"};
+    QTest::newRow("1_Item_from_middle_to_middle_before") << 2 << 1 << 1 << QStringList{"A", "C", "B", "D", "E", "F"};
+    QTest::newRow("1_Item_from_middle_to_middle_after") << 2 << 1 << 4 << QStringList{"A", "B", "D", "C", "E", "F"};
+
+    QTest::newRow("2_Items_from_top_to_middle") << 0 << 2 << 3 << QStringList{"C", "A", "B", "D", "E", "F"};
+    QTest::newRow("2_Items_from_top_to_bottom") << 0 << 2 << 6 << QStringList{"C", "D", "E", "F", "A", "B"};
+    QTest::newRow("2_Items_from_middle_to_top") << 2 << 2 << 0 << QStringList{"C", "D", "A", "B", "E", "F"};
+    QTest::newRow("2_Items_from_bottom_to_middle") << 4 << 2 << 2 << QStringList{"A", "B", "E", "F", "C", "D"};
+    QTest::newRow("2_Items_from_bottom_to_top") << 4 << 2 << 0 << QStringList{"E", "F", "A", "B", "C", "D"};
+    QTest::newRow("2_Items_from_middle_to_bottom") << 2 << 2 << 6 << QStringList{"A", "B", "E", "F", "C", "D"};
+    QTest::newRow("2_Items_from_middle_to_middle_before") << 3 << 2 << 1 << QStringList{"A", "D", "E", "B", "C", "F"};
+    QTest::newRow("2_Items_from_middle_to_middle_after") << 1 << 2 << 5 << QStringList{"A", "D", "E", "B", "C", "F"};
+}
+
+void tst_QTableWidget::moveRows()
+{
+    QFETCH(const int, startRow);
+    QFETCH(const int, count);
+    QFETCH(const int, destination);
+    QFETCH(const QStringList, expected);
+    QTableWidget baseWidget;
+    baseWidget.setRowCount(6);
+    baseWidget.setColumnCount(2);
+    for (int r = 0; r < 6; ++r) {
+        baseWidget.setItem(r, 0, new QTableWidgetItem(QString(QLatin1Char('A' + r)))); // "A", "B", "C", "D", "E", "F"
+        baseWidget.setItem(r, 1, new QTableWidgetItem(QString(QLatin1Char('a' + r)))); // "a", "b", "c", "d", "e", "f"
+    }
+    QAbstractItemModel *baseModel = baseWidget.model();
+    QSignalSpy rowMovedSpy(baseModel, &QAbstractItemModel::rowsMoved);
+    QSignalSpy rowAboutMovedSpy(baseModel, &QAbstractItemModel::rowsAboutToBeMoved);
+    QVERIFY(baseModel->moveRows(QModelIndex(), startRow, count, QModelIndex(), destination));
+    QCOMPARE(baseModel->rowCount(), expected.size());
+    for (int i = 0; i < expected.size(); ++i) {
+        QCOMPARE(baseModel->index(i, 0).data().toString(), expected.at(i));
+        QCOMPARE(baseModel->index(i, 1).data().toString(), expected.at(i).toLower());
+    }
+    QCOMPARE(rowMovedSpy.size(), 1);
+    QCOMPARE(rowAboutMovedSpy.size(), 1);
+    for (const QList<QVariant> &signalArgs : {rowMovedSpy.first(), rowAboutMovedSpy.first()}){
+        QVERIFY(!signalArgs.at(0).value<QModelIndex>().isValid());
+        QCOMPARE(signalArgs.at(1).toInt(), startRow);
+        QCOMPARE(signalArgs.at(2).toInt(), startRow + count - 1);
+        QVERIFY(!signalArgs.at(3).value<QModelIndex>().isValid());
+        QCOMPARE(signalArgs.at(4).toInt(), destination);
+    }
+}
+
+void tst_QTableWidget::moveRowsInvalid_data()
+{
+    QTest::addColumn<QTableWidget*>("baseWidget");
+    QTest::addColumn<QModelIndex>("startParent");
+    QTest::addColumn<int>("startRow");
+    QTest::addColumn<int>("count");
+    QTest::addColumn<QModelIndex>("destinationParent");
+    QTest::addColumn<int>("destination");
+
+    constexpr int rowCount = 6;
+    const auto createWidget = []() -> QTableWidget* {
+        QTableWidget* result = new QTableWidget;
+        result->setRowCount(rowCount);
+        result->setColumnCount(1);
+        int c = 0;
+        for (int r = 0; r < rowCount; ++r)
+            result->setItem(r, c, new QTableWidgetItem(QString(QLatin1Char('A' + r)))); // "A", "B", "C", "D", "E", "F"
+        return result;
+    };
+
+    QTest::addRow("destination_equal_source") << createWidget() << QModelIndex() << 0 << 1 << QModelIndex() << 0;
+    QTest::addRow("count_equal_0") << createWidget() << QModelIndex() << 0 << 0 << QModelIndex() << 2;
+    QTableWidget* tempWidget = createWidget();
+    QTest::addRow("move_child") << tempWidget << tempWidget->model()->index(0, 0) << 0 << 1 << QModelIndex() << 2;
+    tempWidget = createWidget();
+    QTest::addRow("move_to_child") << tempWidget << QModelIndex() << 0 << 1 << tempWidget->model()->index(0, 0) << 2;
+    QTest::addRow("negative_count") << createWidget() << QModelIndex() << 0 << -1 << QModelIndex() << 2;
+    QTest::addRow("negative_source_row") << createWidget() << QModelIndex() << -1 << 1 << QModelIndex() << 2;
+    QTest::addRow("negative_destination_row") << createWidget() << QModelIndex() << 0 << 1 << QModelIndex() << -1;
+    QTest::addRow("source_row_equal_rowCount") << createWidget() << QModelIndex() << rowCount << 1 << QModelIndex() << 1;
+    QTest::addRow("source_row_equal_destination_row") << createWidget() << QModelIndex() << 2 << 1 << QModelIndex() << 2;
+    QTest::addRow("source_row_equal_destination_row_plus1") << createWidget() << QModelIndex() << 2 << 1 << QModelIndex() << 3;
+    QTest::addRow("destination_row_greater_rowCount") << createWidget() << QModelIndex() << 0 << 1 << QModelIndex() << rowCount + 1;
+    QTest::addRow("move_row_within_source_range") << createWidget() << QModelIndex() << 0 << 3 << QModelIndex() << 2;
+}
+
+void tst_QTableWidget::moveRowsInvalid()
+{
+    QFETCH(QTableWidget* const, baseWidget);
+    QFETCH(const QModelIndex, startParent);
+    QFETCH(const int, startRow);
+    QFETCH(const int, count);
+    QFETCH(const QModelIndex, destinationParent);
+    QFETCH(const int, destination);
+    QAbstractItemModel *baseModel = baseWidget->model();
+    QSignalSpy rowMovedSpy(baseModel, &QAbstractItemModel::rowsMoved);
+    QSignalSpy rowAboutMovedSpy(baseModel, &QAbstractItemModel::rowsAboutToBeMoved);
+    QVERIFY(rowMovedSpy.isValid());
+    QVERIFY(rowAboutMovedSpy.isValid());
+    QVERIFY(!baseModel->moveRows(startParent, startRow, count, destinationParent, destination));
+    QCOMPARE(rowMovedSpy.size(), 0);
+    QCOMPARE(rowAboutMovedSpy.size(), 0);
+    delete baseWidget;
+}
+
 QTEST_MAIN(tst_QTableWidget)
 #include "tst_qtablewidget.moc"