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>

4 files changed, 108 insertions, 26 deletions

diff --git a/dist/changes-5.0.0 b/dist/changes-5.0.0
index dd0a0693ed..33a8869397 100644
--- a/dist/changes-5.0.0
+++ b/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                                *
 ****************************************************************************
diff --git a/src/sql/models/qsqltablemodel.cpp b/src/sql/models/qsqltablemodel.cpp
index 40230c3043..aa7dc3dcf8 100644
--- a/src/sql/models/qsqltablemodel.cpp
+++ b/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 
-    presenting it will lose their selections.
+    In OnManualSubmit, on success the model will be repopulated.
+    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()))
-                return false;
+            success = insertRowIntoTable(it.value().rec());
             break;
         case QSqlTableModelPrivate::Update:
-            if (!updateRowInTable(it.key(), it.value().rec()))
-                return false;
+            success = updateRowInTable(it.key(), it.value().rec());
             break;
         case QSqlTableModelPrivate::Delete:
-            if (!deleteRowFromTable(it.key()))
-                return false;
+            success = deleteRowFromTable(it.key());
             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;
+    }
+
+    if (success) {
+        if (d->strategy == OnManualSubmit)
+            success = select();
     }
 
-    return 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 
-    presenting it will lose their selections.
+    Does not automatically repopulate the model. Submitted rows are
+    refreshed from the database on success.
 
     \sa revert(), revertRow(), submitAll(), revertAll(), lastError()
 */
diff --git a/tests/auto/sql/models/qsqlrelationaltablemodel/tst_qsqlrelationaltablemodel.cpp b/tests/auto/sql/models/qsqlrelationaltablemodel/tst_qsqlrelationaltablemodel.cpp
index ddafeea427..ce0d8db1fd 100644
--- a/tests/auto/sql/models/qsqlrelationaltablemodel/tst_qsqlrelationaltablemodel.cpp
+++ b/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"));
diff --git a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp b/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
index 43829987d4..448111cd9f 100644
--- a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
+++ b/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
-                QCOMPARE(spy.count(), 1);
+                // dataChanged() emitted by selectRow() as well as setRecord()
+                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
-    // Row/Manual submit does not resort
+    // Setting record does not cause 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
-    if (submitpolicy == QSqlTableModel::OnFieldChange) {
+    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);
+    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());
-
-    // After the submit we should have the resorted view
+    QVERIFY(model.select());
+    // After the select we should have the resorted view in all strategies
     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,
-    // when OnFieldChange or OnRowChange it's not needed because the model will re-select.
+    // headerDataChanged must be emitted by the model since the row won't vanish until 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);