QSqlTableModel: use selectRow() for field and row edit strategies

Calling select refreshes the query data but disrupts view
navigation.

For OnFieldChange and OnRecordChange it makes sense to only
select the row in question. This does not disturb view navigation.

Assume disruption of view navigation is not a problem
for OnManualSubmit because the user or application decides
when submitAll is called.

Task-number: QTBUG-2875
Change-Id: I1e5f68668fb9102f6296d67d543e80daa403f1c4
Reviewed-by: Yunqiao Yin <charles.yin@nokia.com>

dist/changes-5.0.0

@@ -406,6 +406,14 @@ ignore the rest of the range.
 QSqlTableModel::indexInQuery() as example of how to implement in a
 subclass.
 
+* QSqlTableModel edit strategies OnFieldChange/OnRowChange QTBUG-2875
+Previously, after changes were submitted in these edit strategies, select()
+was called which removed and inserted all rows. This ruined navigation
+in QTableView. Now, with these edit strategies, there is no implicit select()
+done after committing. This includes deleted rows which remain in
+the model as blank rows until the application calls select(). Instead,
+selectRow() is called to refresh only the affected row.
+
 ****************************************************************************
 *                          Database Drivers                                *
 ****************************************************************************

src/sql/models/qsqltablemodel.cpp

@@ -663,8 +663,8 @@ bool QSqlTableModel::deleteRowFromTable(int row)
     Returns false on error, detailed error information can be
     obtained with lastError().
 
-    On success the model will be repopulated. Any views 
+    In OnManualSubmit, on success the model will be repopulated.
-    presenting it will lose their selections.
+    Any views presenting it will lose their selections.
 
     Note: In OnManualSubmit mode, already submitted changes won't
     be cleared from the cache when submitAll() fails. This allows
@@ -677,6 +677,8 @@ bool QSqlTableModel::submitAll()
 {
     Q_D(QSqlTableModel);
 
+    bool success = true;
+
     for (QSqlTableModelPrivate::CacheMap::Iterator it = d->cache.begin();
          it != d->cache.constEnd(); ++it) {
         if (it.value().submitted())
@@ -684,25 +686,35 @@ bool QSqlTableModel::submitAll()
 
         switch (it.value().op()) {
         case QSqlTableModelPrivate::Insert:
-            if (!insertRowIntoTable(it.value().rec()))
+            success = insertRowIntoTable(it.value().rec());
-                return false;
             break;
         case QSqlTableModelPrivate::Update:
-            if (!updateRowInTable(it.key(), it.value().rec()))
+            success = updateRowInTable(it.key(), it.value().rec());
-                return false;
             break;
         case QSqlTableModelPrivate::Delete:
-            if (!deleteRowFromTable(it.key()))
+            success = deleteRowFromTable(it.key());
-                return false;
             break;
         case QSqlTableModelPrivate::None:
             Q_ASSERT_X(false, "QSqlTableModel::submitAll()", "Invalid cache operation");
             break;
         }
-        it.value().setSubmitted();
+
+        if (success) {
+            it.value().setSubmitted();
+            if (d->strategy != OnManualSubmit)
+                success = selectRow(it.key());
+        }
+
+        if (!success)
+            break;
     }
 
-    return select();
+    if (success) {
+        if (d->strategy == OnManualSubmit)
+            success = select();
+    }
+
+    return success;
 }
 
 /*!
@@ -719,8 +731,8 @@ bool QSqlTableModel::submitAll()
     Returns true on success; otherwise returns false. Use lastError()
     to query detailed error information.
 
-    On success the model will be repopulated. Any views 
+    Does not automatically repopulate the model. Submitted rows are
-    presenting it will lose their selections.
+    refreshed from the database on success.
 
     \sa revert(), revertRow(), submitAll(), revertAll(), lastError()
 */

tests/auto/sql/models/qsqlrelationaltablemodel/tst_qsqlrelationaltablemodel.cpp

@@ -542,6 +542,13 @@ void tst_QSqlRelationalTableModel::setRecord()
     model.setSort(0, Qt::AscendingOrder);
     QVERIFY_SQL(model, submit());
 
+    if (model.editStrategy() != QSqlTableModel::OnManualSubmit) {
+        QCOMPARE(model.data(model.index(1, 0)).toInt(), 7);
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("tester"));
+        QCOMPARE(model.data(model.index(1, 2)).toString(), QString("herr"));
+        QVERIFY_SQL(model, select());
+    }
+
     QCOMPARE(model.data(model.index(3, 0)).toInt(), 7);
     QCOMPARE(model.data(model.index(3, 1)).toString(), QString("tester"));
     QCOMPARE(model.data(model.index(3, 2)).toString(), QString("herr"));
@@ -599,6 +606,8 @@ void tst_QSqlRelationalTableModel::insertWithStrategies()
     QVERIFY_SQL(model, submitAll());
 
     model.setEditStrategy(QSqlTableModel::OnManualSubmit);
