QGIM: simplify new/deleteRow for tables

Conceptually, a table can deal with rows that are a pointer type,
and nullptr. We test for validity before accessing the row element, and
can return gracefully from e.g. data() or setData(). This is also
acceptable for lists, where a newly constructed row will hold a nullptr
element as the item. Client code that wants to initialize new rows
explicitly can provide their own protocol, or connect to rowsInserted()
and rowsAboutToBeRemoved().

However, we might end up violating the requirement that in a table, all
rows have to have the same number of columns: if the row-type is a
pointer to a struct or gadget, and we don't instantiate the element,
then we end up with an index that is de-facto invalid even though
it is in range. setData() cannot do anything but return false, and
data() can only return an invalid QVariant.
And if the row type is dynamically sized, and the new row is inserted
at index 0, then we end up reporting that the model has 0 columns, as we
use row 0 as the reference for the column count.

To fix this, implement new/deleteRow consistently for pointers (and
smart pointers): if the row type is a (smart) pointer holding
a default-constructible object, then we can instantiate a new object.
The deleteRow implementation takes care of deleting raw pointers (as
otherwise the `delete row` expression is invalid); for smart pointers,
the smart pointer takes care of memory management.

All other row types can be instantiated by default constructing. This
covers MultiColumn/SingleColumn wrappers as well, which can hold a
nullptr while still reporting the correct column count.

Remove the special handling from canInsertRows() - if newRow() is
available in the protocol, then we can insert rows.

But one ambiguity remains: if we operate on a copy of a container that
has pointers for rows, then adding new rows might allocate (in the
model's copy), resulting in a mixed situation. The client code can not
delete those new rows (they don't exist in the original container),
while the model must not delete rows it didn't create (those do exist
in the original container and might still be used).

Perhaps that cannot be solved, other than by documenting: either move
your range in the model (then ownership is clear), or pass a reference
or pointer the model (then ownership is also clear); or use smart
pointers for the row type.

Change-Id: I18a2e929473d118dcdb9d1f2ed67a7890f681974
Reviewed-by: Artem Dyomin <artem.dyomin@qt.io>

src/corelib/itemmodels/qgenericitemmodel.h

@@ -874,8 +874,7 @@ public:
             { // erase invalidates iterators
                 const auto begin = QGenericItemModelDetails::pos(children, row);
                 const auto end = std::next(begin, count);
-                if constexpr (rows_are_raw_pointers)
+                that().deleteRemovedRows(begin, end);
-                    that().deleteRemovedRows(begin, end);
                 children->erase(begin, end);
             }
             // fix the parent in all children of the modified row, as the
@@ -939,16 +938,18 @@ public:
 protected:
     ~QGenericItemModelImpl()
     {
-        if constexpr (rows_are_raw_pointers && !std::is_pointer_v<Range>
+        // We delete row objects if we are not operating on a reference or pointer
-                   && !QGenericItemModelDetails::is_any_of<Range, std::reference_wrapper,
+        // to a range, as in that case, the owner of the referenced/pointed to
-                                                                  std::shared_ptr,
+        // range also owns the row entries.
-                                                                  QSharedPointer,
+        // ### Problem: if we get a copy of a range (no matter if shared or not),
-                                                                  std::unique_ptr>()) {
+        // then adding rows will create row objects in the model's copy, and the
-            // If data with rows as pointers was moved in, then we own it and
+        // client can never delete those. But copied rows will be the same pointer,
-            // have to delete those rows.
+        // which we must not delete (as we didn't create them).
-            using ref = decltype(std::forward<Range>(std::declval<range_type>()));
+        if constexpr (protocol_traits::has_deleteRow && !std::is_pointer_v<Range>
-            if constexpr (std::is_rvalue_reference_v<ref>)
+                   && !QGenericItemModelDetails::is_any_of<Range, std::reference_wrapper>()) {
-                that().destroyOwnedModel(*m_data.model());
+            const auto begin = QGenericItemModelDetails::begin(*m_data.model());
+            const auto end = QGenericItemModelDetails::end(*m_data.model());
+            that().deleteRemovedRows(begin, end);
         }
     }
 
@@ -958,10 +959,8 @@ protected:
             // If we operate on dynamic columns and cannot resize a newly
             // constructed row, then we cannot insert.
             return false;
-        } else if constexpr (!one_dimensional_range && !rows_are_raw_pointers
+        } else if constexpr (!protocol_traits::has_newRow) {
-                          && !protocol_traits::initializes_rows) {
+            // We also cannot insert if we cannot create a new row element
-            // We also cannot insert if the row we have to create is not
-            // default-initialized by the protocol
             return false;
         } else if constexpr (!range_features::has_insert_range
                           && !std::is_copy_constructible_v<row_type>) {
@@ -1414,19 +1413,13 @@ protected:
         return empty_row;
     }
 
-    template <typename R>
-    void destroyOwnedModel(R &&range)
-    {
-        const auto begin = QGenericItemModelDetails::begin(range);
-        const auto end = QGenericItemModelDetails::end(range);
-        deleteRemovedRows(begin, end);
-    }
-
     template <typename It, typename Sentinel>
     void deleteRemovedRows(It &&begin, Sentinel &&end)
     {
-        for (auto it = begin; it != end; ++it)
+        if constexpr (tree_traits::has_deleteRow) {
-            this->protocol().deleteRow(*it);
+            for (auto it = begin; it != end; ++it)
+                this->protocol().deleteRow(*it);
+        }
     }
 
     void resetParentInChildren(range_type *children)
@@ -1627,19 +1620,13 @@ protected:
         return empty_row;
     }
 
