Qml[Table|Tree]Model: Move validateRow to QAbstractColumnModel

This is the 10th patch in an attempt to make them more similar and
eventually merge common parts in a common abstract class.

Task-number: QTBUG-138703
Pick-to: 6.10
Change-Id: Ibd7fcfe93deee0283a04efb2f48a3e08c2b26286
Reviewed-by: Mate Barany <mate.barany@qt.io>

src/labs/models/qqmlabstractcolumnmodel.cpp

@@ -331,6 +331,102 @@ void QQmlAbstractColumnModel::fetchColumnMetadata()
     }
 }
 
+bool QQmlAbstractColumnModel::validateRowType(QLatin1StringView functionName, const QVariant &row) const
+{
+    if (!row.canConvert<QJSValue>()) {
+        qmlWarning(this) << functionName << ": expected \"row\" argument to be a QJSValue,"
+                         << " but got " << row.typeName() << " instead:\n" << row;
+        return false;
+    }
+
+    const auto rowAsJSValue = row.value<QJSValue>();
+    if (!rowAsJSValue.isObject() && !rowAsJSValue.isArray()) {
+        qmlWarning(this) << functionName << ": expected \"row\" argument "
+                         << "to be an object or array, but got:\n" << rowAsJSValue.toString();
+        return false;
+    }
+
+    return true;
+}
+
+bool QQmlAbstractColumnModel::validateNewRow(QLatin1StringView functionName, const QVariant &row,
+                                   NewRowOperationFlag operation) const
+{
+    if (mColumnMetadata.isEmpty()) {
+        // There is no column metadata, so we have nothing to validate the row against.
+        // Rows have to be added before we can gather metadata from them, so just this
+        // once we'll return true to allow the rows to be added.
+        return true;
+    }
+
+    const bool isVariantMap = (row.userType() == QMetaType::QVariantMap);
+
+    // Don't require each row to be a QJSValue when setting all rows,
+    // as they won't be; they'll be QVariantMap.
+    if (operation != SetRowsOperation && (!isVariantMap && !validateRowType(functionName, row)))
+        return false;
+
+    const QVariant rowAsVariant = operation == SetRowsOperation || isVariantMap
+        ? row : row.value<QJSValue>().toVariant();
+    if (rowAsVariant.userType() != QMetaType::QVariantMap) {
+        qmlWarning(this) << functionName << ": row manipulation functions "
+                         << "do not support complex rows";
+        return false;
+    }
+
+    const QVariantMap rowAsMap = rowAsVariant.toMap();
+    const int columnCount = rowAsMap.size();
+    if (columnCount < mColumnCount) {
+        qmlWarning(this) << functionName << ": expected " << mColumnCount
+                         << " columns, but only got " << columnCount;
+        return false;
+    }
+
+    // We can't validate complex structures, but we can make sure that
+    // each simple string-based role in each column is correct.
+    for (int columnIndex = 0; columnIndex < mColumns.size(); ++columnIndex) {
+        QQmlTableModelColumn *column = mColumns.at(columnIndex);
+        const QHash<QString, QJSValue> getters = column->getters();
+        const auto roleNames = getters.keys();
+        const ColumnMetadata columnMetadata = mColumnMetadata.at(columnIndex);
+        for (const QString &roleName : roleNames) {
+            const ColumnRoleMetadata roleData = columnMetadata.roles.value(roleName);
+            if (roleData.columnRole == ColumnRole::FunctionRole)
+                continue;
+
+            if (!rowAsMap.contains(roleData.name)) {
+                qmlWarning(this).noquote() << functionName << ": expected a property named \""
+                                           << roleData.name << "\" in row";
+                return false;
+            }
+
+            const QVariant rolePropertyValue = rowAsMap.value(roleData.name);
+
+            if (rolePropertyValue.userType() != roleData.type) {
+                if (!rolePropertyValue.canConvert(QMetaType(roleData.type))) {
+                    qmlWarning(this).noquote() << functionName << ": expected the property named \""
+                                               << roleData.name << "\" to be of type \"" << roleData.typeName
+                                               << "\", but got \"" << QString::fromLatin1(rolePropertyValue.typeName())
+                                               << "\" instead";
+                    return false;
+                }
+
+                QVariant effectiveValue = rolePropertyValue;
+                if (!effectiveValue.convert(QMetaType(roleData.type))) {
+                    qmlWarning(this).noquote() << functionName << ": failed converting value \""
+                                               << rolePropertyValue << "\" set at column " << columnIndex << " with role \""
+                                               << QString::fromLatin1(rolePropertyValue.typeName()) << "\" to \""
+                                               << roleData.typeName << "\"";
+                    return false;
+                }
+            }
+        }
+    }
+
+    return true;
+}
+
+
 QT_END_NAMESPACE
 
 #include "moc_qqmlabstractcolumnmodel_p.cpp"

