QSqlTableModel: handle changes between submit and select

Once an insert has been submitted, the cached record behaves like an
update. For row bookkeeping, we still have to remember that it was
originally inserted and is not in the query rows.

Between submitting a delete and selecting, we remove the values
from the deleted record. This causes a blank row to be displayed.
Read-only flag is set for cells in deleted row.

Reverting between submit and select means going back to the last
submitted values.

When removing rows, it's better to process from highest row numbers
to lowest. This avoids complications with higher rows shifting down
when lower rows are removed.

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

src/sql/models/qsqltablemodel.cpp

@@ -87,7 +87,7 @@ int QSqlTableModelPrivate::insertCount(int maxRow) const
     for (;
          i != e && (maxRow < 0 || i.key() <= maxRow);
          ++i) {
-        if (i.value().op() == Insert)
+        if (i.value().insert())
             ++cnt;
     }
 
@@ -122,19 +122,17 @@ void QSqlTableModelPrivate::revertCachedRow(int row)
     Q_Q(QSqlTableModel);
     ModifiedRow r = cache.value(row);
 
-    // cannot revert a committed change
-    if (r.submitted())
-        return;
-
     switch (r.op()) {
     case QSqlTableModelPrivate::None:
         Q_ASSERT_X(false, "QSqlTableModelPrivate::revertCachedRow()", "Invalid entry in cache map");
         return;
     case QSqlTableModelPrivate::Update:
     case QSqlTableModelPrivate::Delete:
-        cache.remove(row);
+        if (!r.submitted()) {
-        emit q->dataChanged(q->createIndex(row, 0),
+            cache[row].revert();
-                            q->createIndex(row, q->columnCount() - 1));
+            emit q->dataChanged(q->createIndex(row, 0),
+                                q->createIndex(row, q->columnCount() - 1));
+        }
         break;
     case QSqlTableModelPrivate::Insert: {
             QMap<int, QSqlTableModelPrivate::ModifiedRow>::Iterator it = cache.find(row);
@@ -373,7 +371,7 @@ bool QSqlTableModel::select()
     while (it != d->cache.constBegin()) {
         --it;
         // rows must be accounted for
-        if (it.value().op() == QSqlTableModelPrivate::Insert) {
+        if (it.value().insert()) {
             beginRemoveRows(QModelIndex(), it.key(), it.key());
             it = d->cache.erase(it);
             endRemoveRows();
@@ -470,11 +468,14 @@ bool QSqlTableModel::setData(const QModelIndex &index, const QVariant &value, in
     if (!index.isValid() || index.column() >= d->rec.count() || index.row() >= rowCount())
         return false;
 
+    if (d->cache.value(index.row()).op() == QSqlTableModelPrivate::Delete)
+        return false;
+
     if (d->strategy == OnFieldChange && d->cache.value(index.row()).op() != QSqlTableModelPrivate::Insert) {
-        d->cache.clear();
+        revertAll();
     } else if (d->strategy == OnRowChange && !d->cache.isEmpty() && !d->cache.contains(index.row())) {
         submit();
-        d->cache.clear();
+        revertAll();
     }
 
     QSqlTableModelPrivate::ModifiedRow &row = d->cache[index.row()];
@@ -759,8 +760,10 @@ void QSqlTableModel::revertAll()
 {
     Q_D(QSqlTableModel);
 
-    while (!d->cache.isEmpty())
+    const QList<int> rows(d->cache.keys());
-        revertRow(d->cache.constBegin().key());
+    for (int i = rows.size() - 1; i >= 0; --i) {
+        revertRow(rows.value(i));
+    }
 }
 
 /*!
@@ -967,15 +970,17 @@ bool QSqlTableModel::removeRows(int row, int count, const QModelIndex &parent)
     else if (!count)
         return true;
 
-    for (int i = 0; i < count; ++i) {
+    // Iterate backwards so we don't have to worry about removed rows causing
-        int idx = row + i;
+    // higher cache entries to shift downwards.
-        if (d->cache.value(idx).op() == QSqlTableModelPrivate::Insert) {
+    for (int idx = row + count - 1; idx >= row; --idx) {
+        QSqlTableModelPrivate::ModifiedRow& mrow = d->cache[idx];
+        if (mrow.op() == QSqlTableModelPrivate::Insert) {
             revertRow(idx);
-            // Reverting a row means all the other cache entries have been adjusted downwards
-            // so fake this by adjusting row
-            --row;
         } else {
-            d->cache[idx] = QSqlTableModelPrivate::ModifiedRow(QSqlTableModelPrivate::Delete, record(idx));
+            if (mrow.op() == QSqlTableModelPrivate::None)
+                mrow = QSqlTableModelPrivate::ModifiedRow(QSqlTableModelPrivate::Delete, record(idx));
+            else
+                mrow.setOp(QSqlTableModelPrivate::Delete);
             if (d->strategy == OnManualSubmit)
                 emit headerDataChanged(Qt::Vertical, idx, idx);
         }
@@ -1158,6 +1163,8 @@ Qt::ItemFlags QSqlTableModel::flags(const QModelIndex &index) const
         return 0;
     if (d->rec.field(index.column()).isReadOnly())
         return Qt::ItemIsSelectable | Qt::ItemIsEnabled;
+    if (d->cache.value(index.row()).op() == QSqlTableModelPrivate::Delete)
+        return Qt::ItemIsSelectable | Qt::ItemIsEnabled;
     return Qt::ItemIsSelectable | Qt::ItemIsEnabled | Qt::ItemIsEditable;
 }
 
@@ -1186,8 +1193,11 @@ bool QSqlTableModel::setRecord(int row, const QSqlRecord &values)
     if (row >= rowCount())
         return false;
 
+    if (d->cache.value(row).op() == QSqlTableModelPrivate::Delete)
+        return false;
+
     if (d->strategy == OnFieldChange && d->cache.value(row).op() != QSqlTableModelPrivate::Insert)
-        d->cache.clear();
+        revertAll();
     else if (d->strategy == OnRowChange && !d->cache.isEmpty() && !d->cache.contains(row))
         submit();
 

src/sql/models/qsqltablemodel_p.h

@@ -101,18 +101,51 @@ public:
     {
     public:
         inline ModifiedRow(Op o = None, const QSqlRecord &r = QSqlRecord())
-            : m_op(o), m_rec(r), m_submitted(false)
+            : m_op(None), m_db_values(r), m_insert(o == Insert)
-        { init_rec(); }
+        { setOp(o); }
         inline Op op() const { return m_op; }
+        inline void setOp(Op o)
+        {
+            if (o == m_op)
+                return;
+            m_submitted = (o != Insert && o != Delete);
+            m_op = o;
+            m_rec = m_db_values;
+            setGenerated(m_rec, m_op == Delete);
+        }
         inline QSqlRecord rec() const { return m_rec; }
         inline QSqlRecord& recRef() { return m_rec; }
         inline void setValue(int c, const QVariant &v)
         {
+            m_submitted = false;
             m_rec.setValue(c, v);
             m_rec.setGenerated(c, true);
         }
         inline bool submitted() const { return m_submitted; }
-        inline void setSubmitted() { m_submitted = true; }
+        inline void setSubmitted()
+        {
+            m_submitted = true;
+            setGenerated(m_rec, false);
+            if (m_op == Delete) {
+                m_rec.clearValues();
+            }
+            else {
+                m_op = Update;
+                m_db_values = m_rec;
+                setGenerated(m_db_values, true);
+            }
+        }
+        inline bool insert() const { return m_insert; }
+        inline void revert()
+        {
+            if (m_submitted)
+                return;
+            if (m_op == Delete)
+                m_op = Update;
+            m_rec = m_db_values;
+            setGenerated(m_rec, false);
+            m_submitted = true;
+        }
         inline QSqlRecord primaryValues(const QSqlRecord& pi) const
         {
             if (m_op == None || m_op == Insert)
@@ -126,16 +159,16 @@ public:
             return values;
         }
     private:
-        void init_rec()
+        inline static void setGenerated(QSqlRecord& r, bool g)
         {
-            for (int i = m_rec.count() - 1; i >= 0; --i)
+            for (int i = r.count() - 1; i >= 0; --i)
-                m_rec.setGenerated(i, false);
+                r.setGenerated(i, g);
-            m_db_values = m_rec;
         }
         Op m_op;
         QSqlRecord m_rec;
         QSqlRecord m_db_values;
         bool m_submitted;
+        bool m_insert;
     };
 
     typedef QMap<int, ModifiedRow> CacheMap;

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

@@ -730,10 +730,10 @@ void tst_QSqlTableModel::removeRows()
     QSignalSpy headerDataChangedSpy(&model, SIGNAL(headerDataChanged(Qt::Orientation, int, int)));
     QVERIFY(model.removeRows(0, 2, QModelIndex()));
     QCOMPARE(headerDataChangedSpy.count(), 2);
-    QCOMPARE(headerDataChangedSpy.at(0).at(1).toInt(), 0);
+    QCOMPARE(headerDataChangedSpy.at(0).at(1).toInt(), 1);
-    QCOMPARE(headerDataChangedSpy.at(0).at(2).toInt(), 0);
+    QCOMPARE(headerDataChangedSpy.at(0).at(2).toInt(), 1);
-    QCOMPARE(headerDataChangedSpy.at(1).at(1).toInt(), 1);
+    QCOMPARE(headerDataChangedSpy.at(1).at(1).toInt(), 0);
-    QCOMPARE(headerDataChangedSpy.at(1).at(2).toInt(), 1);
+    QCOMPARE(headerDataChangedSpy.at(1).at(2).toInt(), 0);
     QCOMPARE(model.rowCount(), 3);
     QVERIFY(beforeDeleteSpy.count() == 0);
     QVERIFY(model.submitAll());