-    template <typename R>
-    void destroyOwnedModel(R &&range)
-    {
-        const auto begin = QGenericItemModelDetails::begin(range);
-        const auto end = QGenericItemModelDetails::end(range);
-        for (auto it = begin; it !=  end; ++it)
-            delete *it;
-    }
-
     template <typename It, typename Sentinel>
-    void deleteRemovedRows(It &&, Sentinel &&)
+    void deleteRemovedRows(It &&begin, Sentinel &&end)
     {
-        // nothing to do for tables and lists as we never create rows either
+        if constexpr (Base::protocol_traits::has_deleteRow) {
+            for (auto it = begin; it != end; ++it)
+                this->protocol().deleteRow(*it);
+        }
     }
 
     decltype(auto) rowDataImpl(const QModelIndex &index) const

src/corelib/itemmodels/qgenericitemmodel_impl.h

@@ -72,8 +72,8 @@ namespace QGenericItemModelDetails
                                            QExplicitlySharedDataPointer, QSharedDataPointer>;
 
     template <typename T>
-    using is_owning_or_raw_pointer = std::disjunction<is_any_shared_ptr<T>, is_any_unique_ptr<T>, std::is_pointer<T>>;
+    using is_owning_or_raw_pointer = std::disjunction<is_any_shared_ptr<T>, is_any_unique_ptr<T>,
-
+                                                      std::is_pointer<T>>;
 
     template <typename T>
     static auto pointerTo(T&& t) {
@@ -354,10 +354,9 @@ namespace QGenericItemModelDetails
         using row_type = typename range_traits<wrapped_t<Range>>::value_type;
 
         template <typename R = row_type,
-                  std::enable_if_t<std::is_destructible_v<wrapped_t<R>>, bool> = true,
+                  std::enable_if_t<std::conjunction_v<std::is_destructible<wrapped_t<R>>,
-                  std::enable_if_t<is_owning_or_raw_pointer<R>::value, bool> = true>
+                                                      is_owning_or_raw_pointer<R>>, bool> = true>
-        auto newRow() -> decltype(R(new wrapped_t<R>))
+        auto newRow() -> decltype(R(new wrapped_t<R>)) {
-        {
             if constexpr (is_any_of<R, std::shared_ptr>())
                 return std::make_shared<wrapped_t<R>>();
             else
@@ -365,13 +364,12 @@ namespace QGenericItemModelDetails
         }
 
         template <typename R = row_type,
-                  std::enable_if_t<std::negation_v<is_wrapped<R>>, bool> = true,
+                  std::enable_if_t<!is_owning_or_raw_pointer<R>::value, bool> = true>
-                  std::enable_if_t<std::negation_v<is_validatable<R>>, bool> = true>
         auto newRow() -> decltype(R{}) { return R{}; }
 
-        template <typename R,
+        template <typename R = row_type,
-                  std::enable_if_t<std::is_pointer_v<R>, bool> = true>
+                  std::enable_if_t<std::is_pointer_v<std::remove_reference_t<R>>, bool> = true>
-        auto deleteRow(R row) -> decltype(delete(row)) { delete row; }
+        auto deleteRow(R&& row) -> decltype(delete row) { delete row; }
     };
 
     template <typename Range, typename R = typename range_traits<wrapped_t<Range>>::value_type>
@@ -485,7 +483,6 @@ namespace QGenericItemModelDetails
         static constexpr bool has_setParentRow = protocol_setParentRow<protocol, row>();
         static constexpr bool has_mutable_childRows = protocol_mutable_childRows<protocol, row>();
 
-        static constexpr bool initializes_rows = has_newRow && !is_any_of<protocol, ListProtocol>();
         static constexpr bool is_default = is_any_of<protocol, ListProtocol, TableProtocol, DefaultTreeProtocol>();
     };
 

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

@@ -474,7 +474,7 @@ private:
         Object row2;
         Object row3;
         std::list<Object *> listOfObjects = {
-            &row1, &row2, &row3
+            new Object, new Object, new Object
         };
 
         MetaObjectTuple mot1;
@@ -896,10 +896,12 @@ void tst_QGenericItemModel::ownership()
         std::vector<Object *> objects {
             object
         };
-        { // model does not take ownership
+        { // model takes ownership of its own copy of the vector (!)
             QGenericItemModel modelOnCopy(objects);
         }
-        QVERIFY(guard);
+        QVERIFY(!guard);
+        objects[0] = new Object;
+        guard = objects[0];
         { // model does not take ownership
             QGenericItemModel modelOnPointer(&objects);
         }
@@ -911,6 +913,7 @@ void tst_QGenericItemModel::ownership()
 
         { // model does take ownership
             QGenericItemModel movedIntoModel(std::move(objects));
+            QCOMPARE(movedIntoModel.columnCount(), 2);
         }
         QVERIFY(!guard);
     }