Fix Expected, <void> not following c++20 standard, document empty catch(...)

core/Expected.h

@@ -63,32 +63,6 @@ public:
 		}
 	}
 
-//	void swap(Expected& rhs)
-//	{
-//		if (m_valid) {
-//			if (rhs.m_valid) {
-//				using std::swap;
-//				swap(m_value, rhs.m_value);
-//			}
-//			else {
-//				auto t = std::move(rhs.m_error);
-//				new(&rhs.m_value) T(std::move(m_value));
-//				new(&m_error) std::exception_ptr(t);
-//				std::swap(m_valid, rhs.m_valid);
-//			}
-//		}
-//		else {
-//			if (rhs.m_valid) {
-//				rhs.swap(*this);
-//			}
-//			else {
-//				using std::swap;
-//				swap(m_error, rhs.m_error);
-//				std::swap(m_valid, rhs.m_valid);
-//			}
-//		}
-//	}
-
 	template <class E>
 	static Expected<T> fromException(const E& exception)
 	{
@@ -144,6 +118,7 @@ public:
 			return true;
 		}
 		catch (...) {
+			// The stored exception is not of type E. It is still held in m_error.
 		}
 		return false;
 	}
@@ -168,7 +143,7 @@ class Expected<void> {
 
 public:
 
-	Expected<void>()
+	Expected()
 		: m_valid(true)
 	{}
 
@@ -186,26 +161,6 @@ public:
 	~Expected()
 	{}
 
-//	void swap(Expected& rhs)
-//	{
-//		if (m_valid) {
-//			if (!rhs.m_valid) {
-//				m_error = rhs.m_error;
-//				std::swap(m_valid, rhs.m_valid);
-//			}
-//		}
-//		else {
-//			if (rhs.m_valid) {
-//				rhs.swap(*this);
-//			}
-//			else {
-//				using std::swap;
-//				swap(m_error, rhs.m_error);
-//				std::swap(m_valid, rhs.m_valid);
-//			}
-//		}
-//	}
-
 	template <class E>
 	static Expected<void> fromException(const E& exception)
 	{
@@ -253,6 +208,7 @@ public:
 			return true;
 		}
 		catch (...) {
+			// The stored exception is not of type E. It is still held in m_error.
 		}
 		return false;
 	}

pglab/crud/CrudModel.cpp

@@ -6,7 +6,6 @@
 #include "catalog/PgConstraintContainer.h"
 #include "catalog/PgClassContainer.h"
 #include "SqlFormattingUtils.h"
-#include "Pgsql_oids.h"
 #include "CustomDataRole.h"
 #include <QtConcurrent>
 #include <QFuture>
@@ -98,45 +97,46 @@ Oid CrudModel::getType(int column) const
 
 CrudModel::Value CrudModel::getLatestData(int columnIndex, int rowIndex) const
 {
-    if (m_roData)
+    if (!m_roData)
+        return {};
+
+    const RowMapping &row_mapping = m_rowMapping[rowIndex];
+    if (row_mapping.pending)
     {
-        auto row_mapping = m_rowMapping[rowIndex];
-
-        std::optional<Value> val;
-        if (row_mapping.pending)
-            val = m_pendingRowList.getValue(columnIndex, row_mapping.rowKey);
-
-        if (!val.has_value())
-            val = getSavedData(row_mapping, columnIndex);
-
-        return val.value_or(Value());
+        // an edited cell can be NULL, that is different from not having been edited
+        if (auto edited = m_pendingRowList.getValue(columnIndex, row_mapping.rowKey))
+            return *edited;
     }
-    return {};
-}
-
-CrudModel::Value CrudModel::getSavedData(int columnIndex, int rowIndex) const
-{
-    if (m_roData)
-    {
-        auto row_mapping = m_rowMapping[rowIndex];
-        return getSavedData(row_mapping, columnIndex);
-    }
-    return {};
+    return getSavedData(row_mapping, columnIndex);
 }
 
 CrudModel::Value CrudModel::getSavedData(const RowMapping &row_mapping, int columnIndex) const
 {
-    std::optional<Value> val;
-    if (!val.has_value() && row_mapping.isModified())
-        val = row_mapping.modifiedValue(columnIndex);
+    // rows that have been saved (or are new) have their own copy of the data
+    if (row_mapping.isModified())
+        return row_mapping.modifiedValue(columnIndex);
 
-    if (!val.has_value() && row_mapping.rowKey < m_roData->rows())
-        if (!m_roData->null(columnIndex, row_mapping.rowKey))
-            val = std::string(m_roData->val(columnIndex, row_mapping.rowKey));
+    if (row_mapping.rowKey < m_roData->rows() && !m_roData->null(columnIndex, row_mapping.rowKey))
+        return std::string(m_roData->val(columnIndex, row_mapping.rowKey));
 
-    return val.value_or(Value());
+    return {};
 }
 
+CrudModel::RowMapping* CrudModel::findRowMapping(int rowKey)
+{
+    auto iter = std::find_if(m_rowMapping.begin(), m_rowMapping.end(),
+                             [rowKey](const RowMapping &rhs) { return rhs.rowKey == rowKey; });
+    return iter != m_rowMapping.end() ? &*iter : nullptr;
+}
+
+void CrudModel::updatePendingState(bool was_dirty)
+{
+    if (was_dirty != hasPendingChanges())
+        emit pendingChangesChanged(hasPendingChanges());
+}
+
+namespace {
+
 QVariant StringToVariant(const std::string &str, Oid oid)
 {
     Pgsql::Value v(str.c_str(), oid);
@@ -174,9 +174,10 @@ QVariant StringToVariant(const std::string &str, Oid oid)
         case Pgsql::float8_oid:
             return v.operator double();
     }
-
 }
 
+} // namespace
+
 QVariant CrudModel::data(const QModelIndex &index, int role) const
 {
     if (index.column() < PreColumnCount)
@@ -254,8 +255,7 @@ void CrudModel::loadIntoModel(std::shared_ptr<Pgsql::Result> data)
     if (m_wholeTable)
         appendNewRowInternal();
 	endResetModel();
-	if (was_dirty)
-		emit pendingChangesChanged(false);
+	updatePendingState(was_dirty);
 }
 
 void CrudModel::initRowMapping()
@@ -269,26 +269,11 @@ void CrudModel::initRowMapping()
 
 void CrudModel::connectionStateChanged()
 {
-    switch (m_dbConn.state()) {
-	case ASyncDBConnection::State::NotConnected:
-		break;
-	case ASyncDBConnection::State::Connecting:
-		break;
-	case ASyncDBConnection::State::Connected:
-        if (callLoadData)
-        {
-			callLoadData = false;
-			loadData();
-		}
-		break;
-	case ASyncDBConnection::State::QuerySend:
-
-		break;
-	case ASyncDBConnection::State::CancelSend:
-		break;
-	case ASyncDBConnection::State::Terminating:
-		break;
-	}
+    if (m_dbConn.state() == ASyncDBConnection::State::Connected && callLoadData)
+    {
+        callLoadData = false;
+        loadData();
+    }
 }
 
 Qt::ItemFlags CrudModel::flags(const QModelIndex &index) const
@@ -326,8 +311,7 @@ bool CrudModel::setData(const QModelIndex &index, const QVariant &value, int rol
 		}
 		bool was_dirty = hasPendingChanges();
 		m_pendingRowList.setValue(col, row_mapping.rowKey, val);
-		if (!was_dirty)
-			emit pendingChangesChanged(true);
+		updatePendingState(was_dirty);
 
 		emit dataChanged(index, index, QVector<int>() << role);
 		return true;