src/labs/models/qqmlabstractcolumnmodel_p.h

@@ -98,6 +98,15 @@ protected:
     ColumnRoleMetadata fetchColumnRoleData(const QString &roleNameKey, QQmlTableModelColumn *tableModelColumn, int columnIndex) const;
     void fetchColumnMetadata();
 
+    enum NewRowOperationFlag {
+        OtherOperation, // insert(), set(), etc.
+        SetRowsOperation,
+        AppendOperation
+    };
+
+    bool validateRowType(QLatin1StringView functionName, const QVariant &row) const;
+    virtual bool validateNewRow(QLatin1StringView functionName, const QVariant &row,
+                                NewRowOperationFlag operation  = OtherOperation) const;
 
     QList<QQmlTableModelColumn *> mColumns;
 

src/labs/models/qqmltablemodel.cpp

@@ -192,7 +192,7 @@ void QQmlTableModel::setRowsPrivate(const QVariantList &rowsAsVariantList)
             // validateNewRow() expects a QVariant wrapping a QJSValue, so to
             // simplify the code, just create one here.
             const QVariant row = QVariant::fromValue(rowsAsVariantList.at(rowIndex));
-            if (!validateNewRow("setRows()"_L1, row, rowIndex, SetRowsOperation))
+            if (!validateNewRow("setRows()"_L1, row, SetRowsOperation))
                 return;
         }
     }
