QGIM: implement moveRows

Change-Id: I55cab809cd23ccc2d6ccfb9167df81fd510acbb6
Reviewed-by: Axel Spoerl <axel.spoerl@qt.io>

src/corelib/itemmodels/qgenericitemmodel.cpp

@@ -728,6 +728,23 @@ bool QGenericItemModel::removeRows(int row, int count, const QModelIndex &parent
     return impl->call<bool>(QGenericItemModelImplBase::RemoveRows, row, count, parent);
 }
 
+/*!
+    \reimp
+
+    Moves \a count rows starting with the given \a sourceRow under parent
+    \a sourceParent to row \a destinationRow under parent \a destinationParent.
+
+    Returns \c{true} if the rows were successfully moved; otherwise returns
+    \c{false}.
+*/
+bool QGenericItemModel::moveRows(const QModelIndex &sourceParent, int sourceRow, int count,
+                                 const QModelIndex &destinationParent, int destinationRow)
+{
+    return impl->call<bool>(QGenericItemModelImplBase::MoveRows,
+                            sourceParent, sourceRow, count,
+                            destinationParent, destinationRow);
+}
+
 QT_END_NAMESPACE
 
 #include "moc_qgenericitemmodel.cpp"

src/corelib/itemmodels/qgenericitemmodel.h

@@ -7,6 +7,8 @@
 #include <QtCore/qgenericitemmodel_impl.h>
 #include <QtCore/qmap.h>
 
+#include <algorithm>
+
 QT_BEGIN_NAMESPACE
 
 class Q_CORE_EXPORT QGenericItemModel : public QAbstractItemModel
@@ -72,6 +74,8 @@ public:
     bool removeColumns(int column, int count, const QModelIndex &parent = {}) override;
     bool insertRows(int row, int count, const QModelIndex &parent = {}) override;
     bool removeRows(int row, int count, const QModelIndex &parent = {}) override;
+    bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count,
+                  const QModelIndex &destParent, int destRow) override;
 
 private:
     Q_DISABLE_COPY_MOVE(QGenericItemModel)
@@ -132,6 +136,16 @@ void QGenericItemModelImplBase::endRemoveRows()
 {
     m_itemModel->endRemoveRows();
 }
+bool QGenericItemModelImplBase::beginMoveRows(const QModelIndex &sourceParent, int sourceFirst,
+                                              int sourceLast,
+                                              const QModelIndex &destParent, int destRow)
+{
+    return m_itemModel->beginMoveRows(sourceParent, sourceFirst, sourceLast, destParent, destRow);
+}
+void QGenericItemModelImplBase::endMoveRows()
+{
+    m_itemModel->endMoveRows();
+}
 
 template <typename Structure, typename Range>
 class QGenericItemModelImpl : public QGenericItemModelImplBase
@@ -280,6 +294,8 @@ public:
             break;
         case RemoveRows: makeCall(that, &Self::removeRows, r, args);
             break;
+        case MoveRows: makeCall(that, &Self::moveRows, r, args);
+            break;
         }
     }
 
@@ -849,6 +865,48 @@ public:
         }
     }
 
+    bool moveRows(const QModelIndex &sourceParent, int sourceRow, int count,
+                  const QModelIndex &destParent, int destRow)
+    {
+        if constexpr (isMutable()) {
+            if (!Structure::canMoveRows(sourceParent, destParent))
+                return false;
+
+            if (sourceParent != destParent) {
+                return that().moveRowsAcross(sourceParent, sourceRow, count,
+                                             destParent, destRow);
+            }
+
+            if (sourceRow == destRow || sourceRow == destRow - 1 || count <= 0
+             || sourceRow < 0 || sourceRow + count - 1 >= m_itemModel->rowCount(sourceParent)
+             || destRow < 0 || destRow > m_itemModel->rowCount(destParent)) {
+                return false;
+            }
+
+            range_type *source = childRange(sourceParent);
+            // moving within the same range
+            if (!beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1, destParent, destRow))
+                return false;
+
+            const auto first = std::next(std::begin(*source), sourceRow);
+            const auto middle = std::next(first, count);
+            const auto last = std::next(std::begin(*source), destRow);
+
+            if (sourceRow < destRow) // moving down
+                std::rotate(first, middle, last);
+            else // moving up
+                std::rotate(last, first, middle);
+
+            if constexpr (!rows_are_pointers)
+                that().resetParentInChildren(source);
+
+            endMoveRows();
+            return true;
+        } else {
+            return false;
+        }
+    }
+
 protected:
     ~QGenericItemModelImpl()
     {
@@ -1164,6 +1222,90 @@ protected:
              && Base::dynamicRows() && range_features::has_erase;
     }
 
