QSqlTableModel::setData()/setRecord(): fix incorrect row

For OnFieldChange and OnRowChange, before submitting new changes,
setData() and setRecord() attempt to submit pending changes and
revert them upon failure. However, they fail to consider that
reverting pending insertions removes rows from the model. As a
result, the new change can be applied to a row higher than intended.

One possible solution would be to adjust the targetted index for the
removed rows, so that the intended row is affected by the new change.
But this still causes the strange editing experience as rows jump
up just as they are being edited.

It does not seem right in the first place for the model to initiate
reverting changes. It should be up to the application to decide what
to do when data cannot be committed. In particular, setData() and
setRecord() should not have the side effect of reverting already
pending changes.

The chosen solution is simply to refuse new changes that don't make
sense for the edit strategy. For OnFieldChange, flag() will
indicate read-only when editing is blocked by a pending change.

Since setData() and setRecord() submit data immediately for
OnFieldChange, it no longer makes sense to resubmit changes
automatically before a new change.

For OnRowChange, setData() keeps the behavior of automatically
submitting a pending row before starting on a new row. This is
historical behavior and is probably motivated by the fact that
QTableView does not automatically call submit() when editing leaves a
row. The obvious shortcoming of this is that the last row to be edited
will not be submitted automatically. It also prevents us from flagging
rows other than the pending row as read-only.

For OnRowChange, setRecord(), being row-oriented by nature, should
submit the change immediately rather than waiting for the next call
to setRecord(). This makes setRecord() consistent with insertRecord().

Change-Id: Icb4019d8b7c53a7ee48f8121a7a525e8bc35d523
Reviewed-by: Honglei Zhang <honglei.zhang@nokia.com>

1 files changed, 68 insertions, 4 deletions

diff --git a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp b/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
index 7dcc109cdc..bb1131655d 100644
--- a/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
+++ b/tests/auto/sql/models/qsqltablemodel/tst_qsqltablemodel.cpp
@@ -83,6 +83,8 @@ private slots:
     void setRecord();
     void insertRow_data() { generic_data_with_strategies(); }
     void insertRow();
+    void insertRowFailure_data() { generic_data_with_strategies(); }
+    void insertRowFailure();
     void insertRecord_data() { generic_data(); }
     void insertRecord();
     void insertMultiRecords_data() { generic_data(); }
@@ -161,7 +163,8 @@ void tst_QSqlTableModel::dropTestTables()
                    << qTableName("test4", __FILE__)
                    << qTableName("emptytable", __FILE__)
                    << qTableName("bigtable", __FILE__)
-                   << qTableName("foo", __FILE__);
+                   << qTableName("foo", __FILE__)
+                   << qTableName("pktest", __FILE__);
         if (testWhiteSpaceNames(db.driverName()))
             tableNames << qTableName("qtestw hitespace", db.driver());
 
@@ -197,6 +200,8 @@ void tst_QSqlTableModel::createTestTables()
             QString qry = "create table " + qTableName("qtestw hitespace", db.driver()) + " ("+ db.driver()->escapeIdentifier("a field", QSqlDriver::FieldName) + " int)";
             QVERIFY_SQL( q, exec(qry));
         }
+
+        QVERIFY_SQL( q, exec("create table "+qTableName("pktest", __FILE__)+"(id int not null primary key, a varchar(20))"));
     }
 }
 
@@ -414,7 +419,7 @@ void tst_QSqlTableModel::setRecord()
                 model.submit();
             else {
                 // dataChanged() emitted by selectRow() as well as setRecord()
-                if ((QSqlTableModel::EditStrategy)submitpolicy == QSqlTableModel::OnFieldChange)
+                if ((QSqlTableModel::EditStrategy)submitpolicy != QSqlTableModel::OnManualSubmit)
                     QCOMPARE(spy.count(), 2);
                 else
                     QCOMPARE(spy.count(), 1);
@@ -541,6 +546,65 @@ void tst_QSqlTableModel::insertRow()
     QCOMPARE(model.data(model.index(3, 2)).toInt(), 2);
 }
 
