From 07804a83c4019dca396adee079ad14e4b9b18cb8 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Mon, 17 Mar 2025 18:45:52 +0100 Subject: [PATCH] QGIM: support move-only row types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A move-only type can be used as the row if the container of the rows can be populated via insert from another container. This is the case for the standard containers, but not for e.g. QList. If an insert(pos, start, end) overload is available, then use that also in the moveRows() implementation to move the data over. The only limitation now is that we cannot assign a complete row type as data of an item, as we cannot get a move-only value out of a const QVariant. Change-Id: Icea66ed82c601fd819f7db2eca2f8e023d8aa7c2 Reviewed-by: Tor Arne Vestbø --- src/corelib/itemmodels/qgenericitemmodel.h | 52 +++++++++++++++++-- .../itemmodels/qgenericitemmodel_impl.h | 17 ++++++ .../qgenericitemmodel/CMakeLists.txt | 6 ++- .../tst_qgenericitemmodel.cpp | 31 ++++------- 4 files changed, 79 insertions(+), 27 deletions(-) diff --git a/src/corelib/itemmodels/qgenericitemmodel.h b/src/corelib/itemmodels/qgenericitemmodel.h index bd1b2296bee..ba8f729ce8a 100644 --- a/src/corelib/itemmodels/qgenericitemmodel.h +++ b/src/corelib/itemmodels/qgenericitemmodel.h @@ -196,6 +196,38 @@ protected: std::is_pointer_v, Range, std::remove_reference_t> >; + + // A iterator type to use as the input iterator with the + // range_type::insert(pos, start, end) overload if available (it is in + // std::vector, but not in QList). Generates a prvalue when dereferenced, + // which then gets moved into the newly constructed row, which allows us to + // implement insertRows() for move-only row types. + struct EmptyRowGenerator + { + using value_type = row_type; + using reference = value_type; + using pointer = value_type *; + using iterator_category = std::input_iterator_tag; + using difference_type = int; + + value_type operator*() { return impl.makeEmptyRow(parent); } + EmptyRowGenerator &operator++() { ++n; return *this; } + friend bool operator==(const EmptyRowGenerator &lhs, const EmptyRowGenerator &rhs) noexcept + { return lhs.n == rhs.n; } + friend bool operator!=(const EmptyRowGenerator &lhs, const EmptyRowGenerator &rhs) noexcept + { return !(lhs == rhs); } + + difference_type n = 0; + Structure &impl; + const QModelIndex parent; + }; + + // If we have a move-only row_type and can add/remove rows, then the range + // must have an insert-from-range overload. + static_assert(static_row_count || range_features::has_insert_range + || std::is_copy_constructible_v, + "The range holding a move-only row-type must support insert(pos, start, end)"); + public: explicit QGenericItemModelImpl(Range &&model, QGenericItemModel *itemModel) : QGenericItemModelImplBase(itemModel, static_cast(nullptr)) @@ -473,8 +505,13 @@ public: using multi_role = QGenericItemModelDetails::is_multi_role; if constexpr (has_metaobject) { if (QMetaType::fromType() == data.metaType()) { - target = data.value(); - return true; + if constexpr (std::is_copy_assignable_v) { + target = std::move(data).value(); + return true; + } else { + qCritical("Cannot assign %s", QMetaType::fromType().name()); + return false; + } } else if (row_traits::fixed_size() <= 1) { return writeRole(role, target, data); } else if (column <= row_traits::fixed_size() @@ -739,14 +776,19 @@ public: beginInsertRows(parent, row, row + count - 1); const auto pos = std::next(std::begin(*children), row); - if constexpr (rows_are_pointers) { + if constexpr (range_features::has_insert_range) { + EmptyRowGenerator first{0, that(), parent}; + EmptyRowGenerator last{count, that(), parent}; + children->insert(pos, first, last); + } else if constexpr (rows_are_pointers) { auto start = children->insert(pos, count, nullptr); auto end = std::next(start, count); for (auto it = start; it != end; ++it) *it = that().makeEmptyRow(parent); } else { - row_type empty_value = that().makeEmptyRow(parent); - children->insert(pos, count, empty_value); + children->insert(pos, count, that().makeEmptyRow(parent)); + } + if constexpr (!rows_are_pointers) { // fix the parent in all children of the modified row, as the // references back to the parent might have become invalid. that().resetParentInChildren(children); diff --git a/src/corelib/itemmodels/qgenericitemmodel_impl.h b/src/corelib/itemmodels/qgenericitemmodel_impl.h index d7e2a08ed20..e4e010b74e6 100644 --- a/src/corelib/itemmodels/qgenericitemmodel_impl.h +++ b/src/corelib/itemmodels/qgenericitemmodel_impl.h @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -74,6 +75,20 @@ namespace QGenericItemModelDetails : std::true_type {}; + // Can we insert from another (identical) range? Required to support + // move-only types + template + struct test_insert_range : std::false_type {}; + + template + struct test_insert_range().insert( + std::declval(), + std::declval&>(), + std::declval&>() + ))>> + : std::true_type + {}; + template struct test_erase : std::false_type {}; @@ -127,6 +142,7 @@ namespace QGenericItemModelDetails struct range_traits : std::false_type { static constexpr bool is_mutable = !std::is_const_v; static constexpr bool has_insert = false; + static constexpr bool has_insert_range = false; static constexpr bool has_erase = false; static constexpr bool has_resize = false; }; @@ -139,6 +155,7 @@ namespace QGenericItemModelDetails using value_type = std::remove_reference_t()))>; static constexpr bool is_mutable = !std::is_const_v && !std::is_const_v; static constexpr bool has_insert = test_insert(); + static constexpr bool has_insert_range = test_insert_range(); static constexpr bool has_erase = test_erase(); static constexpr bool has_resize = test_resize(); }; diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/CMakeLists.txt b/tests/auto/corelib/itemmodels/qgenericitemmodel/CMakeLists.txt index 72ed3b43cfb..f0e38f7259e 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/CMakeLists.txt +++ b/tests/auto/corelib/itemmodels/qgenericitemmodel/CMakeLists.txt @@ -18,7 +18,11 @@ qt_internal_add_test(tst_qgenericitemmodel Qt::Gui ) -if (NOT INTEGRITY AND NOT VXWORKS) +if ( + # INTEGRITY and VxWorks don't come with all the concepts headers + NOT INTEGRITY AND NOT VXWORKS AND + # gcc 10.2.0 chokes in a standard library header + (NOT CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_VERSION VERSION_GREATER 10.2.0)) if ("${CMAKE_CXX_COMPILE_FEATURES}" MATCHES "cxx_std_20") set_property(TARGET tst_qgenericitemmodel PROPERTY CXX_STANDARD 20) endif() diff --git a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp index d5ba4aa8e43..b4d96d71ce8 100644 --- a/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp +++ b/tests/auto/corelib/itemmodels/qgenericitemmodel/tst_qgenericitemmodel.cpp @@ -145,7 +145,7 @@ namespace std { } struct tree_row; -using value_tree = QList; +using value_tree = std::vector; using pointer_tree = QList; struct tree_row @@ -161,22 +161,6 @@ public: qDeleteAll(*m_childrenPointers); } - tree_row(const tree_row &other) - : m_value(other.m_value), m_description(other.m_description) - , m_parent(other.m_parent), m_children(other.m_children) - , m_childrenPointers(other.m_childrenPointers) - {} - - tree_row &operator=(const tree_row &other) - { - m_parent = other.m_parent; - m_children = other.m_children; - m_childrenPointers = other.m_childrenPointers; - m_value = other.m_value; - m_description = other.m_description; - return *this; - } - tree_row(tree_row &&other) = default; tree_row &operator=(tree_row &&other) = default; @@ -384,11 +368,13 @@ private: if (children) { for (const auto &child : *children) { const bool match = [&child, &row]{ + tree_row emptyRow; if constexpr (pointerTree) { if (child->parentRow() != row) { qCritical().noquote() << "Parent out of sync for:" << *child; qCritical().noquote() << " Actual: " << child->parentRow() - << (child->parentRow() ? *child->parentRow() : tree_row{}); + << std::move(child->parentRow() ? *child->parentRow() + : emptyRow); qCritical().noquote() << "Expected: " << row << *row; return false; } @@ -396,7 +382,8 @@ private: if (child.parentRow() != std::addressof(row)) { qCritical().noquote() << "Parent out of sync for:" << child; qCritical().noquote() << " Actual: " << child.parentRow() - << (child.parentRow() ? *child.parentRow() : tree_row{}); + << std::move(child.parentRow() ? *child.parentRow() + : emptyRow); qCritical().noquote() << "Expected: " << std::addressof(row) << row; return false; } @@ -1210,13 +1197,15 @@ enum class TreeProtocol { ValueImplicit, ValueReadOnly, PointerExplicit, Pointer void tst_QGenericItemModel::createTree() { - m_data->m_tree.reset(new value_tree{ + tree_row root[] = { {"1", "one"}, {"2", "two"}, {"3", "three"}, {"4", "four"}, {"5", "five"}, - }); + }; + m_data->m_tree.reset(new value_tree{std::make_move_iterator(std::begin(root)), + std::make_move_iterator(std::end(root))}); (*m_data->m_tree)[1].addChild("2.1", "two.one"); (*m_data->m_tree)[1].addChild("2.2", "two.two");