+    static constexpr bool canMoveRows(const QModelIndex &, const QModelIndex &)
+    {
+        return true;
+    }
+
+    bool moveRowsAcross(const QModelIndex &sourceParent, int sourceRow, int count,
+                        const QModelIndex &destParent, int destRow)
+    {
+        // If rows are pointers, then reference to the parent row don't
+        // change, so we can move them around freely. Otherwise we need to
+        // be able to explicitly update the parent pointer.
+        if constexpr (!Base::rows_are_pointers && !tree_traits::has_setParentRow) {
+            return false;
+        } else if constexpr (!(range_features::has_insert && range_features::has_erase)) {
+            return false;
+        } else if (!this->beginMoveRows(sourceParent, sourceRow, sourceRow + count - 1,
+                                        destParent, destRow)) {
+            return false;
+        }
+
+        range_type *source = this->childRange(sourceParent);
+        range_type *destination = this->childRange(destParent);
+
+        // If we can insert data from another range into, then
+        // use that to move the old data over.
+        const auto destStart = std::next(std::begin(*destination), destRow);
+        if constexpr (range_features::has_insert_range) {
+            const auto sourceStart = std::next(std::begin(*source), sourceRow);
+            const auto sourceEnd = std::next(sourceStart, count);
+
+            destination->insert(destStart, std::move_iterator(sourceStart),
+                                           std::move_iterator(sourceEnd));
+        } else if constexpr (std::is_copy_constructible_v<row_type>) {
+            // otherwise we have to make space first, and copy later.
+            destination->insert(destStart, count, row_type{});
+        }
+
+        row_ptr parentRow = destParent.isValid()
+                          ? QGenericItemModelDetails::pointerTo(this->rowData(destParent))
+                          : nullptr;
+
+        // if the source's parent was already inside the new parent row,
+        // then the source row might have become invalid, so reset it.
+        if (parentRow == static_cast<row_ptr>(sourceParent.internalPointer())) {
+            if (sourceParent.row() < destRow) {
+                source = this->childRange(sourceParent);
+            } else {
+                // the source parent moved down within destination
+                source = this->childRange(this->createIndex(sourceParent.row() + count, 0,
+                                                            sourceParent.internalPointer()));
+            }
+        }
+
+        // move the data over and update the parent pointer
+        {
+            const auto writeStart = std::next(std::begin(*destination), destRow);
+            const auto writeEnd = std::next(writeStart, count);
+            const auto sourceStart = std::next(std::begin(*source), sourceRow);
+            const auto sourceEnd = std::next(sourceStart, count);
+
+            for (auto write = writeStart, read = sourceStart; write != writeEnd; ++write, ++read) {
+                // move data over if not already done, otherwise
+                // only fix the parent pointer
+                if constexpr (!range_features::has_insert_range)
+                    *write = std::move(*read);
+                m_protocol.setParentRow(*write, parentRow);
+            }
+            // remove the old rows from the source parent
+            source->erase(sourceStart, sourceEnd);
+        }
+
+        // Fix the parent pointers in children of both source and destination
+        // ranges, as the references to the entries might have become invalid.
+        // We don't have to do that if the rows are pointers, as in that case
+        // the references to the entries are stable.
+        if constexpr (!Base::rows_are_pointers) {
+            resetParentInChildren(destination);
+            resetParentInChildren(source);
+        }
+
+        this->endMoveRows();
+        return true;
+    }
+
     auto makeEmptyRow(const QModelIndex &parent)
     {
         // tree traversal protocol: if we are here, then it must be possible
@@ -1365,6 +1507,18 @@ protected:
         return Base::dynamicRows() && range_features::has_erase;
     }
 