@@ -254,7 +254,7 @@ void QQmlTableModel::setDataPrivate(const QModelIndex &index, const QString &rol
 */
 void QQmlTableModel::appendRow(const QVariant &row)
 {
-    if (!validateNewRow("appendRow()"_L1, row, -1, AppendOperation))
+    if (!validateNewRow("appendRow()"_L1, row, AppendOperation))
         return;
 
     doInsert(mRowCount, row);
@@ -297,9 +297,8 @@ void QQmlTableModel::clear()
 */
 QVariant QQmlTableModel::getRow(int rowIndex)
 {
-    if (!validateRowIndex("getRow()"_L1, "rowIndex", rowIndex))
+    if (!validateRowIndex("getRow()"_L1, "rowIndex"_L1, rowIndex, NeedsExisting))
         return QVariant();
-
     return mRows.at(rowIndex);
 }
 
@@ -326,7 +325,8 @@ QVariant QQmlTableModel::getRow(int rowIndex)
 */
 void QQmlTableModel::insertRow(int rowIndex, const QVariant &row)
 {
-    if (!validateNewRow("insertRow()"_L1, row, rowIndex))
+    if (!validateNewRow("insertRow()"_L1, row) ||
+        !validateRowIndex("insertRow()"_L1, "rowIndex"_L1, rowIndex, CanAppend))
         return;
 
     doInsert(rowIndex, row);
@@ -382,10 +382,10 @@ void QQmlTableModel::moveRow(int fromRowIndex, int toRowIndex, int rows)
         return;
     }
 
-    if (!validateRowIndex("moveRow()"_L1, "fromRowIndex", fromRowIndex))
+    if (!validateRowIndex("moveRow()"_L1, "fromRowIndex"_L1, fromRowIndex, NeedsExisting))
         return;
 
-    if (!validateRowIndex("moveRow()"_L1, "toRowIndex", toRowIndex))
+    if (!validateRowIndex("moveRow()"_L1, "toRowIndex"_L1, toRowIndex, NeedsExisting))
         return;
 
     if (fromRowIndex + rows > mRowCount) {
@@ -444,7 +444,7 @@ void QQmlTableModel::moveRow(int fromRowIndex, int toRowIndex, int rows)
 */
 void QQmlTableModel::removeRow(int rowIndex, int rows)
 {
-    if (!validateRowIndex("removeRow()"_L1, "rowIndex", rowIndex))
+    if (!validateRowIndex("removeRow()"_L1, "rowIndex"_L1, rowIndex, NeedsExisting))
         return;
 
     if (rows <= 0) {
@@ -499,7 +499,8 @@ void QQmlTableModel::removeRow(int rowIndex, int rows)
 */
 void QQmlTableModel::setRow(int rowIndex, const QVariant &row)
 {
-    if (!validateNewRow("setRow()"_L1, row, rowIndex))
+    if (!validateNewRow("setRow()"_L1, row) ||
+        !validateRowIndex("setRow()"_L1, "rowIndex"_L1, rowIndex, CanAppend))
         return;
 
     if (rowIndex != mRowCount) {
@@ -627,128 +628,26 @@ int QQmlTableModel::columnCount(const QModelIndex &parent) const
     \sa data(), index()
 */
 
-bool QQmlTableModel::validateRowType(QLatin1StringView functionName, const QVariant &row) const
+bool QQmlTableModel::validateRowIndex(QLatin1StringView functionName, QLatin1StringView argumentName,
-{
+                                      int rowIndex, RowOption operation) const
-    if (!row.canConvert<QJSValue>()) {
-        qmlWarning(this) << functionName << ": expected \"row\" argument to be a QJSValue,"
-                         << " but got " << row.typeName() << " instead:\n" << row;
-        return false;
-    }
-
-    const auto rowAsJSValue = row.value<QJSValue>();
-    if (!rowAsJSValue.isObject() && !rowAsJSValue.isArray()) {
-        qmlWarning(this) << functionName << ": expected \"row\" argument "
-                         << "to be an object or array, but got:\n" << rowAsJSValue.toString();
-        return false;
-    }
-
-    return true;
-}
-
-bool QQmlTableModel::validateNewRow(QLatin1StringView functionName, const QVariant &row,
-                                    int rowIndex, NewRowOperationFlag operation) const
-{
-    if (mColumnMetadata.isEmpty()) {
-        // There is no column metadata, so we have nothing to validate the row against.
-        // Rows have to be added before we can gather metadata from them, so just this
-        // once we'll return true to allow the rows to be added.
-        return true;
-    }
-
-    const bool isVariantMap = (row.userType() == QMetaType::QVariantMap);
-
-    // Don't require each row to be a QJSValue when setting all rows,
-    // as they won't be; they'll be QVariantMap.
-    if (operation != SetRowsOperation && (!isVariantMap && !validateRowType(functionName, row)))
-        return false;
-
-    if (operation == OtherOperation) {
-        // Inserting/setting.
-        if (rowIndex < 0) {
-            qmlWarning(this) << functionName << ": \"rowIndex\" cannot be negative";
-            return false;
-        }
-
-        if (rowIndex > mRowCount) {
-            qmlWarning(this) << functionName << ": \"rowIndex\" " << rowIndex
-                << " is greater than rowCount() of " << mRowCount;
-            return false;
-        }
-    }
-
-    const QVariant rowAsVariant = operation == SetRowsOperation || isVariantMap
-        ? row : row.value<QJSValue>().toVariant();
-    if (rowAsVariant.userType() != QMetaType::QVariantMap) {
-        qmlWarning(this) << functionName << ": row manipulation functions "
-                         << "do not support complex rows"
-                         << " (row index: " << rowIndex << ")";
-        return false;
-    }
-
-    const QVariantMap rowAsMap = rowAsVariant.toMap();
-    const int columnCount = rowAsMap.size();
-    if (columnCount < mColumnCount) {
-        qmlWarning(this) << functionName << ": expected " << mColumnCount
-                         << " columns, but only got " << columnCount;
-        return false;
-    }
-
-    // We can't validate complex structures, but we can make sure that
-    // each simple string-based role in each column is correct.
-    for (int columnIndex = 0; columnIndex < mColumns.size(); ++columnIndex) {
-        QQmlTableModelColumn *column = mColumns.at(columnIndex);
-        const QHash<QString, QJSValue> getters = column->getters();
-        const auto roleNames = getters.keys();
-        const ColumnMetadata columnMetadata = mColumnMetadata.at(columnIndex);
-        for (const QString &roleName : roleNames) {
-            const ColumnRoleMetadata roleData = columnMetadata.roles.value(roleName);
-            if (roleData.columnRole == ColumnRole::FunctionRole)
-                continue;
-
-            if (!rowAsMap.contains(roleData.name)) {
-                qmlWarning(this).noquote() << functionName << ": expected a property named \""
-                                           << roleData.name << "\" in row"
-                                           << " at index " << rowIndex << ", but couldn't find one";
-                return false;
-            }
-
-            const QVariant rolePropertyValue = rowAsMap.value(roleData.name);
-
-            if (rolePropertyValue.userType() != roleData.type) {
-                if (!rolePropertyValue.canConvert(QMetaType(roleData.type))) {
-                    qmlWarning(this).noquote() << functionName << ": expected the property named \""
-                                               << roleData.name << "\" to be of type \"" << roleData.typeName
-                                               << "\", but got \"" << QString::fromLatin1(rolePropertyValue.typeName())
-                                               << "\" instead";
-                    return false;
-                }
-
-                QVariant effectiveValue = rolePropertyValue;
-                if (!effectiveValue.convert(QMetaType(roleData.type))) {
-                    qmlWarning(this).noquote() << functionName << ": failed converting value \""
-                                               << rolePropertyValue << "\" set at column " << columnIndex << " with role \""
-                                               << QString::fromLatin1(rolePropertyValue.typeName()) << "\" to \""
-                                               << roleData.typeName << "\"";
-                    return false;
-                }
-            }
-        }
-    }
-
-    return true;
-}
-
-bool QQmlTableModel::validateRowIndex(QLatin1StringView functionName, const char *argumentName, int rowIndex) const
 {
     if (rowIndex < 0) {
-        qmlWarning(this) << functionName << ": \"" << argumentName << "\" cannot be negative";
+        qmlWarning(this).noquote() << functionName << ": \"" << argumentName << "\" cannot be negative";
         return false;
     }
 
-    if (rowIndex >= mRowCount) {
+    if (operation == NeedsExisting) {
-        qmlWarning(this) << functionName << ": \"" << argumentName
+        if (rowIndex >= mRowCount) {
-            << "\" " << rowIndex << " is greater than or equal to rowCount() of " << mRowCount;
+            qmlWarning(this).noquote() << functionName << ": \"" << argumentName
-        return false;
+                << "\" " << rowIndex << " is greater than or equal to rowCount() of " << mRowCount;
+            return false;
+        }
+    } else {
+        if (rowIndex > mRowCount) {
+            qmlWarning(this).noquote() << functionName << ": \"" << argumentName
+                << "\" " << rowIndex << " is greater than rowCount() of " << mRowCount;
+            return false;
+        }
     }
 
     return true;

src/labs/models/qqmltablemodel_p.h

@@ -69,20 +69,18 @@ protected:
 
 private:
 
-    enum NewRowOperationFlag {
-        OtherOperation, // insert(), set(), etc.
-        SetRowsOperation,
-        AppendOperation
-    };
-
     void setRowsPrivate(const QVariantList &rowsAsVariantList);
     QVariant dataPrivate(const QModelIndex &index, const QString &roleName) const override;
     void setDataPrivate(const QModelIndex &index, const QString &roleName, QVariant value) override;
 
-    bool validateRowType(QLatin1StringView functionName, const QVariant &row) const;
+    enum RowOption {
-    bool validateNewRow(QLatin1StringView functionName, const QVariant &row,
+        NeedsExisting,
-        int rowIndex, NewRowOperationFlag operation = OtherOperation) const;
+        CanAppend
-    bool validateRowIndex(QLatin1StringView functionName, const char *argumentName, int rowIndex) const;
+    };
+
+
+    bool validateRowIndex(QLatin1StringView functionName,  QLatin1StringView argumentName,
+                          int rowIndex, RowOption operation) const;
 
     void doInsert(int rowIndex, const QVariant &row);
 

src/labs/models/qqmltreemodel.cpp

@@ -522,98 +522,13 @@ int QQmlTreeModel::columnCount(const QModelIndex &parent) const
     \sa data(), index()
 */
 
-bool QQmlTreeModel::validateRowType(QLatin1StringView functionName, const QVariant &row) const
-{
-    if (!row.canConvert<QJSValue>()) {
-        qmlWarning(this) << functionName << ": expected \"row\" argument to be a QJSValue,"
-                         << " but got " << row.typeName() << " instead:\n" << row;
-        return false;
-    }
-
-    const auto rowAsJSValue = row.value<QJSValue>();
-    if (!rowAsJSValue.isObject() && !rowAsJSValue.isArray()) {
-        qmlWarning(this) << functionName << ": expected \"row\" argument "
-                         << "to be an object or array, but got:\n" << rowAsJSValue.toString();
-        return false;
-    }
-
-    return true;
-}
-
 bool QQmlTreeModel::validateNewRow(QLatin1StringView functionName, const QVariant &row,
                                    NewRowOperationFlag operation) const
 {
-    if (mColumnMetadata.isEmpty()) {
-        // There is no column metadata, so we have nothing to validate the row against.
-        // Rows have to be added before we can gather metadata from them, so just this
-        // once we'll return true to allow the rows to be added.
-        return true;
-    }
-
     const bool isVariantMap = (row.userType() == QMetaType::QVariantMap);
-
-    // Don't require each row to be a QJSValue when setting all rows,
-    // as they won't be; they'll be QVariantMap.
-    if (operation != SetRowsOperation && (!isVariantMap && !validateRowType(functionName, row)))
-        return false;
-
     const QVariant rowAsVariant = operation == SetRowsOperation || isVariantMap
         ? row : row.value<QJSValue>().toVariant();
-    if (rowAsVariant.userType() != QMetaType::QVariantMap) {
-        qmlWarning(this) << functionName << ": row manipulation functions "
-                         << "do not support complex rows";
-        return false;
-    }
-
     const QVariantMap rowAsMap = rowAsVariant.toMap();
-    const int columnCount = rowAsMap.size();
-    if (columnCount < mColumnCount) {
-        qmlWarning(this) << functionName << ": expected " << mColumnCount
-                         << " columns, but only got " << columnCount;
-        return false;
-    }
-
-    // We can't validate complex structures, but we can make sure that
-    // each simple string-based role in each column is correct.
-    for (int columnIndex = 0; columnIndex < mColumns.size(); ++columnIndex) {
-        QQmlTableModelColumn *column = mColumns.at(columnIndex);
-        const QHash<QString, QJSValue> getters = column->getters();
-        const auto roleNames = getters.keys();
-        const ColumnMetadata columnMetadata = mColumnMetadata.at(columnIndex);
-        for (const QString &roleName : roleNames) {
-            const ColumnRoleMetadata roleData = columnMetadata.roles.value(roleName);
-            if (roleData.columnRole == ColumnRole::FunctionRole)
-                continue;
-
-            if (!rowAsMap.contains(roleData.name)) {
-                qmlWarning(this).noquote() << functionName << ": expected a property named \""
-                                           << roleData.name << "\" in row";
-                return false;
-            }
-
-            const QVariant rolePropertyValue = rowAsMap.value(roleData.name);
-
-            if (rolePropertyValue.userType() != roleData.type) {
-                if (!rolePropertyValue.canConvert(QMetaType(roleData.type))) {
-                    qmlWarning(this).noquote() << functionName << ": expected the property named \""
-                                               << roleData.name << "\" to be of type \"" << roleData.typeName
-                                               << "\", but got \"" << QString::fromLatin1(rolePropertyValue.typeName())
-                                               << "\" instead";
-                    return false;
-                }
-
-                QVariant effectiveValue = rolePropertyValue;
-                if (!effectiveValue.convert(QMetaType(roleData.type))) {
-                    qmlWarning(this).noquote() << functionName << ": failed converting value \""
-                                               << rolePropertyValue << "\" set at column " << columnIndex << " with role \""
-                                               << QString::fromLatin1(rolePropertyValue.typeName()) << "\" to \""
-                                               << roleData.typeName << "\"";
-                    return false;
-                }
-            }
-        }
-    }
-
     if (rowAsMap.contains(ROWS_PROPERTY_NAME) && rowAsMap[ROWS_PROPERTY_NAME].userType() == QMetaType::Type::QVariantList)
     {
         const QList<QVariant> variantList = rowAsMap[ROWS_PROPERTY_NAME].toList();
@@ -622,7 +537,7 @@ bool QQmlTreeModel::validateNewRow(QLatin1StringView functionName, const QVarian
                 return false;
     }
 
-    return true;
+    return QQmlAbstractColumnModel::validateNewRow(functionName, row, operation);
 }
 
 int QQmlTreeModel::treeSize() const

src/labs/models/qqmltreemodel_p.h

@@ -72,19 +72,12 @@ private:
     int treeSize() const;
     friend class ::tst_QQmlTreeModel;
 
-    enum NewRowOperationFlag {
-        OtherOperation, // insert(), set(), etc.
-        SetRowsOperation,
-        AppendOperation
-    };
-
     void setRowsPrivate(const QVariantList &rowsAsVariantList);
     QVariant dataPrivate(const QModelIndex &index, const QString &roleName) const override;
     void setDataPrivate(const QModelIndex &index, const QString &roleName, QVariant value) override;
 
-    bool validateRowType(QLatin1StringView functionName, const QVariant &row) const;
     bool validateNewRow(QLatin1StringView functionName, const QVariant &row,
-        NewRowOperationFlag = OtherOperation) const;
+        NewRowOperationFlag = OtherOperation) const override;
 
     std::vector<std::unique_ptr<QQmlTreeRow>> mRows;
 

tests/auto/qml/qqmltablemodel/tst_qqmltablemodel.cpp

@@ -162,7 +162,7 @@ void tst_QQmlTableModel::appendRemoveRow()
 
     // Call append() with a row that is an array instead of a simple object.
     QTest::ignoreMessage(QtWarningMsg, QRegularExpression(
-        ".*appendRow\\(\\): row manipulation functions do not support complex rows \\(row index: -1\\)"));
+        ".*appendRow\\(\\): row manipulation functions do not support complex rows"));
     QVERIFY(QMetaObject::invokeMethod(view.rootObject(), "appendRowInvalid3"));
     // Nothing should change.
     QCOMPARE(model->rowCount(), 2);
@@ -415,7 +415,7 @@ void tst_QQmlTableModel::insertRow()
 
     // Try to insert a row that is an array instead of a simple object.
     QTest::ignoreMessage(QtWarningMsg, QRegularExpression(
-        ".*insertRow\\(\\): row manipulation functions do not support complex rows \\(row index: 0\\)"));
+        ".*insertRow\\(\\): row manipulation functions do not support complex rows"));
     QVERIFY(QMetaObject::invokeMethod(view.rootObject(), "insertRowInvalid3"));
     QCOMPARE(model->rowCount(), 2);
     QCOMPARE(model->columnCount(), 2);
@@ -759,7 +759,7 @@ void tst_QQmlTableModel::setRow()
 
     // Try to insert a row that is an array instead of a simple object.
     QTest::ignoreMessage(QtWarningMsg, QRegularExpression(
-        ".*setRow\\(\\): row manipulation functions do not support complex rows \\(row index: 0\\)"));
+        ".*setRow\\(\\): row manipulation functions do not support complex rows"));
     QVERIFY(QMetaObject::invokeMethod(view.rootObject(), "setRowInvalid3"));
     QCOMPARE(model->rowCount(), 2);
     QCOMPARE(model->columnCount(), 2);
@@ -993,7 +993,7 @@ void tst_QQmlTableModel::setRowsMultipleTimes()
 
     // Set invalid rows; we should get a warning and nothing should change.
     QTest::ignoreMessage(QtWarningMsg, QRegularExpression(
-        ".*setRows\\(\\): expected a property named \"name\" in row at index 0, but couldn't find one"));
+        ".*setRows\\(\\): expected a property named \"name\" in row"));
     QVERIFY(QMetaObject::invokeMethod(view.rootObject(), "setRowsInvalid"));
     QCOMPARE(model->rowCount(), 3);
     QCOMPARE(model->columnCount(), 2);