+    // The changes were submitted, but there was no automatic select to resort
+    QVERIFY_SQL(model, select());
 
     QCOMPARE(model.data(model.index(0,0)).toInt(), 1);
     QCOMPARE(model.data(model.index(0,1)).toString(), QString("harry"));
@@ -1401,6 +1410,8 @@ void tst_QSqlRelationalTableModel::whiteSpaceInIdentifiers()
 
     QVERIFY_SQL(model, insertRecord(-1, rec));
     model.submitAll();
+    if (model.editStrategy() != QSqlTableModel::OnManualSubmit)
+        QVERIFY_SQL(model, select());
 
     QCOMPARE(model.data(model.index(0, 0)).toInt(), 3);
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("Washington"));

tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp

@@ -411,8 +411,11 @@ void tst_QSqlTableModel::setRecord()
             } else if ((QSqlTableModel::EditStrategy)submitpolicy == QSqlTableModel::OnRowChange && i == model.rowCount() -1)
                 model.submit();
             else {
-                // dataChanged() is not emitted when submitAll() is called
+                // dataChanged() emitted by selectRow() as well as setRecord()
-                QCOMPARE(spy.count(), 1);
+                if ((QSqlTableModel::EditStrategy)submitpolicy == QSqlTableModel::OnFieldChange)
+                    QCOMPARE(spy.count(), 2);
+                else
+                    QCOMPARE(spy.count(), 1);
                 QCOMPARE(spy.at(0).count(), 2);
                 QCOMPARE(qvariant_cast<QModelIndex>(spy.at(0).at(0)), model.index(i, 0));
                 QCOMPARE(qvariant_cast<QModelIndex>(spy.at(0).at(1)), model.index(i, rec.count() - 1));
@@ -471,8 +474,7 @@ void tst_QSqlTableModel::insertRow()
     rec.setValue(0, 42);
     rec.setValue(1, QString("francis"));
 
-    // FieldChange updates immediately and resorts
+    // Setting record does not cause resort
-    // Row/Manual submit does not resort
     QVERIFY(model.setRecord(2, rec));
 
     QCOMPARE(model.data(model.index(0, 0)).toInt(), 1);
@@ -482,8 +484,23 @@ void tst_QSqlTableModel::insertRow()
     QCOMPARE(model.data(model.index(1, 1)).toString(), QString("trond"));
     QCOMPARE(model.data(model.index(1, 2)).toInt(), 2);
 
-    // See comment above setRecord
+    QCOMPARE(model.data(model.index(2, 0)).toInt(), 42);
-    if (submitpolicy == QSqlTableModel::OnFieldChange) {
+    QCOMPARE(model.data(model.index(2, 1)).toString(), QString("francis"));
+    QCOMPARE(model.data(model.index(2, 2)).toInt(), 2);
+    QCOMPARE(model.data(model.index(3, 0)).toInt(), 3);
+    QCOMPARE(model.data(model.index(3, 1)).toString(), QString("vohi"));
+    QCOMPARE(model.data(model.index(3, 2)).toInt(), 3);
+
+    QVERIFY(model.submitAll());
+
+    if (submitpolicy == QSqlTableModel::OnManualSubmit) {
+        // After the submit we should have the resorted view
+        QCOMPARE(model.data(model.index(0, 0)).toInt(), 1);
+        QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
+        QCOMPARE(model.data(model.index(0, 2)).toInt(), 1);
+        QCOMPARE(model.data(model.index(1, 0)).toInt(), 2);
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("trond"));
+        QCOMPARE(model.data(model.index(1, 2)).toInt(), 2);
         QCOMPARE(model.data(model.index(2, 0)).toInt(), 3);
         QCOMPARE(model.data(model.index(2, 1)).toString(), QString("vohi"));
         QCOMPARE(model.data(model.index(2, 2)).toInt(), 3);
@@ -491,6 +508,13 @@ void tst_QSqlTableModel::insertRow()
         QCOMPARE(model.data(model.index(3, 1)).toString(), QString("francis"));
         QCOMPARE(model.data(model.index(3, 2)).toInt(), 2);
     } else {
+        // Submit does not select, therefore not resorted
+        QCOMPARE(model.data(model.index(0, 0)).toInt(), 1);
+        QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
+        QCOMPARE(model.data(model.index(0, 2)).toInt(), 1);
+        QCOMPARE(model.data(model.index(1, 0)).toInt(), 2);
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("trond"));
+        QCOMPARE(model.data(model.index(1, 2)).toInt(), 2);
         QCOMPARE(model.data(model.index(2, 0)).toInt(), 42);
         QCOMPARE(model.data(model.index(2, 1)).toString(), QString("francis"));
         QCOMPARE(model.data(model.index(2, 2)).toInt(), 2);
@@ -499,9 +523,8 @@ void tst_QSqlTableModel::insertRow()
         QCOMPARE(model.data(model.index(3, 2)).toInt(), 3);
     }
 