+    static constexpr bool canMoveRows(const QModelIndex &source, const QModelIndex &destination)
+    {
+        return !source.isValid() && !destination.isValid();
+    }
+
+    constexpr bool moveRowsAcross(const QModelIndex &, int , int,
+                                  const QModelIndex &, int) noexcept
+    {
+        // table/flat model: can't move rows between different parents
+        return false;
+    }
+
     auto makeEmptyRow(const QModelIndex &)
     {
         if constexpr (Base::dynamicColumns()) {

src/corelib/itemmodels/qgenericitemmodel_impl.h

@@ -495,6 +495,7 @@ public:
         RemoveColumns,
         InsertRows,
         RemoveRows,
+        MoveRows,
     };
 
     void destroy()
@@ -535,6 +536,9 @@ protected:
     inline void endInsertRows();
     inline void beginRemoveRows(const QModelIndex &parent, int start, int count);
     inline void endRemoveRows();
+    inline bool beginMoveRows(const QModelIndex &sourceParent, int sourceFirst, int sourceLast,
+                              const QModelIndex &destParent, int destRow);
+    inline void endMoveRows();
 
 public:
     template <typename Ret, typename ...Args>

tests/auto/corelib/itemmodels/qgenericitemmodel/BLACKLIST

@@ -1,4 +1,4 @@
-# QTBUG-135419
+# QTBUG-135419 - all tests of tree modifications fail on vxworks
 [setData:value tree]
 vxworks
 [setData:pointer tree]
@@ -15,9 +15,15 @@ vxworks
 vxworks
 [removeRows:pointer tree]
 vxworks
+[moveRows:value tree]
+vxworks
+[moveRows:pointer tree]
+vxworks
 [treeModifyBranch]
 vxworks
 [treeCreateBranch]
 vxworks
 [treeRemoveBranch]
 vxworks
+[treeMoveRowBranches]
+vxworks

tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp

@@ -294,6 +294,8 @@ private slots:
     void insertRows();
     void removeRows_data() { createTestData(); }
     void removeRows();
+    void moveRows_data() { createTestData(); }
+    void moveRows();
     void insertColumns_data() { createTestData(); }
     void insertColumns();
     void removeColumns_data() { createTestData(); }
@@ -309,15 +311,13 @@ private slots:
     void treeCreateBranch();
     void treeRemoveBranch_data() { tree_data(); }
     void treeRemoveBranch();
+    void treeMoveRows_data() { tree_data(); }
+    void treeMoveRows();
+    void treeMoveRowBranches_data() { tree_data(); }
+    void treeMoveRowBranches();
 
 private:
-    enum TestedModels {
-        Lists   = 0x01,
-        Tables  = 0x02,
-        Trees   = 0x04,
-        All     = Lists|Tables|Trees,
-    };
-    void createTestData(TestedModels tested = All);
+    void createTestData();
     void createTree();
 
     QList<QPersistentModelIndex> allIndexes(QAbstractItemModel *model, const QModelIndex &parent = {})
@@ -598,7 +598,7 @@ void createBackup(QObject* object, T& model) {
 template <typename T, std::enable_if_t<!std::is_copy_assignable_v<T>, bool> = true>
 void createBackup(QObject* , T& ) {}
 
-void tst_QGenericItemModel::createTestData(TestedModels tested)
+void tst_QGenericItemModel::createTestData()
 {
     m_data.reset(new Data);
 
@@ -705,19 +705,17 @@ void tst_QGenericItemModel::createTestData(TestedModels tested)
     }) << 6 << 2 << (ChangeAction::ChangeRows | ChangeAction::SetData);
 
     // special case: tree
-    if (tested & Trees) {
-        QTest::addRow("value tree") << Factory([this]{
-            return std::unique_ptr<QAbstractItemModel>(new QGenericItemModel(m_data->m_tree.get()));
-        }) << int(std::size(*m_data->m_tree.get())) << int(std::tuple_size_v<tree_row>)
-           << (ChangeAction::ChangeRows | ChangeAction::SetData);
+    QTest::addRow("value tree") << Factory([this]{
+        return std::unique_ptr<QAbstractItemModel>(new QGenericItemModel(m_data->m_tree.get()));
+    }) << int(std::size(*m_data->m_tree.get())) << int(std::tuple_size_v<tree_row>)
+       << (ChangeAction::ChangeRows | ChangeAction::SetData);
 
-        QTest::addRow("pointer tree") << Factory([this]{
-            return std::unique_ptr<QAbstractItemModel>(
-                new QGenericItemModel(m_data->m_pointer_tree.get(), tree_row::ProtocolPointerImpl{})
-            );
-        }) << int(std::size(*m_data->m_pointer_tree.get())) << int(std::tuple_size_v<tree_row>)
-           << (ChangeAction::ChangeRows | ChangeAction::SetData);
-    }
+    QTest::addRow("pointer tree") << Factory([this]{
+        return std::unique_ptr<QAbstractItemModel>(
+            new QGenericItemModel(m_data->m_pointer_tree.get(), tree_row::ProtocolPointerImpl{})
+        );
+    }) << int(std::size(*m_data->m_pointer_tree.get())) << int(std::tuple_size_v<tree_row>)
+       << (ChangeAction::ChangeRows | ChangeAction::SetData);
 }
 
 void tst_QGenericItemModel::basics()
