From 1c44eb8e16c1642d631e508472ca32837ad4ffdf Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 12 Feb 2025 17:37:32 +0100 Subject: [PATCH] QGIM: implement moveRows Change-Id: I55cab809cd23ccc2d6ccfb9167df81fd510acbb6 Reviewed-by: Axel Spoerl --- src/corelib/itemmodels/qgenericitemmodel.cpp | 17 ++ src/corelib/itemmodels/qgenericitemmodel.h | 154 +++++++++++++++ .../itemmodels/qgenericitemmodel_impl.h | 4 + .../itemmodels/qgenericitemmodel/BLACKLIST | 8 +- .../tst_qgenericitemmodel.cpp | 177 ++++++++++++++++-- 5 files changed, 339 insertions(+), 21 deletions(-) diff --git a/src/corelib/itemmodels/qgenericitemmodel.cpp b/src/corelib/itemmodels/qgenericitemmodel.cpp index 24fd37c5710..830d0361d93 100644 --- a/src/corelib/itemmodels/qgenericitemmodel.cpp +++ b/src/corelib/itemmodels/qgenericitemmodel.cpp @@ -728,6 +728,23 @@ bool QGenericItemModel::removeRows(int row, int count, const QModelIndex &parent return impl->call(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(QGenericItemModelImplBase::MoveRows, + sourceParent, sourceRow, count, + destinationParent, destinationRow); +} + QT_END_NAMESPACE #include "moc_qgenericitemmodel.cpp" diff --git a/src/corelib/itemmodels/qgenericitemmodel.h b/src/corelib/itemmodels/qgenericitemmodel.h index e57814425e3..a25d7fc6a4d 100644 --- a/src/corelib/itemmodels/qgenericitemmodel.h +++ b/src/corelib/itemmodels/qgenericitemmodel.h @@ -7,6 +7,8 @@ #include #include +#include + 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 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) { + // 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(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()) { diff --git a/src/corelib/itemmodels/qgenericitemmodel_impl.h b/src/corelib/itemmodels/qgenericitemmodel_impl.h index 83fbf8ecd93..fefcad5ef75 100644 --- a/src/corelib/itemmodels/qgenericitemmodel_impl.h +++ b/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 diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/BLACKLIST b/tests/auto/corelib/itemmodels/qgenericitemmodel/BLACKLIST index 9c6f8b06de8..21bb1048aeb 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/BLACKLIST +++ b/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 diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp index b4d96d71ce8..cd53b43dd8d 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp +++ b/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 allIndexes(QAbstractItemModel *model, const QModelIndex &parent = {}) @@ -598,7 +598,7 @@ void createBackup(QObject* object, T& model) { template , 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(new QGenericItemModel(m_data->m_tree.get())); - }) << int(std::size(*m_data->m_tree.get())) << int(std::tuple_size_v) - << (ChangeAction::ChangeRows | ChangeAction::SetData); + QTest::addRow("value tree") << Factory([this]{ + return std::unique_ptr(new QGenericItemModel(m_data->m_tree.get())); + }) << int(std::size(*m_data->m_tree.get())) << int(std::tuple_size_v) + << (ChangeAction::ChangeRows | ChangeAction::SetData); - QTest::addRow("pointer tree") << Factory([this]{ - return std::unique_ptr( - 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) - << (ChangeAction::ChangeRows | ChangeAction::SetData); - } + QTest::addRow("pointer tree") << Factory([this]{ + return std::unique_ptr( + 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) + << (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, rowsWithChildren); + QFETCH(const ChangeActions, changeActions); + if (!changeActions.testFlag(ChangeAction::ChangeRows)) + return; + + const QList 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, rowsWithChildren); + QFETCH(const ChangeActions, changeActions); + +#if QT_CONFIG(itemmodeltester) + QAbstractItemModelTester modelTest(model.get()); +#endif + + const QList 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"