@@ -337,8 +321,7 @@ bool CrudModel::setData(const QModelIndex &index, const QVariant &value, int rol
 
 std::tuple<bool, std::vector<CrudModel::Value>> CrudModel::saveRow(const PendingRow &pending_row, const RowMapping &row_mapping)
 {
-    auto data = pending_row.data();
-    if (data.empty())
+    if (pending_row.data().empty())
         return { true, {} };
 
     if (row_mapping.isNew())
@@ -348,31 +331,16 @@ std::tuple<bool, std::vector<CrudModel::Value>> CrudModel::saveRow(const Pending
         std::tie(buffer, params) = createInsertQuery(pending_row);
 
         Pgsql::Connection db_update_conn;
-        auto dbconfig = m_database->config();
-        db_update_conn.connect(dbconfig.connectionString());
+        connectForUpdate(db_update_conn);
         try
         {
             auto result = db_update_conn.queryParam(buffer, params);
             if (result && result.rows() == 1)
-            {
-                std::vector<Value> values;
-                auto row = *result.begin();
-                for (auto v : row)
-                {
-                    if (v.null())
-                        values.push_back(Value());
-                    else
-                        values.push_back(std::string(v.c_str()));
-                }
-
-                return { true, values };
-            }
+                return { true, rowValues(*result.begin()) };
         }
         catch (const Pgsql::PgResultError &ex)
         {
-            QMessageBox msgBox;
-            msgBox.setText(ex.what());
-            msgBox.exec();
+            reportError(ex.what());
         }
         return { false, {} };
     }
@@ -389,8 +357,7 @@ std::tuple<bool, std::vector<CrudModel::Value>> CrudModel::saveUpdate(const Pend
     try
     {
         Pgsql::Connection db_update_conn;
-        auto dbconfig = m_database->config();
-        db_update_conn.connect(dbconfig.connectionString());
+        connectForUpdate(db_update_conn);
         auto tx = Pgsql::Transaction::startTransaction(db_update_conn);
         for (Oid table_oid : tables)
         {
@@ -418,32 +385,47 @@ std::tuple<bool, std::vector<CrudModel::Value>> CrudModel::saveUpdate(const Pend
     }
     catch (const Pgsql::PgResultError &ex)
     {
-        QMessageBox msgBox;
-        msgBox.setText(ex.what());
-        msgBox.exec();
+        reportError(ex.what());
     }
     return { false, {} };
 }
 
+void CrudModel::connectForUpdate(Pgsql::Connection &connection) const
+{
+    auto dbconfig = m_database->config();
+    connection.connect(dbconfig.connectionString());
+}
+
+void CrudModel::reportError(const QString &message) const
+{
+    QMessageBox msgBox;
+    msgBox.setText(message);
+    msgBox.exec();
+}
+
+std::vector<CrudModel::Value> CrudModel::rowValues(const Pgsql::Row &row)
+{
+    std::vector<Value> values;
+    for (auto v : row)
+    {
+        if (v.null())
+            values.push_back(Value());
+        else
+            values.push_back(std::string(v.c_str()));
+    }
+    return values;
+}
+
 QString CrudModel::tableName(Oid table_oid) const
 {
     auto table = m_database->catalog()->classes()->getByKey(table_oid);
     return table ? table->fullyQualifiedQuotedObjectName() : QString();
 }
 
-std::vector<int> CrudModel::resultColumnsOfTable(Oid table_oid) const
-{
-    std::vector<int> cols;
-    for (int col = 0; col < m_roData->cols(); ++col)
-        if (m_roData->ftable(col) == table_oid && m_roData->ftableCol(col) > 0)
-            cols.push_back(col);
-    return cols;
-}
-
 QString CrudModel::attributeName(int col) const
 {
-    if (col < static_cast<int>(columnList.size()) && columnList[col])
-        return columnList[col]->name;
+    if (col < static_cast<int>(m_columnList.size()) && m_columnList[col])
+        return m_columnList[col]->name;
     return QString::fromUtf8(m_roData->getColName(col));
 }
 
@@ -493,7 +475,7 @@ std::tuple<QString, Pgsql::Params> CrudModel::createUpdateQuery(Oid table_oid, c
 std::tuple<QString, Pgsql::Params> CrudModel::createInsertQuery(const PendingRow &pending_row)
 {
 	Pgsql::Params params;
-	auto data = pending_row.data();
+	const auto &data = pending_row.data();
     QString table_name = m_table->fullyQualifiedQuotedObjectName();
 	QString buffer;
 	QTextStream q(&buffer);
@@ -523,7 +505,6 @@ std::tuple<QString, Pgsql::Params> CrudModel::createInsertQuery(const PendingRow
 
 QString CrudModel::createDeleteStatement() const
 {
-	Pgsql::Params params;
 	QString table_name = m_table->fullyQualifiedQuotedObjectName();
 	QString buffer;
 	QTextStream q(&buffer);
@@ -547,40 +528,25 @@ bool CrudModel::savePendingChanges()
     while (!m_pendingRowList.m_rows.empty())
     {
 		auto iter = m_pendingRowList.m_rows.begin();
-        int pendingKey = iter->first;
-        auto mapping_iter = std::find_if(m_rowMapping.begin(), m_rowMapping.end(),
-                                         [pendingKey](const RowMapping &rhs) -> bool { return rhs.rowKey == pendingKey; });
-        if (mapping_iter == m_rowMapping.end())
+        RowMapping *mapping = findRowMapping(iter->first);
+        if (!mapping)
             return false;
 
-        auto [ok, modified_row] = saveRow(iter->second, *mapping_iter);
-        if (ok)
-        {
-			int rowKey = iter->first;
-			m_pendingRowList.m_rows.erase(iter);
+        auto [ok, modified_row] = saveRow(iter->second, *mapping);
+        if (!ok)
+            return false;
 
-			auto iter = std::find_if(m_rowMapping.begin(), m_rowMapping.end(),
-                                  [rowKey](const RowMapping &rhs) -> bool { return rhs.rowKey == rowKey; });
-            if (iter != m_rowMapping.end())
-            {
-                iter->setModifiedRowData(modified_row);
-				iter->pending = false;
-                if (IsLastRow(iter))
-					appendNewRow();
-			}
-		}
-        else
-			return false;
+        m_pendingRowList.m_rows.erase(iter);
+        mapping->setModifiedRowData(modified_row);
+        mapping->pending = false;
+        // appending invalidates mapping, so keep this last
+        if (mapping == &m_rowMapping.back())
+            appendNewRow();
 	}
 
 	return true;
 }
 
-bool CrudModel::IsLastRow(RowMappingVector::iterator mapping_iter) const
-{
-    return mapping_iter == --m_rowMapping.end();
-}
-
 bool CrudModel::columnIsReadOnly(int column_index) const
 {
     if (m_roData == nullptr)
@@ -590,30 +556,30 @@ bool CrudModel::columnIsReadOnly(int column_index) const
     if (!m_mapping || !m_mapping->isEditable(col))
         return true;
 
-    const auto &column = columnList[col];
+    const auto &column = m_columnList[col];
     return !column || column->getIdentity() == PgAttribute::Identity::Always
             || column->getGenerated() != PgAttribute::Generated::None;
 }
 
 void CrudModel::initializeColumnList()
 {
-    columnList.clear();
-    columnList.reserve(m_roData->cols());
+    m_columnList.clear();
+    m_columnList.reserve(m_roData->cols());
     auto atts = m_database->catalog()->attributes();
     for (int col = 0; col < m_roData->cols(); ++col)
     {
         int attnum = m_roData->ftableCol(col);
         if (attnum <= 0)
         {
-            columnList.emplace_back();
+            m_columnList.emplace_back();
             continue;
         }
         auto columns = atts->getColumnsForRelation(m_roData->ftable(col));
         auto find_result = std::find_if(columns.begin(), columns.end(), [attnum](const PgAttribute &att) { return att.num == attnum; });
         if (find_result != columns.end())
-            columnList.emplace_back(*find_result);
+            m_columnList.emplace_back(*find_result);
         else
-            columnList.emplace_back();
+            m_columnList.emplace_back();
     }
 }
 
@@ -621,8 +587,7 @@ bool CrudModel::saveChanges()
 {
     bool was_dirty = hasPendingChanges();
     bool ok = savePendingChanges();
-    if (was_dirty != hasPendingChanges())
-        emit pendingChangesChanged(hasPendingChanges());
+    updatePendingState(was_dirty);
     return ok;
 }
 
@@ -664,49 +629,42 @@ std::tuple<bool, QString> CrudModel::removeRows(const std::set<IntegerRange<int>
         return { false, "Range error" };
     if (!m_wholeTable || !m_table || m_mapping->primaryKeyColumns(m_table->oid()).empty())
         return { false, "Rows can only be removed from a table with a primary key" };
-	// When removing rows there is no direct mapping anymore between the rows in the grid
-	// and the rows in m_roData
-
-	// Therefor we need an indirection to keep track of which rows are visible and which
-	// grid row maps to what data row. Maybe we can also keep track where the current data
-	// of the row is located original data, pending data or modified data
-
-	// 1. Get PKEY and remove that row from table
 
+    // When removing rows there is no direct mapping anymore between the rows in the grid
+    // and the rows in m_roData, that is why the grid rows are mapped through m_rowMapping.
     try
     {
-		Pgsql::Connection db_update_conn;
-		auto dbconfig = m_database->config();
-		db_update_conn.connect(dbconfig.connectionString());
-
-		// First delete rows in table
-		QString delete_statement = createDeleteStatement();
-		{
-			auto tx = Pgsql::Transaction::startTransaction(db_update_conn);
-            for (auto range : row_ranges)
-            {
-                for (int current_row = range.start(); current_row < range.end(); ++current_row)
-                {
-					auto&& mapping = m_rowMapping[static_cast<size_t>(current_row)];
-					auto params = getPKeyParamsForRow(mapping, m_table->oid());
-                    if (!params.empty())
-						db_update_conn.queryParam(delete_statement, params);
-				}
-			}
-			tx.commit();
-			// If something goes wrong after this commit we should reload contents of model
-		}
-		// Then from model
+        deleteRowsFromDatabase(row_ranges);
+        // If something goes wrong after the commit we should reload contents of model
         bool was_dirty = hasPendingChanges();
-        RemoveRangesOfRowsFromModel(row_ranges);
-        if (was_dirty != hasPendingChanges())
-            emit pendingChangesChanged(hasPendingChanges());
-		return  { true, "" };
+        removeRangesFromModel(row_ranges);
+        updatePendingState(was_dirty);
+        return { true, "" };
     }
     catch (const Pgsql::PgResultError &error)
     {
-		return { false, QString::fromUtf8(error.details().messageDetail.c_str()) };
-	}
+        return { false, QString::fromUtf8(error.details().messageDetail.c_str()) };
+    }
+}
+
+void CrudModel::deleteRowsFromDatabase(const std::set<IntegerRange<int>> &row_ranges)
+{
+    Pgsql::Connection db_update_conn;
+    connectForUpdate(db_update_conn);
+
+    QString delete_statement = createDeleteStatement();
+    auto tx = Pgsql::Transaction::startTransaction(db_update_conn);
+    for (const auto &range : row_ranges)
+    {
+        for (int current_row = range.start(); current_row < range.end(); ++current_row)
+        {
+            const auto &mapping = m_rowMapping[static_cast<size_t>(current_row)];
+            auto params = getPKeyParamsForRow(mapping, m_table->oid());
+            if (!params.empty())
+                db_update_conn.queryParam(delete_statement, params);
+        }
+    }
+    tx.commit();
 }
 
 bool CrudModel::removeRows(int row, int count, const QModelIndex &)
@@ -719,18 +677,18 @@ bool CrudModel::removeRows(int row, int count, const QModelIndex &)
 	return res;
 }
 
-void CrudModel::RemoveRangesOfRowsFromModel(const std::set<IntegerRange<int>> &row_ranges)
+void CrudModel::removeRangesFromModel(const std::set<IntegerRange<int>> &row_ranges)
 {
     int rows_deleted = 0;
     for (auto range : row_ranges)
     {
         range.setStart(range.start() - rows_deleted);
-        RemoveRangeOfRowsFromModel(range);
+        removeRangeFromModel(range);
         rows_deleted += range.length();
     }
 }
 
-void CrudModel::RemoveRangeOfRowsFromModel(IntegerRange<int> range)
+void CrudModel::removeRangeFromModel(IntegerRange<int> range)
 {
     beginRemoveRows(QModelIndex(), range.start(), range.end() - 1);
     SCOPE_EXIT { endRemoveRows(); };

pglab/crud/CrudModel.h

@@ -60,11 +60,10 @@ signals:
 	/// Emitted when the model goes from clean to having pending changes or back
 	void pendingChangesChanged(bool hasChanges);
 
-public slots:
-
 private:
 
-    const int PreColumnCount = 1;
+	/// Number of grid columns in front of the result columns (the row number column)
+	static constexpr int PreColumnCount = 1;
 
 
 	class ColumnSort {
@@ -125,17 +124,6 @@ private:
 	public:
 		using Map = std::map<int, PendingRow>;
 
-		PendingRow& getRow(int row)
-		{
-			auto iter = m_rows.lower_bound(row);
-			if (iter != m_rows.end() && iter->first == row) {
-				return iter->second;
-			}
-			else {
-				return m_rows.insert(iter, {row, PendingRow(row)})->second;
-			}
-		}
-
 		void setValue(int col, int row, const Value &value)
 		{
 			auto iter = m_rows.find(row);
@@ -226,13 +214,7 @@ private:
 
 	std::shared_ptr<Pgsql::Result> m_roData;
 
-    struct ColumnData
-    {
-        std::string name;
-        // identity allways or generated
-    };
-
-    std::vector<std::optional<PgAttribute>> columnList; // attribute per column in m_roData, empty for expression columns
+	std::vector<std::optional<PgAttribute>> m_columnList; // attribute per column in m_roData, empty for expression columns
 
 	PendingRowList m_pendingRowList;
 
@@ -242,47 +224,64 @@ private:
     /// call on initial load to fill in the mappings
 	void initRowMapping();
 
+    /// Value as shown in the grid, pending edits take precedence over saved data
     Value getLatestData(int column, int row) const;
     Value getLatestData(const QModelIndex &index) const
     {
         return getLatestData(index.column() - PreColumnCount, index.row());
     }
-    Value getSavedData(int columnIndex, int rowIndex) const;
+    /// Value as last loaded from or saved to the database
     Value getSavedData(const RowMapping &row_mapping, int columnIndex) const;
 
+    /// Mapping of the row with the given key, nullptr when the row is not in the grid (anymore)
+    RowMapping* findRowMapping(int rowKey);
+    /// Call after changing the pending rows to emit pendingChangesChanged when the state toggled
+    void updatePendingState(bool was_dirty);
+
 	Oid getType(int column) const;
 
-
-    /// Get the PKey values from when the row was last restored or retrieved.
-    Pgsql::Params getPKeyParamsForRow(const RowMapping &row_mapping, Oid table_oid) const;
+	// ---- Saving, all of these only use the data of the pending row and the row mapping
 
 	bool savePendingChanges();
+	/// Inserts the row when it is new otherwise updates it. Returns the values of the row as stored in the database.
+	std::tuple<bool, std::vector<Value>> saveRow(const PendingRow &pending_row, const RowMapping &row_mapping);
+	std::tuple<bool, std::vector<Value>> saveUpdate(const PendingRow &pending_row, const RowMapping &row_mapping);
 
-    std::tuple<QString, Pgsql::Params> createUpdateQuery(Oid table_oid, const PendingRow &pending_row, const RowMapping &row_mapping);
+	/// Get the PKey values from when the row was last restored or retrieved.
+	Pgsql::Params getPKeyParamsForRow(const RowMapping &row_mapping, Oid table_oid) const;
+	std::tuple<QString, Pgsql::Params> createUpdateQuery(Oid table_oid, const PendingRow &pending_row, const RowMapping &row_mapping);
 	std::tuple<QString, Pgsql::Params> createInsertQuery(const PendingRow &pending_row);
-
 	QString createDeleteStatement() const;
-    std::tuple<bool, std::vector<Value>> saveRow(const PendingRow &pending_row, const RowMapping &row_mapping);
-    std::tuple<bool, std::vector<Value>> saveUpdate(const PendingRow &pending_row, const RowMapping &row_mapping);
+
+	/// Opens a separate connection for writing so edits don't interfere with the loading connection
+	void connectForUpdate(Pgsql::Connection &connection) const;
+	/// Tells the user about a failed write, the one place where the model shows a dialog
+	void reportError(const QString &message) const;
+	static std::vector<Value> rowValues(const Pgsql::Row &row);
+
+	// ---- Rows
 
 	void appendNewRow();
-    void appendNewRowInternal();
+	void appendNewRowInternal();
 
-    int lastRowKey = -1;
+	int lastRowKey = -1;
 	int allocNewRowKey() { return ++lastRowKey; }
+
+	/// Deletes the rows from the database in one transaction, throws on failure
+	void deleteRowsFromDatabase(const std::set<IntegerRange<int>> &row_ranges);
+	/// Removes the rows from the grid and drops their pending edits
+	void removeRangesFromModel(const std::set<IntegerRange<int>> &row_ranges);
+	void removeRangeFromModel(IntegerRange<int> row_range);
+
+	// ---- Table and column information
+
 	/// Name of the table with the given oid, quoted and schema qualified
 	QString tableName(Oid table_oid) const;
-	/// Result columns that are user columns (attnum > 0) of the given table, in result order
-	std::vector<int> resultColumnsOfTable(Oid table_oid) const;
 	/// Catalog name of the attribute behind a result column, falls back to the result column name
 	QString attributeName(int col) const;
+	bool columnIsReadOnly(int column_index) const;
+	void initializeColumnList();
 
-    void RemoveRangesOfRowsFromModel(const std::set<IntegerRange<int>> &row_ranges);
-    void RemoveRangeOfRowsFromModel(IntegerRange<int> row_range);
-
-    bool IsLastRow(RowMappingVector::iterator mapping_iter) const;
-    bool columnIsReadOnly(int column_index) const;
-    void initializeColumnList();
 private slots:
 
 	void loadIntoModel(std::shared_ptr<Pgsql::Result> data);

tests/integration/CMakeLists.txt

@@ -3,6 +3,20 @@ add_executable(IntegrationTests
     tst_Smoke.cpp
     tst_CatalogLoad.cpp
     tst_DatabaseSizes.cpp
+    tst_DataGridMapping.cpp
+    tst_DataGridUpdate.cpp
+    tst_CrudModelBehavior.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/crud/CrudModel.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/crud/DataGridWidget.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/OpenDatabase.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/util/PgLabItemDelegate.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/util/Colors.cpp
+    ${CMAKE_SOURCE_DIR}/pglab/ResultTableModelUtil.cpp
+    )
+
+target_include_directories(IntegrationTests PRIVATE
+    ${CMAKE_SOURCE_DIR}/pglab
+    ${CMAKE_SOURCE_DIR}/pglab/crud
     )
 
 target_link_libraries(IntegrationTests PRIVATE
@@ -10,6 +24,10 @@ target_link_libraries(IntegrationTests PRIVATE
     pglablib
     GTest::gtest
     Qt6::Core
+    Qt6::Gui
+    Qt6::Widgets
+    Qt6::Sql
+    Qt6::Concurrent
     )
 
 target_compile_definitions(IntegrationTests PRIVATE

tests/integration/DataGridTest.h

@@ -0,0 +1,96 @@
+#pragma once
+
+#include <gtest/gtest.h>
+
+#include "DbTest.h"
+#include "ConnectionConfig.h"
+#include "OpenDatabase.h"
+#include "crud/CrudModel.h"
+#include "crud/DataGridWidget.h"
+#include "Pgsql_Connection.h"
+
+#include <QCoreApplication>
+#include <QDeadlineTimer>
+#include <QSortFilterProxyModel>
+#include "catalog/PgClassContainer.h"
+#include "catalog/PgDatabaseCatalog.h"
+#include <QUrl>
+#include <memory>
+#include <optional>
+#include <string>
+#include <vector>
+
+class DataGridTest : public pgtest::DbTest {
+protected:
+	std::shared_ptr<OpenDatabase> openDb()
+	{
+		QUrl url(dbUrl());
+		ConnectionConfig cfg;
+		cfg.setHost(url.host());
+		cfg.setPort(static_cast<unsigned short>(url.port(5432)));
+		cfg.setUser(url.userName());
+		cfg.setPassword(url.password());
+		cfg.setDbname(url.path().mid(1));
+		return OpenDatabase::createOpenDatabase(cfg);
+	}
+
+	/// Runs the query and shows its result in a grid
+	void showQuery(const char *sql)
+	{
+		db = openDb();
+		auto result = std::make_shared<Pgsql::Result>(conn().query(sql));
+		model = std::make_unique<CrudModel>();
+		widget = std::make_unique<DataGridWidget>();
+		widget->setSourceModel(model.get());
+		model->setResult(db, result);
+	}
+
+	/// Shows a whole table (CRUD mode) and waits for the asynchronous load to finish
+	bool showTable(const QString &name)
+	{
+		db = openDb();
+		auto table = db->catalog()->classes()->findIf([&name](const PgClass &c)
+			{ return c.fullyQualifiedQuotedObjectName().endsWith("." + name); });
+		if (!table)
+			return false;
+		model = std::make_unique<CrudModel>();
+		widget = std::make_unique<DataGridWidget>();
+		widget->setSourceModel(model.get());
+		bool reset = false;
+		QObject::connect(model.get(), &QAbstractItemModel::modelReset, model.get(), [&reset] { reset = true; });
+		model->setConfig(db, *table);
+		QDeadlineTimer deadline(10000);
+		while (!reset && !deadline.hasExpired())
+			QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+		return reset;
+	}
+
+	/// Editable flag of every result column, as seen through the widget's proxy
+	std::vector<bool> editableColumns()
+	{
+		auto proxy = widget->proxy();
+		std::vector<bool> r;
+		for (int c = 1; c < proxy->columnCount(); ++c)
+			r.push_back(proxy->flags(proxy->index(0, c)) & Qt::ItemIsEditable);
+		return r;
+	}
+
+	QString columnName(int resultCol)
+	{
+		return widget->proxy()->headerData(resultCol + 1, Qt::Horizontal).toString().section('\n', 0, 0);
+	}
+
+	/// Reads a single cell with the test's own connection
+	std::optional<std::string> dbValue(const char *sql)
+	{
+		auto res = conn().query(sql);
+		auto v = res.get(0, 0);
+		if (v.null())
+			return std::nullopt;
+		return std::string(v.c_str());
+	}
+
+	std::shared_ptr<OpenDatabase> db;
+	std::unique_ptr<CrudModel> model;
+	std::unique_ptr<DataGridWidget> widget;
+};

tests/integration/fixtures/schema.sql

@@ -4,3 +4,57 @@ CREATE TABLE fixture_item (
 );
 
 INSERT INTO fixture_item (id, name) VALUES (1, 'one'), (2, 'two');
+
+CREATE TABLE grid_parent (
+    id integer PRIMARY KEY,
+    name text NOT NULL
+);
+INSERT INTO grid_parent (id, name) VALUES (1, 'alpha'), (2, 'beta'), (3, 'gamma');
+
+CREATE TABLE grid_child (
+    id integer PRIMARY KEY,
+    parent_id integer,
+    note text
+);
+INSERT INTO grid_child (id, parent_id, note) VALUES (10, 1, 'c1'), (11, 2, 'c2'), (12, 3, 'c3');
+
+CREATE TABLE grid_composite (
+    a integer,
+    b integer,
+    val text,
+    PRIMARY KEY (a, b)
+);
+INSERT INTO grid_composite (a, b, val) VALUES (1, 1, 'x'), (1, 2, 'y'), (2, 1, 'z');
+
+CREATE TABLE grid_nopk (
+    x integer,
+    y text
+);
+INSERT INTO grid_nopk (x, y) VALUES (1, 'a'), (2, 'b');
+
+CREATE TABLE crud_item (
+    id serial PRIMARY KEY,
+    name text NOT NULL,
+    qty integer,
+    flag boolean,
+    born date,
+    tag text DEFAULT 'dflt'
+);
+INSERT INTO crud_item (name, qty, flag, born) VALUES
+    ('a', 1, true, '2020-01-02'),
+    ('b', 2, false, NULL),
+    ('c', NULL, NULL, '2021-03-04'),
+    ('d', 4, true, NULL),
+    ('e', 5, false, NULL);
+
+CREATE TABLE crud_empty (
+    id serial PRIMARY KEY,
+    v text
+);
+
+CREATE TABLE crud_gen (
+    id integer GENERATED ALWAYS AS IDENTITY PRIMARY KEY,
+    a integer,
+    b integer GENERATED ALWAYS AS (a * 2) STORED
+);
+INSERT INTO crud_gen (a) VALUES (1), (2);

tests/integration/main.cpp

@@ -1,9 +1,15 @@
 #include <gtest/gtest.h>
 
+#include <QApplication>
+#include <QtGlobal>
+
 #include "PgTestEnvironment.h"
 
 int main(int argc, char *argv[])
 {
+	if (!qEnvironmentVariableIsSet("QT_QPA_PLATFORM"))
+		qputenv("QT_QPA_PLATFORM", "offscreen");
+	QApplication app(argc, argv);
 	::testing::InitGoogleTest(&argc, argv);
 	::testing::AddGlobalTestEnvironment(new pgtest::PgEnv(PGTEST_FIXTURE_DIR));
 	return RUN_ALL_TESTS();

tests/integration/tst_CrudModelBehavior.cpp

@@ -0,0 +1,656 @@
+// Characterization tests for CrudModel, they lock in the behavior of the model
+// so it can be refactored safely.
+#include "DataGridTest.h"
+
+#include "CustomDataRole.h"
+#include "Pgsql_oids.h"
+
+#include <QApplication>
+#include <QBrush>
+#include <QDate>
+#include <QTimer>
+
+namespace {
+
+/// Failed saves show a modal message box, this closes it so the test can continue
+void autoCloseModalDialogs(int triesLeft = 100)
+{
+	if (auto w = QApplication::activeModalWidget())
+		w->close();
+	else if (triesLeft > 0)
+		QTimer::singleShot(20, [triesLeft] { autoCloseModalDialogs(triesLeft - 1); });
+}
+
+/// Records the values of CrudModel::pendingChangesChanged
+struct PendingSpy {
+	explicit PendingSpy(CrudModel *m)
+	{
+		QObject::connect(m, &CrudModel::pendingChangesChanged, m, [this](bool b) { values.push_back(b); });
+	}
+	std::vector<bool> values;
+};
+
+/// Counts the emissions of a signal
+struct CountSpy {
+	template <typename Sender, typename Func>
+	CountSpy(Sender *sender, Func signal)
+	{
+		QObject::connect(sender, signal, sender, [this] { ++n; });
+	}
+	int count() const { return n; }
+	int n = 0;
+};
+
+// crud_item columns
+constexpr int ColId = 0;
+constexpr int ColName = 1;
+constexpr int ColQty = 2;
+constexpr int ColFlag = 3;
+constexpr int ColBorn = 4;
+constexpr int ColTag = 5;
+
+class CrudModelBehaviorTest : public DataGridTest {
+protected:
+	QModelIndex idx(int row, int resultCol) const { return model->index(row, resultCol + 1); }
+
+	QVariant cell(int row, int resultCol, int role = Qt::DisplayRole) const
+	{
+		return model->data(idx(row, resultCol), role);
+	}
+
+	bool edit(int row, int resultCol, const QString &value)
+	{
+		return model->setData(idx(row, resultCol), value, Qt::EditRole);
+	}
+
+	/// Saves while auto closing any error dialog
+	bool save()
+	{
+		QTimer::singleShot(0, [] { autoCloseModalDialogs(); });
+		return model->saveChanges();
+	}
+
+	/// Row of crud_item with the given name, -1 if not shown
+	int rowOfName(const QString &name) const
+	{
+		for (int r = 0; r < model->rowCount(); ++r)
+			if (cell(r, ColName, Qt::EditRole).toString() == name)
+				return r;
+		return -1;
+	}
+
+	QStringList names() const
+	{
+		QStringList l;
+		for (int r = 0; r < model->rowCount(); ++r)
+			l << cell(r, ColName, Qt::EditRole).toString();
+		return l;
+	}
+
+	void reload()
+	{
+		bool reset = false;
+		QObject::connect(model.get(), &QAbstractItemModel::modelReset, model.get(), [&reset] { reset = true; });
+		model->loadData();
+		QDeadlineTimer deadline(10000);
+		while (!reset && !deadline.hasExpired())
+			QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+		ASSERT_TRUE(reset);
+	}
+};
+
+} // namespace
+
+// ---- loading, headers, row number column
+
+TEST_F(CrudModelBehaviorTest, WholeTableLoadsRowsPlusEmptyNewRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(model->columnCount(), 7);
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", ""}));
+	for (int c = 0; c <= ColTag; ++c)
+		EXPECT_FALSE(cell(5, c).isValid()) << c;
+}
+
+TEST_F(CrudModelBehaviorTest, EmptyTableHasOnlyTheNewRow)
+{
+	ASSERT_TRUE(showTable("crud_empty"));
+	EXPECT_EQ(model->rowCount(), 1);
+}
+
+TEST_F(CrudModelBehaviorTest, QueryResultHasNoNewRow)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	EXPECT_EQ(model->rowCount(), 3);
+}
+
+TEST_F(CrudModelBehaviorTest, HeaderShowsNameAndType)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(model->headerData(0, Qt::Horizontal).toString(), "#");
+	for (int c = 0; c <= ColTag; ++c)
+	{
+		auto text = model->headerData(c + 1, Qt::Horizontal, Qt::DisplayRole).toString();
+		EXPECT_EQ(text.count('\n'), 1) << c;
+		EXPECT_FALSE(text.section('\n', 1).isEmpty()) << c;
+	}
+	EXPECT_EQ(columnName(ColId), "id");
+	EXPECT_EQ(columnName(ColName), "name");
+	EXPECT_EQ(columnName(ColTag), "tag");
+	EXPECT_FALSE(model->headerData(1, Qt::Horizontal, Qt::EditRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, RowNumberColumnData)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	for (int r = 0; r < model->rowCount(); ++r)
+	{
+		auto i = model->index(r, 0);
+		EXPECT_EQ(model->data(i, Qt::DisplayRole).toInt(), r + 1);
+		EXPECT_EQ(model->data(i, CustomSortRole).toInt(), r + 1);
+		EXPECT_EQ(model->data(i, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+		EXPECT_FALSE(model->data(i, Qt::EditRole).isValid());
+		EXPECT_FALSE(model->data(i, Qt::ForegroundRole).isValid());
+	}
+	EXPECT_FALSE(model->setData(model->index(0, 0), "5", Qt::EditRole));
+	EXPECT_FALSE(model->hasPendingChanges());
+}
+
+// ---- data roles
+
+TEST_F(CrudModelBehaviorTest, DisplayAndEditRoles)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(cell(0, ColName).toString(), "a");
+	EXPECT_EQ(cell(0, ColQty).toString(), "1");
+	EXPECT_EQ(cell(0, ColQty, Qt::EditRole).toString(), "1");
+	EXPECT_EQ(cell(0, ColBorn).toString(), "2020-01-02");
+	EXPECT_EQ(cell(0, ColTag).toString(), "dflt");
+	// NULL
+	EXPECT_FALSE(cell(1, ColBorn).isValid());
+	EXPECT_FALSE(cell(1, ColBorn, Qt::EditRole).isValid());
+	// Roles that are not handled
+	EXPECT_FALSE(cell(0, ColName, Qt::ToolTipRole).isValid());
+	EXPECT_FALSE(cell(0, ColName, Qt::ForegroundRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, BoolDisplaysAsBool)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto t = cell(0, ColFlag);
+	EXPECT_EQ(t.typeId(), QMetaType::Bool);
+	EXPECT_TRUE(t.toBool());
+	auto f = cell(1, ColFlag);
+	EXPECT_EQ(f.typeId(), QMetaType::Bool);
+	EXPECT_FALSE(f.toBool());
+	EXPECT_FALSE(cell(2, ColFlag).isValid());
+	EXPECT_EQ(cell(0, ColFlag, Qt::EditRole).toString(), "t");
+}
+
+TEST_F(CrudModelBehaviorTest, DisplayTruncatesButEditDoesNot)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, QString(300, 'x')));
+	EXPECT_EQ(cell(0, ColName, Qt::DisplayRole).toString().size(), 256);
+	EXPECT_EQ(cell(0, ColName, Qt::EditRole).toString().size(), 300);
+}
+
+TEST_F(CrudModelBehaviorTest, SortRoleGivesTypedValues)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto i = cell(0, ColQty, CustomSortRole);
+	EXPECT_EQ(i.typeId(), QMetaType::Int);
+	EXPECT_EQ(i.toInt(), 1);
+	EXPECT_EQ(cell(0, ColBorn, CustomSortRole).toDate(), QDate(2020, 1, 2));
+	EXPECT_EQ(cell(0, ColBorn, CustomSortRole).typeId(), QMetaType::QDate);
+	auto b = cell(0, ColFlag, CustomSortRole);
+	EXPECT_EQ(b.typeId(), QMetaType::Bool);
+	EXPECT_TRUE(b.toBool());
+	EXPECT_EQ(cell(0, ColName, CustomSortRole).toString(), "a");
+	EXPECT_FALSE(cell(1, ColBorn, CustomSortRole).isValid());
+	EXPECT_FALSE(cell(2, ColQty, CustomSortRole).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, DataTypeRole)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(cell(0, ColId, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+	EXPECT_EQ(cell(0, ColName, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::text_oid));
+	EXPECT_EQ(cell(0, ColFlag, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::bool_oid));
+	EXPECT_EQ(cell(0, ColBorn, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::date_oid));
+	// also for the new row
+	EXPECT_EQ(cell(5, ColQty, CustomDataTypeRole).toUInt(), static_cast<uint>(Pgsql::int4_oid));
+}
+
+TEST_F(CrudModelBehaviorTest, ReadOnlyColumnsAreGrayInQueryMode)
+{
+	showQuery("SELECT id, name || 'x' AS e FROM grid_parent");
+	EXPECT_FALSE(cell(0, 0, Qt::ForegroundRole).isValid());
+	auto brush = cell(0, 1, Qt::ForegroundRole);
+	ASSERT_TRUE(brush.isValid());
+	EXPECT_EQ(brush.value<QBrush>().color(), QColor(Qt::gray));
+}
+
+// ---- flags
+
+TEST_F(CrudModelBehaviorTest, FlagsOfGeneratedAndIdentityColumns)
+{
+	ASSERT_TRUE(showTable("crud_gen"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, true, false}));
+	auto f = model->flags(model->index(0, 0));
+	EXPECT_TRUE(f & Qt::ItemIsSelectable);
+	EXPECT_TRUE(f & Qt::ItemIsEnabled);
+	EXPECT_FALSE(f & Qt::ItemIsEditable);
+}
+
+TEST_F(CrudModelBehaviorTest, FlagsOfWholeTable)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>(6, true)));
+}
+
+TEST_F(CrudModelBehaviorTest, ExpressionColumnIsNotEditable)
+{
+	showQuery("SELECT id, name || 'x' AS e FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, false}));
+}
+
+TEST_F(CrudModelBehaviorTest, TableWithoutPkIsNotEditable)
+{
+	ASSERT_TRUE(showTable("grid_nopk"));
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, false}));
+}
+
+// ---- setData
+
+TEST_F(CrudModelBehaviorTest, SetDataRejectsOtherRoles)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_FALSE(model->setData(idx(0, ColName), "x", Qt::DisplayRole));
+	EXPECT_FALSE(model->hasPendingChanges());
+}
+
+TEST_F(CrudModelBehaviorTest, SetDataShowsPendingValueAndEmitsDataChanged)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	CountSpy spy(model.get(), &QAbstractItemModel::dataChanged);
+	ASSERT_TRUE(edit(0, ColName, "changed"));
+	EXPECT_EQ(spy.count(), 1);
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(cell(0, ColName).toString(), "changed");
+	EXPECT_EQ(cell(1, ColName).toString(), "b");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "a");
+}
+
+TEST_F(CrudModelBehaviorTest, EmptyStringBecomesNullAndQuotesBecomeEmptyString)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColTag, ""));
+	ASSERT_TRUE(edit(1, ColTag, "''"));
+	EXPECT_FALSE(cell(0, ColTag).isValid());
+	EXPECT_TRUE(cell(1, ColTag).isValid());
+	EXPECT_EQ(cell(1, ColTag).toString(), "");
+	ASSERT_TRUE(save());
+
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'a'"), std::nullopt);
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'b'"), "");
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'c'"), "dflt");
+}
+
+TEST_F(CrudModelBehaviorTest, RepeatedEditsOfSameCellKeepLastValue)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "one"));
+	ASSERT_TRUE(edit(0, ColName, "two"));
+	ASSERT_TRUE(edit(1, ColName, "three"));
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "two");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "three");
+}
+
+// ---- saving updates
+
+TEST_F(CrudModelBehaviorTest, SavedUpdateIsShownAfterwards)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(2, ColQty, "42"));
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(cell(2, ColQty).toString(), "42");
+	EXPECT_EQ(cell(2, ColName).toString(), "c");
+	EXPECT_EQ(cell(2, ColBorn).toString(), "2021-03-04");
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'c'"), "42");
+}
+
+TEST_F(CrudModelBehaviorTest, SaveWithoutChangesIsOk)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	EXPECT_TRUE(model->saveChanges());
+	EXPECT_TRUE(spy.values.empty());
+}
+
+TEST_F(CrudModelBehaviorTest, RevertThenEditAgain)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "discarded"));
+	model->revertChanges();
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(cell(0, ColName).toString(), "a");
+	model->revertChanges();	// nothing to revert, no signal
+	ASSERT_TRUE(edit(0, ColName, "kept"));
+	EXPECT_EQ(cell(0, ColName).toString(), "kept");
+	ASSERT_TRUE(save());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false, true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "kept");
+}
+
+TEST_F(CrudModelBehaviorTest, RevertDoesNotRemoveSavedValues)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, "saved"));
+	ASSERT_TRUE(save());
+	ASSERT_TRUE(edit(0, ColName, "unsaved"));
+	model->revertChanges();
+	EXPECT_EQ(cell(0, ColName).toString(), "saved");
+}
+
+TEST_F(CrudModelBehaviorTest, PartialFailureKeepsOnlyFailingRowPending)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(0, ColName, "ok"));
+	ASSERT_TRUE(edit(1, ColName, ""));	// NULL violates NOT NULL
+	ASSERT_TRUE(edit(2, ColName, "never saved"));
+	EXPECT_FALSE(save());
+
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 1"), "ok");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "b");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 3"), "c");
+	EXPECT_EQ(cell(0, ColName).toString(), "ok");
+	EXPECT_FALSE(cell(1, ColName).isValid());	// still shows the pending value
+
+	// Fixing the failing row lets the rest be saved
+	ASSERT_TRUE(edit(1, ColName, "fixed"));
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 2"), "fixed");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 3"), "never saved");
+}
+
+TEST_F(CrudModelBehaviorTest, UpdateOfRowChangedElsewhereFails)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	conn().query("DELETE FROM crud_item WHERE id = 1");
+	ASSERT_TRUE(edit(0, ColName, "gone"));
+	EXPECT_FALSE(save());
+	EXPECT_TRUE(model->hasPendingChanges());
+}
+
+// ---- reload
+
+TEST_F(CrudModelBehaviorTest, ReloadClearsPendingChangesAndShowsDbState)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(0, ColName, "pending"));
+	conn().query("INSERT INTO crud_item (name) VALUES ('f')");
+	PendingSpy spy(model.get());
+	reload();
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{false}));
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "f", ""}));
+}
+
+TEST_F(CrudModelBehaviorTest, ReloadWithoutPendingChangesEmitsNoSignal)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	reload();
+	EXPECT_TRUE(spy.values.empty());
+	EXPECT_EQ(model->rowCount(), 6);
+}
+
+// ---- insert
+
+TEST_F(CrudModelBehaviorTest, InsertNewRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(5, ColName, "new"));
+	ASSERT_TRUE(edit(5, ColQty, "7"));
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	ASSERT_TRUE(save());
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "6");
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'new'"), "7");
+
+	// RETURNING * brings the defaults into the model
+	EXPECT_EQ(cell(5, ColName).toString(), "new");
+	EXPECT_EQ(cell(5, ColQty).toString(), "7");
+	EXPECT_EQ(cell(5, ColTag).toString(), "dflt");
+	EXPECT_EQ(cell(5, ColId).toString(), *dbValue("SELECT id FROM crud_item WHERE name = 'new'"));
+	EXPECT_FALSE(cell(5, ColBorn).isValid());
+
+	// and a fresh empty row is appended
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "new", ""}));
+	EXPECT_FALSE(cell(6, ColId).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, InsertTwoRowsInARow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "n1"));
+	ASSERT_TRUE(save());
+	ASSERT_TRUE(edit(6, ColName, "n2"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 8);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "7");
+	EXPECT_EQ(names(), (QStringList{"a", "b", "c", "d", "e", "n1", "n2", ""}));
+}
+
+TEST_F(CrudModelBehaviorTest, InsertIntoEmptyTable)
+{
+	ASSERT_TRUE(showTable("crud_empty"));
+	ASSERT_TRUE(edit(0, 1, "first"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 2);
+	EXPECT_EQ(dbValue("SELECT v FROM crud_empty"), "first");
+	EXPECT_EQ(cell(0, 0).toString(), "1");
+}
+
+TEST_F(CrudModelBehaviorTest, InsertWithExplicitNull)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "nulltag"));
+	ASSERT_TRUE(edit(5, ColTag, ""));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(dbValue("SELECT tag FROM crud_item WHERE name = 'nulltag'"), std::nullopt);
+	EXPECT_FALSE(cell(5, ColTag).isValid());
+}
+
+TEST_F(CrudModelBehaviorTest, FailedInsertKeepsRowPending)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColQty, "9"));	// name is NOT NULL and missing
+	EXPECT_FALSE(save());
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+	EXPECT_EQ(cell(5, ColQty).toString(), "9");
+
+	ASSERT_TRUE(edit(5, ColName, "now valid"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(model->rowCount(), 7);
+	EXPECT_EQ(dbValue("SELECT qty FROM crud_item WHERE name = 'now valid'"), "9");
+}
+
+TEST_F(CrudModelBehaviorTest, EditingOnlyThenRevertingNewRowDoesNotInsert)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(edit(5, ColName, "temp"));
+	model->revertChanges();
+	EXPECT_FALSE(cell(5, ColName).isValid());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+// ---- delete
+
+TEST_F(CrudModelBehaviorTest, RemoveSingleRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	CountSpy spy(model.get(), &QAbstractItemModel::rowsRemoved);
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(spy.count(), 1);
+	EXPECT_EQ(names(), (QStringList{"a", "c", "d", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "4");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item WHERE name = 'b'"), "0");
+	EXPECT_EQ(model->data(model->index(4, 0), Qt::DisplayRole).toInt(), 5);
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveContiguousRange)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 3)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(names(), (QStringList{"a", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveNonContiguousRanges)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1), IntegerRange<int>(2, 2)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(names(), (QStringList{"b", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT string_agg(name, ',' ORDER BY name) FROM crud_item"), "b,e");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveRowsOverloadForViews)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	EXPECT_TRUE(model->removeRows(0, 2));
+	EXPECT_EQ(names(), (QStringList{"c", "d", "e", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveNothingIsOk)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({});
+	EXPECT_TRUE(ok);
+	EXPECT_EQ(model->rowCount(), 6);
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveDropsPendingEditsOfRemovedRows)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(1, ColName, "edited then removed"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok);
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+}
+
+TEST_F(CrudModelBehaviorTest, PendingEditsOfOtherRowsSurviveRemoval)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	PendingSpy spy(model.get());
+	ASSERT_TRUE(edit(3, ColName, "d edited"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 3)});
+	EXPECT_TRUE(ok);
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+	EXPECT_EQ(names(), (QStringList{"d edited", "e", ""}));
+
+	ASSERT_TRUE(save());
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 4"), "d edited");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 5"), "e");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, EditAfterRemovalUpdatesTheRightRow)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(std::get<0>(model->removeRows({IntegerRange<int>(0, 3)})));
+	ASSERT_TRUE(edit(1, ColName, "e edited"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 5"), "e edited");
+	EXPECT_EQ(dbValue("SELECT name FROM crud_item WHERE id = 4"), "d");
+}
+
+TEST_F(CrudModelBehaviorTest, InsertAfterRemoval)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	ASSERT_TRUE(std::get<0>(model->removeRows({IntegerRange<int>(0, 1)})));
+	ASSERT_TRUE(edit(model->rowCount() - 1, ColName, "after delete"));
+	ASSERT_TRUE(save());
+	EXPECT_EQ(names(), (QStringList{"b", "c", "d", "e", "after delete", ""}));
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveOutOfRangeFails)
+{
+	ASSERT_TRUE(showTable("crud_item"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(5, 5)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 6);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM crud_item"), "5");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromQueryResultFails)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 3);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_parent"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromTableWithoutPkFails)
+{
+	ASSERT_TRUE(showTable("grid_nopk"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_FALSE(msg.isEmpty());
+	EXPECT_EQ(model->rowCount(), 3);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_nopk"), "2");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveRowReferencedByForeignKeyFails)
+{
+	conn().query("ALTER TABLE grid_child ADD FOREIGN KEY (parent_id) REFERENCES grid_parent (id)");
+	ASSERT_TRUE(showTable("grid_parent"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(0, 1)});
+	EXPECT_FALSE(ok);
+	EXPECT_EQ(model->rowCount(), 4);
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_parent"), "3");
+}
+
+TEST_F(CrudModelBehaviorTest, RemoveFromCompositePkTable)
+{
+	ASSERT_TRUE(showTable("grid_composite"));
+	auto [ok, msg] = model->removeRows({IntegerRange<int>(1, 1)});
+	EXPECT_TRUE(ok) << msg.toStdString();
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_composite"), "2");
+	EXPECT_EQ(dbValue("SELECT count(*) FROM grid_composite WHERE a = 1 AND b = 2"), "0");
+}

tests/integration/tst_DataGridMapping.cpp

@@ -0,0 +1,112 @@
+#include <gtest/gtest.h>
+
+#include "catalog/PgClassContainer.h"
+#include "catalog/PgDatabaseCatalog.h"
+
+#include <QCoreApplication>
+#include <QDeadlineTimer>
+#include <QSortFilterProxyModel>
+#include <QEventLoop>
+
+#include "DataGridTest.h"
+
+
+TEST_F(DataGridTest, SingleTableLoadsRows)
+{
+	showQuery("SELECT id, name FROM grid_parent ORDER BY id");
+	EXPECT_EQ(model->rowCount(), 3);
+	ASSERT_NE(widget->proxy(), nullptr);
+	EXPECT_EQ(widget->proxy()->rowCount(), 3);
+	EXPECT_FALSE(model->hasPendingChanges());
+}
+
+TEST_F(DataGridTest, RowNumberColumnIsNeverEditable)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	EXPECT_FALSE(widget->proxy()->flags(widget->proxy()->index(0, 0)) & Qt::ItemIsEditable);
+}
+
+TEST_F(DataGridTest, SingleTableAllColumnsEditable)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true}));
+	EXPECT_EQ(columnName(0), "id");
+	EXPECT_EQ(columnName(1), "name");
+}
+
+TEST_F(DataGridTest, SingleTableSubsetWithPkIsEditable)
+{
+	showQuery("SELECT name, id FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true}));
+	EXPECT_EQ(columnName(0), "name");
+}
+
+TEST_F(DataGridTest, JoinOfTwoTablesWithPksIsEditable)
+{
+	showQuery("SELECT p.id, p.name, c.id AS child_id, c.note FROM grid_parent p JOIN grid_child c ON c.parent_id = p.id");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true, true, true}));
+	EXPECT_EQ(columnName(2), "child_id");
+}
+
+TEST_F(DataGridTest, JoinWithoutOnePkOnlyOtherTableEditable)
+{
+	showQuery("SELECT p.id, p.name, c.note FROM grid_parent p JOIN grid_child c ON c.parent_id = p.id");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true, false}));
+}
+
+TEST_F(DataGridTest, AliasedColumnStaysEditable)
+{
+	showQuery("SELECT id, name AS title FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true}));
+	EXPECT_EQ(columnName(1), "title");
+}
+
+TEST_F(DataGridTest, ExpressionColumnIsReadOnly)
+{
+	showQuery("SELECT id, name, upper(name) AS shout, id + 1 AS next_id FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true, false, false}));
+}
+
+TEST_F(DataGridTest, TableWithoutPkIsReadOnly)
+{
+	showQuery("SELECT x, y FROM grid_nopk");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, false}));
+}
+
+TEST_F(DataGridTest, PkNotSelectedIsReadOnly)
+{
+	showQuery("SELECT name FROM grid_parent");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false}));
+}
+
+TEST_F(DataGridTest, CompositePkFullyPresentIsEditable)
+{
+	showQuery("SELECT a, b, val FROM grid_composite");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true, true}));
+}
+
+TEST_F(DataGridTest, CompositePkPartiallyPresentIsReadOnly)
+{
+	showQuery("SELECT a, val FROM grid_composite");
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{false, false}));
+}
+
+TEST_F(DataGridTest, WholeTableViaSetConfigLoadsAsynchronously)
+{
+	db = openDb();
+	auto cls = db->catalog()->classes()->getByName("grid_parent");
+	ASSERT_NE(cls, nullptr);
+	model = std::make_unique<CrudModel>();
+	widget = std::make_unique<DataGridWidget>();
+	widget->setSourceModel(model.get());
+	model->setConfig(db, *cls);
+	model->loadData();
+
+	QDeadlineTimer deadline(10000);
+	while (model->rowCount() < 4 && !deadline.hasExpired())
+		QCoreApplication::processEvents(QEventLoop::AllEvents, 50);
+
+	// the whole table view appends an empty row for inserting
+	ASSERT_EQ(model->rowCount(), 4);
+	EXPECT_EQ(editableColumns(), (std::vector<bool>{true, true}));
+}