@@ -1135,6 +1133,49 @@ void tst_QGenericItemModel::removeRows()
     QCOMPARE(couldRemove, model->rowCount() != newRowCount);
 }
 
+void tst_QGenericItemModel::moveRows()
+{
+    QFETCH(Factory, factory);
+    auto model = factory();
+    QFETCH(const int, expectedRowCount);
+    QFETCH(const ChangeActions, changeActions);
+
+    QCOMPARE(model->rowCount(), expectedRowCount);
+    if (expectedRowCount < 3)
+        QSKIP("Model is to small for testing moveRows");
+
+    const QVariant first = model->index(0, 0).data();
+    const QVariant second = model->index(1, 0).data();
+    const QVariant last = model->index(expectedRowCount - 1, 0).data();
+
+    // try to move first to last
+    QCOMPARE(model->moveRows({}, 0, 1, {}, expectedRowCount),
+             changeActions != ChangeAction::ReadOnly);
+    if (changeActions == ChangeAction::ReadOnly)
+        return;
+
+    QCOMPARE(model->index(0, 0).data(), second); // second is now on first
+    QCOMPARE(model->index(expectedRowCount - 2, 0).data(), last); // last is now second last
+    QCOMPARE(model->index(expectedRowCount - 1, 0).data(), first);
+
+    // move all but one row to the end - this restores the order
+    QVERIFY(model->moveRows({}, 0, expectedRowCount - 1,
+                            {}, expectedRowCount));
+    QCOMPARE(model->index(0, 0).data(), first);
+    QCOMPARE(model->index(1, 0).data(), second);
+    QCOMPARE(model->index(expectedRowCount - 1, 0).data(), last);
+
+    // move the last row step by step up to the top
+    for (int row = model->rowCount() - 1; row > 0; --row)
+        QVERIFY(model->moveRow({}, row, {}, row - 1));
+    QCOMPARE(model->index(0, 0).data(), last);
+    // move all except the first row up - this restores the order again
+    QVERIFY(model->moveRows({}, 1, expectedRowCount - 1, {}, 0));
+    QCOMPARE(model->index(0, 0).data(), first);
+    QCOMPARE(model->index(1, 0).data(), second);
+    QCOMPARE(model->index(expectedRowCount - 1, 0).data(), last);
+}
+
 void tst_QGenericItemModel::insertColumns()
 {
     QFETCH(Factory, factory);
@@ -1466,5 +1507,101 @@ void tst_QGenericItemModel::treeRemoveBranch()
     QCOMPARE(model->rowCount(parent), 0);
 }
 