-    QVERIFY(model.submitAll());
+    QVERIFY(model.select());
-
+    // After the select we should have the resorted view in all strategies
-    // After the submit we should have the resorted view
     QCOMPARE(model.data(model.index(0, 0)).toInt(), 1);
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
     QCOMPARE(model.data(model.index(0, 2)).toInt(), 1);
@@ -514,7 +537,6 @@ void tst_QSqlTableModel::insertRow()
     QCOMPARE(model.data(model.index(3, 0)).toInt(), 42);
     QCOMPARE(model.data(model.index(3, 1)).toString(), QString("francis"));
     QCOMPARE(model.data(model.index(3, 2)).toInt(), 2);
-
 }
 
 void tst_QSqlTableModel::insertRecord()
@@ -647,8 +669,7 @@ void tst_QSqlTableModel::removeRow()
     QVERIFY_SQL(model, select());
     QCOMPARE(model.rowCount(), 3);
 
-    // headerDataChanged must be emitted by the model when the edit strategy is OnManualSubmit,
+    // headerDataChanged must be emitted by the model since the row won't vanish until select
-    // when OnFieldChange or OnRowChange it's not needed because the model will re-select.
     qRegisterMetaType<Qt::Orientation>("Qt::Orientation");
     QSignalSpy headerDataChangedSpy(&model, SIGNAL(headerDataChanged(Qt::Orientation, int, int)));
 
@@ -673,7 +694,10 @@ void tst_QSqlTableModel::removeRow()
 
     headerDataChangedSpy.clear();
     QVERIFY(model.removeRow(1));
-    QCOMPARE(headerDataChangedSpy.count(), 0);
+    QCOMPARE(headerDataChangedSpy.count(), 1);
+    QCOMPARE(model.rowCount(), 3);
+
+    QVERIFY_SQL(model, select());
     QCOMPARE(model.rowCount(), 2);
 
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
@@ -706,6 +730,11 @@ void tst_QSqlTableModel::removeRows()
     QCOMPARE(beforeDeleteSpy.count(), 2);
     QVERIFY(beforeDeleteSpy.at(0).at(0).toInt() == 0);
     QVERIFY(beforeDeleteSpy.at(1).at(0).toInt() == 1);
+    // deleted rows shown as empty until select
+    QCOMPARE(model.rowCount(), 3);
+    QCOMPARE(model.data(model.index(0, 1)).toString(), QString(""));
+    QVERIFY(model.select());
+    // deleted rows are gone
     QCOMPARE(model.rowCount(), 1);
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("vohi"));
     model.clear();
@@ -778,6 +807,14 @@ void tst_QSqlTableModel::removeInsertedRow()
 
     model.submitAll();
 
+    if (model.editStrategy() != QSqlTableModel::OnManualSubmit) {
+        QCOMPARE(model.rowCount(), 4);
+        QCOMPARE(model.data(model.index(1, 0)).toInt(), 55);
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("null columns"));
+        QCOMPARE(model.data(model.index(1, 2)).isNull(), true);
+        QVERIFY(model.select());
+    }
+
     QCOMPARE(model.rowCount(), 4);
     QCOMPARE(model.data(model.index(3, 0)).toInt(), 55);
     QCOMPARE(model.data(model.index(3, 1)).toString(), QString("null columns"));
@@ -785,8 +822,17 @@ void tst_QSqlTableModel::removeInsertedRow()
 
     QVERIFY(model.removeRow(3));
     model.submitAll();
-    QCOMPARE(model.rowCount(), 3);
 
+    if (model.editStrategy() != QSqlTableModel::OnManualSubmit) {
+        QCOMPARE(model.rowCount(), 4);
+        QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("trond"));
+        QCOMPARE(model.data(model.index(2, 1)).toString(), QString("vohi"));
+        QCOMPARE(model.data(model.index(3, 1)).toString(), QString(""));
+        QVERIFY(model.select());
+    }
+
+    QCOMPARE(model.rowCount(), 3);
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
     QCOMPARE(model.data(model.index(1, 1)).toString(), QString("trond"));
     QCOMPARE(model.data(model.index(2, 1)).toString(), QString("vohi"));
@@ -1129,6 +1175,11 @@ void tst_QSqlTableModel::insertRecordBeforeSelect()
     buffer.setValue("title", 0);
     QVERIFY_SQL(model, insertRecord(1, buffer));
 
+    if (model.editStrategy() != QSqlTableModel::OnManualSubmit) {
+        QCOMPARE(model.rowCount(), 2);
+        QVERIFY_SQL(model, select());
+    }
+
     int rowCount = model.rowCount();
     model.clear();
     QCOMPARE(model.rowCount(), 0);