tests/integration/tst_DataGridUpdate.cpp

@@ -0,0 +1,190 @@
+#include "DataGridTest.h"
+
+#include <QAction>
+#include <QApplication>
+#include <QStandardItemModel>
+#include <QTimer>
+#include <QToolBar>
+
+namespace {
+
+/// Failed saves show a modal message box, this closes it so the test can continue
+void autoCloseModalDialogs(int triesLeft = 100)
+{
+	if (auto w = QApplication::activeModalWidget())
+		w->close();
+	else if (triesLeft > 0)
+		QTimer::singleShot(20, [triesLeft] { autoCloseModalDialogs(triesLeft - 1); });
+}
+
+/// Records the values of CrudModel::pendingChangesChanged
+struct PendingSpy {
+	explicit PendingSpy(CrudModel *m)
+	{
+		QObject::connect(m, &CrudModel::pendingChangesChanged, m, [this](bool b) { values.push_back(b); });
+	}
+	std::vector<bool> values;
+};
+
+class DataGridUpdateTest : public DataGridTest {
+protected:
+	void edit(int proxyRow, int resultCol, const QString &value)
+	{
+		auto proxy = widget->proxy();
+		ASSERT_TRUE(proxy->setData(proxy->index(proxyRow, resultCol + 1), value, Qt::EditRole));
+	}
+
+	QAction *action(const QString &text)
+	{
+		for (auto a : widget->findChildren<QAction *>())
+			if (a->text() == text)
+				return a;
+		return nullptr;
+	}
+};
+
+} // namespace
+
+TEST_F(DataGridUpdateTest, EditSingleCellAndSave)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	edit(0, 1, "renamed");
+	EXPECT_TRUE(model->hasPendingChanges());
+	action("Save")->trigger();
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 1"), "renamed");
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 2"), "beta");
+}
+
+TEST_F(DataGridUpdateTest, EditSeveralColumnsAndRows)
+{
+	showQuery("SELECT id, parent_id, note FROM grid_child");
+	edit(0, 1, "3");
+	edit(0, 2, "first");
+	edit(2, 2, "last");
+	action("Save")->trigger();
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(dbValue("SELECT parent_id FROM grid_child WHERE id = 10"), "3");
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 10"), "first");
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 11"), "c2");
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 12"), "last");
+}
+
+TEST_F(DataGridUpdateTest, EmptyValueIsSavedAsNull)
+{
+	showQuery("SELECT id, note FROM grid_child");
+	edit(1, 1, "");
+	action("Save")->trigger();
+
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 11"), std::nullopt);
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 10"), "c1");
+}
+
+TEST_F(DataGridUpdateTest, JoinUpdatesTheOwningTable)
+{
+	showQuery("SELECT p.id, p.name, c.id AS child_id, c.note FROM grid_parent p JOIN grid_child c ON c.parent_id = p.id");
+	edit(0, 1, "parent edited");
+	edit(0, 3, "child edited");
+	action("Save")->trigger();
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 1"), "parent edited");
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 10"), "child edited");
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 2"), "beta");
+	EXPECT_EQ(dbValue("SELECT note FROM grid_child WHERE id = 11"), "c2");
+}
+
+TEST_F(DataGridUpdateTest, CompositePkUpdatesOnlyMatchingRow)
+{
+	showQuery("SELECT a, b, val FROM grid_composite");
+	edit(1, 2, "changed");
+	action("Save")->trigger();
+
+	EXPECT_EQ(dbValue("SELECT val FROM grid_composite WHERE a = 1 AND b = 2"), "changed");
+	EXPECT_EQ(dbValue("SELECT val FROM grid_composite WHERE a = 1 AND b = 1"), "x");
+	EXPECT_EQ(dbValue("SELECT val FROM grid_composite WHERE a = 2 AND b = 1"), "z");
+}
+
+TEST_F(DataGridUpdateTest, EditAfterSortingUpdatesTheShownRow)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	widget->proxy()->sort(2, Qt::DescendingOrder);
+	auto proxy = widget->proxy();
+	ASSERT_EQ(proxy->index(0, 1).data(Qt::DisplayRole).toString(), "3");
+	ASSERT_EQ(proxy->index(0, 2).data(Qt::DisplayRole).toString(), "gamma");
+
+	edit(0, 1, "sorted edit");
+	action("Save")->trigger();
+
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 3"), "sorted edit");
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 1"), "alpha");
+}
+
+TEST_F(DataGridUpdateTest, SaveAndRevertFollowPendingChanges)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	auto save = action("Save");
+	auto revert = action("Revert");
+	ASSERT_NE(save, nullptr);
+	ASSERT_NE(revert, nullptr);
+	EXPECT_FALSE(save->isEnabled());
+	EXPECT_FALSE(revert->isEnabled());
+
+	PendingSpy spy(model.get());
+	edit(0, 1, "x");
+	EXPECT_TRUE(save->isEnabled());
+	EXPECT_TRUE(revert->isEnabled());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true}));
+
+	save->trigger();
+	EXPECT_FALSE(save->isEnabled());
+	EXPECT_FALSE(revert->isEnabled());
+	EXPECT_EQ(spy.values, (std::vector<bool>{true, false}));
+}
+
+TEST_F(DataGridUpdateTest, RevertDiscardsEdits)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	edit(0, 1, "discard me");
+	EXPECT_EQ(widget->proxy()->index(0, 2).data(Qt::EditRole).toString(), "discard me");
+
+	PendingSpy spy(model.get());
+	action("Revert")->trigger();
+
+	EXPECT_FALSE(model->hasPendingChanges());
+	EXPECT_FALSE(action("Save")->isEnabled());
+	EXPECT_EQ(spy.values, (std::vector<bool>{false}));
+	EXPECT_EQ(widget->proxy()->index(0, 2).data(Qt::EditRole).toString(), "alpha");
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 1"), "alpha");
+}
+
+TEST_F(DataGridUpdateTest, FailedSaveKeepsRowPending)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	edit(0, 1, "");	// NULL violates NOT NULL on name
+	QTimer::singleShot(0, [] { autoCloseModalDialogs(); });
+	action("Save")->trigger();
+
+	EXPECT_TRUE(model->hasPendingChanges());
+	EXPECT_TRUE(action("Save")->isEnabled());
+	EXPECT_EQ(dbValue("SELECT name FROM grid_parent WHERE id = 1"), "alpha");
+}
+
+TEST_F(DataGridUpdateTest, ToolbarHiddenForNonCrudModel)
+{
+	showQuery("SELECT id, name FROM grid_parent");
+	auto toolbar = widget->findChild<QToolBar *>();
+	ASSERT_NE(toolbar, nullptr);
+	EXPECT_FALSE(toolbar->isHidden());
+
+	QStandardItemModel other(1, 1);
+	widget->setSourceModel(&other);
+	EXPECT_TRUE(toolbar->isHidden());
+	EXPECT_FALSE(action("Save")->isEnabled());
+
+	widget->setSourceModel(model.get());
+	EXPECT_FALSE(toolbar->isHidden());
+	widget->setSourceModel(nullptr);
+}