+void tst_QGenericItemModel::treeMoveRows()
+{
+    auto model = makeTreeModel();
+    QFETCH(const QList<int>, rowsWithChildren);
+    QFETCH(const ChangeActions, changeActions);
+    if (!changeActions.testFlag(ChangeAction::ChangeRows))
+        return;
+
+    const QList<QPersistentModelIndex> pmiList = allIndexes(model.get());
+
+    // move the first row down
+    for (int currentRow = 0; currentRow < model->rowCount(); ++currentRow) {
+        model->moveRow({}, currentRow, {}, currentRow + 2);
+        QVERIFY(treeIntegrityCheck());
+    }
+
+    // move the last row back up
+    for (int currentRow = model->rowCount() - 1; currentRow > 0; --currentRow) {
+        model->moveRow({}, currentRow, {}, currentRow - 1);
+        QVERIFY(treeIntegrityCheck());
+    }
+
+    verifyPmiList(pmiList);
+}
+
+void tst_QGenericItemModel::treeMoveRowBranches()
+{
+    auto model = makeTreeModel();
+    QFETCH(const QList<int>, rowsWithChildren);
+    QFETCH(const ChangeActions, changeActions);
+
+#if QT_CONFIG(itemmodeltester)
+    QAbstractItemModelTester modelTest(model.get());
+#endif
+
+    const QList<QPersistentModelIndex> pmiList = allIndexes(model.get());
+
+    auto rowData = [model = model.get()](int row, const QModelIndex &parent = {}) -> QVariantList
+    {
+        QVariantList data;
+        for (int i = 0; i < model->columnCount(parent); ++i)
+            data << model->data(model->index(row, i, parent));
+        return data;
+    };
+
+    int branchRow = rowsWithChildren.first();
+    // those operations invalidate the model index, so get a fresh one every time
+    auto branchParent = [&model, &branchRow] { return model->index(branchRow, 0); };
+
+    int oldRootCount = model->rowCount();
+    int oldBranchCount = model->rowCount(branchParent());
+
+    QPersistentModelIndex pmi = model->index(0, 0);
+    QVariantList oldData = rowData(0);
+
+    QVERIFY(treeIntegrityCheck());
+
+    // move the first toplevel child to the end of the branch
+    QCOMPARE(model->moveRow({}, 0, branchParent(), oldBranchCount),
+             changeActions.testFlag(ChangeAction::ChangeRows));
+    if (!changeActions.testFlag(ChangeAction::ChangeRows))
+        return; // nothing else to test with a read-only model
+
+    QVERIFY(treeIntegrityCheck());
+    QCOMPARE(model->rowCount(), --oldRootCount);
+    // this moves the branch up
+    --branchRow;
+    QCOMPARE(model->rowCount(branchParent()), ++oldBranchCount);
+    // verify that the data has been copied
+    QCOMPARE(rowData(oldBranchCount - 1, branchParent()), oldData);
+    // make sure that the moved row has the right parent
+    QVERIFY(pmi.isValid());
+    QCOMPARE(pmi.parent(), branchParent());
+
+    pmi = model->index(0, 0, branchParent());
+    oldData = rowData(0, branchParent());
+
+    // move first child from the branch to the end of the toplevel list
+    model->moveRow(branchParent(), 0, {}, model->rowCount());
+    QCOMPARE(model->rowCount(branchParent()), --oldBranchCount);
+    QCOMPARE(model->rowCount(), ++oldRootCount);
+    QCOMPARE(rowData(oldRootCount - 1), oldData);
+    QVERIFY(pmi.isValid());
+    QVERIFY(!pmi.parent().isValid());
+
+    // move the last child one level up, right before it's own parent
+    {
+        const QModelIndex parent = branchParent();
+        const QModelIndex lastChild = model->index(model->rowCount(parent) - 1, 0, parent);
+        const auto grandParent = parent.parent();
+        QVERIFY(model->moveRow(parent, lastChild.row(), grandParent, parent.row()));
+    }
+
+    verifyPmiList(pmiList);
+}
+
 QTEST_MAIN(tst_QGenericItemModel)
 #include "tst_qgenericitemmodel.moc"