+void tst_QSqlTableModel::insertRowFailure()
+{
+    QFETCH(QString, dbName);
+    QSqlDatabase db = QSqlDatabase::database(dbName);
+    QFETCH(int, submitpolicy_i);
+    QSqlTableModel::EditStrategy submitpolicy = (QSqlTableModel::EditStrategy) submitpolicy_i;
+    CHECK_DATABASE(db);
+
+    QSqlTableModel model(0, db);
+    model.setTable(qTableName("pktest", __FILE__));
+    model.setEditStrategy(submitpolicy);
+
+    QSqlRecord values = model.record();
+    values.setValue(0, 42);
+    values.setGenerated(0, true);
+    values.setValue(1, QString("blah"));
+    values.setGenerated(1, true);
+
+    // populate 1 row
+    QVERIFY_SQL(model, insertRecord(0, values));
+    QVERIFY_SQL(model, submitAll());
+    QVERIFY_SQL(model, select());
+    QCOMPARE(model.rowCount(), 1);
+    QCOMPARE(model.data(model.index(0, 0)).toInt(), 42);
+    QCOMPARE(model.data(model.index(0, 1)).toString(), QString("blah"));
+
+    // primary key conflict will succeed in model but fail in database
+    QVERIFY_SQL(model, insertRow(0));
+    QVERIFY_SQL(model, setData(model.index(0, 0), 42));
+    QVERIFY_SQL(model, setData(model.index(0, 1), "conflict"));
+    QFAIL_SQL(model, submitAll());
+
+    // failed insert is still cached
+    QCOMPARE(model.rowCount(), 2);
+    QCOMPARE(model.data(model.index(0, 1)).toString(), QString("conflict"));
+    QCOMPARE(model.data(model.index(1, 1)).toString(), QString("blah"));
+
+    // cached insert affects subsequent operations
+    values.setValue(1, QString("spam"));
+    if (submitpolicy != QSqlTableModel::OnManualSubmit) {
+        QFAIL_SQL(model, setData(model.index(1, 1), QString("eggs")));
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("blah"));
+        QFAIL_SQL(model, setRecord(1, values));
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("blah"));
+    } else {
+        QVERIFY_SQL(model, setData(model.index(1, 1), QString("eggs")));
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("eggs"));
+        QVERIFY_SQL(model, setRecord(1, values));
+        QCOMPARE(model.data(model.index(1, 1)).toString(), QString("spam"));
+    }
+
+    // restore empty table
+    model.revertAll();
+    QVERIFY_SQL(model, removeRow(0));
+    QVERIFY_SQL(model, submitAll());
+    QVERIFY_SQL(model, select());
+    QCOMPARE(model.rowCount(), 0);
+}
+
 void tst_QSqlTableModel::insertRecord()
 {
     QFETCH(QString, dbName);
@@ -1026,7 +1090,7 @@ void tst_QSqlTableModel::isDirty()
     QSqlRecord newvals = model.record(0);
     newvals.setValue(1, QString("sam i am"));
     newvals.setGenerated(1, true);
-    if (submitpolicy != QSqlTableModel::OnFieldChange) {
+    if (submitpolicy == QSqlTableModel::OnManualSubmit) {
         // setRecord() followed by revertAll()
         QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
         QVERIFY_SQL(model, setRecord(0, newvals));
@@ -1054,7 +1118,7 @@ void tst_QSqlTableModel::isDirty()
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("harry"));
     QVERIFY_SQL(model, setRecord(0, newvals));
     QCOMPARE(model.data(model.index(0, 1)).toString(), QString("sam i am"));
-    if (submitpolicy != QSqlTableModel::OnFieldChange) {
+    if (submitpolicy == QSqlTableModel::OnManualSubmit) {
         QVERIFY_SQL(model, isDirty());
         QVERIFY_SQL(model, isDirty(model.index(0, 1)));
     }