From 62c6ad5bfb6d66d9a931a8049414e0d66a85086e Mon Sep 17 00:00:00 2001 From: eelke Date: Sat, 15 Dec 2018 11:24:58 +0100 Subject: [PATCH] Improved support from removing rows in crud tabs. It can handle now complex selections and reports back errors encountered when removing the rows fails. --- core/IntegerRange.h | 136 ++++++++++++++++++++++++++++++++++++ core/core.pro | 3 +- pglab/CrudModel.cpp | 108 +++++++++++++++++----------- pglab/CrudModel.h | 6 +- pglab/CrudTab.cpp | 60 ++++++++++------ pglab/CustomDataRole.h | 15 +++- pgsql/Pgsql_Connection.cpp | 33 +++++++++ pgsql/Pgsql_Connection.h | 1 + pgsql/Pgsql_PgException.cpp | 55 ++++++++++++++- pgsql/Pgsql_PgException.h | 64 +++++------------ 10 files changed, 365 insertions(+), 116 deletions(-) create mode 100644 core/IntegerRange.h diff --git a/core/IntegerRange.h b/core/IntegerRange.h new file mode 100644 index 0000000..e51ae11 --- /dev/null +++ b/core/IntegerRange.h @@ -0,0 +1,136 @@ +#ifndef INTEGERRANGE_H +#define INTEGERRANGE_H + +#include + +template +class IntegerRange { +public: + using Val = Value; + + IntegerRange() = default; + IntegerRange(Value start, Value length) + : m_start(start) + , m_length(length) + {} + + bool canCombine(const IntegerRange &rhs) const + { + if (m_start == rhs.m_start) { + return true; + } + if (m_start < rhs.m_start) { + return m_start + m_length >= rhs.m_start; + } + else { + return rhs.canCombine(*this); + } + } + + void setLength(Value length) + { + m_length = length; + } + + Value length() const { return m_length; } + + void setStart(Value start) + { + m_start = start; + } + Value start() const { return m_start; } + + void setEnd(Value end) + { + m_length = end - m_start; + } + Value end() const { return m_start + m_length; } + + bool operator==(const IntegerRange &rhs) const + { + return m_start == rhs.m_start && m_length == rhs.m_length; + } + + bool operator!=(const IntegerRange &rhs) const + { + return !operator==(rhs); + } + + bool operator<(const IntegerRange &rhs) const + { + return m_start < rhs.m_start; + } + + bool operator<=(const IntegerRange &rhs) const + { + return m_start < rhs.m_start || (m_start == rhs.m_start && m_length < rhs.m_length); + } + + bool operator>(const IntegerRange &rhs) const + { + return m_start > rhs.m_start; + } + + bool operator>=(const IntegerRange &rhs) const + { + return m_start > rhs.m_start || (m_start == rhs.m_start && m_length > rhs.m_length); + } +private: + Value m_start = 0; + Value m_length = 0; +}; + +/// This function merges elements that overlap or touch into single elements +/// and stores the resulting elements into the destination range starting at d_first +/// +/// Function assumes the input range is sorted +template +OutputIt merge_ranges(InputIt first, InputIt last, OutputIt d_first) +{ + using Elem = typename InputIt::value_type; + + InputIt i = first; + while (first != last) { + Elem e = *first; + ++first; + while (first != last && e.canCombine(*first)) { + if (first->end() > e.end()) + e.setEnd(first->end()); + ++first; + } + d_first = e; + } + return d_first; +} + +//template +//void insert(Container cont, RangeType range) +//{ +// auto ipos = std::lower_bound(begin(cont), end(cont), range); + +// // Maybe it can be combined with previous +// if (ipos != begin(cont)) { +// auto prev = ipos; +// --prev; +// if (prev->canCombine(range)) +// prev->setEnd(range->end()); + +// } +// else { +// // We need to either insert it or combine with the next +// if (ipos != end(cont) && range.canCombine(*ipos)) { +// // remember where end of ipos range was +// auto e = ipos->end(); +// if (e > range.end()) +// range.setEnd(e); +// // We cannot modify element in set so we remove one element and add a new one +// ipos = cont.erase(ipos); +// cont.insert(ipos, range); +// } +// else { +// cont->insert(ipos, range); +// } +// } +//} + +#endif // INTEGERRANGE_H diff --git a/core/core.pro b/core/core.pro index 9b02792..a4a7e22 100644 --- a/core/core.pro +++ b/core/core.pro @@ -59,7 +59,8 @@ HEADERS += PasswordManager.h \ SqlAstSelectListEntry.h \ SqlAstSelect.h \ SqlAstExpression.h \ - std_utils.h + std_utils.h \ + IntegerRange.h unix { target.path = /usr/lib diff --git a/pglab/CrudModel.cpp b/pglab/CrudModel.cpp index 41ac1b6..3fc1e74 100644 --- a/pglab/CrudModel.cpp +++ b/pglab/CrudModel.cpp @@ -13,7 +13,9 @@ #include #include #include "Pgsql_oids.h" +#include "Pgsql_PgException.h" #include "Pgsql_Params.h" +#include "Pgsql_Transaction.h" #include #include "ScopeGuard.h" @@ -94,7 +96,6 @@ CrudModel::Value CrudModel::getData(const QModelIndex &index) const int col = index.column(); auto row_mapping = m_rowMapping[grid_row]; - const int last_row = rowCount() - 1; //Oid o = m_roData->type(col); @@ -225,7 +226,7 @@ void CrudModel::connectionStateChanged(ASyncDBConnection::State state) Qt::ItemFlags CrudModel::flags(const QModelIndex &) const { - Qt::ItemFlags flags = Qt::ItemIsSelectable + Qt::ItemIsEnabled; + Qt::ItemFlags flags = Qt::ItemIsSelectable | Qt::ItemIsEnabled; if (m_primaryKey) { flags |= Qt::ItemIsEditable; } @@ -508,16 +509,12 @@ void CrudModel::appendNewRow() endInsertRows(); } -void CrudModel::removeRows() -{ -// determine selection -// remove selected rows -} -bool CrudModel::removeRows(int row, int count, const QModelIndex &parent) +std::tuple CrudModel::removeRows(const std::set> &row_ranges) { - if (m_rowMapping.empty()) return false; + if (row_ranges.empty()) return { true, "" }; + if (row_ranges.rbegin()->end() > m_rowMapping.size()) return { false, "Range error" }; // When removing rows there is no direct mapping anymore between the rows in the grid // and the rows in m_roData @@ -527,42 +524,69 @@ bool CrudModel::removeRows(int row, int count, const QModelIndex &parent) // 1. Get PKEY and remove that row from table - Pgsql::Connection db_update_conn; - auto dbconfig = m_database->config(); - bool res = db_update_conn.connect(dbconfig.getKeywords(), dbconfig.getValues(), false); - if (!res) { - return false; - } - - // First delete rows in table - QString delete_statement = createDeleteStatement(); - db_update_conn.query("BEGIN;"); - for (int current_row = row; current_row < row + count; ++current_row) { - auto&& mapping = m_rowMapping[static_cast(current_row)]; - auto params = getPKeyParamsForRow(mapping.rowKey); - if (!params.empty()) { - // Execute DELETE - auto result = db_update_conn.queryParam(delete_statement, params); + try { + Pgsql::Connection db_update_conn; + auto dbconfig = m_database->config(); + bool res = db_update_conn.connect(dbconfig.getKeywords(), dbconfig.getValues(), false); + if (!res) { + return { false, "Cannot connect to the database" }; } - } - db_update_conn.query("COMMIT;"); - // Then from model - { - beginRemoveRows(parent, row, row); - SCOPE_EXIT { endRemoveRows(); }; - for (int current_row = row; current_row < row + count; ++current_row) { - auto&& mapping = m_rowMapping[static_cast(current_row)]; - // if the row is in modified it can be removed from modified - if (mapping.modified) { - m_modifiedRowList.erase(mapping.rowKey); + // First delete rows in table + QString delete_statement = createDeleteStatement(); + { + db_update_conn.query("BEGIN;"); + 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(current_row)]; + auto params = getPKeyParamsForRow(mapping.rowKey); + if (!params.empty()) { + // Execute DELETE + db_update_conn.queryParam(delete_statement, params); + } + } } - /// \todo can it be pending? should be removed if it is. - + tx.commit(); + // If something goes wrong after this commit we should reload contents of model } - // remove the rows from m_rowMapping - auto first = m_rowMapping.begin() + row; - m_rowMapping.erase(first, first + count); + // Then from model + { + + int rows_deleted = 0; + for (auto range : row_ranges) { + range.setStart(range.start() - rows_deleted); + beginRemoveRows(QModelIndex(), range.start(), range.end() - 1); + SCOPE_EXIT { endRemoveRows(); }; + for (int current_row = range.start(); current_row < range.end(); ++current_row) { + auto&& mapping = m_rowMapping[static_cast(current_row)]; + + // if the row is in modified it can be removed from modified + if (mapping.modified) { + m_modifiedRowList.erase(mapping.rowKey); + } + /// \todo can it be pending? should be removed if it is. + + } + // remove the rows from m_rowMapping + auto first = m_rowMapping.begin() + range.start(); + m_rowMapping.erase(first, first + range.length()); + rows_deleted += range.length(); + } + } + return { true, "" }; + } catch (const Pgsql::PgResultError &error) { + return { false, QString::fromUtf8(error.details().messageDetail.c_str()) }; } - return true; } + +bool CrudModel::removeRows(int row, int count, const QModelIndex &) +{ + if (m_rowMapping.empty()) return false; + + IntegerRange range(row, count); + auto [res, message] = removeRows({ range }); + return res; +} + + diff --git a/pglab/CrudModel.h b/pglab/CrudModel.h index 7b904d0..707decf 100644 --- a/pglab/CrudModel.h +++ b/pglab/CrudModel.h @@ -5,11 +5,13 @@ #include "ASyncDBConnection.h" #include "Pgsql_Connection.h" +#include "IntegerRange.h" #include "PgClass.h" #include "PgConstraint.h" #include "Pgsql_Connection.h" #include #include +#include #include #include #include @@ -73,9 +75,9 @@ public: void loadData(); + std::tuple removeRows(const std::set> &row_ranges); bool removeRows(int row, int count, const QModelIndex &parent = QModelIndex()) override; - /// Removes selected rows - void removeRows(); + public slots: virtual bool submit() override; virtual void revert() override; diff --git a/pglab/CrudTab.cpp b/pglab/CrudTab.cpp index 75e7802..0cfe208 100644 --- a/pglab/CrudTab.cpp +++ b/pglab/CrudTab.cpp @@ -4,7 +4,12 @@ #include "MainWindow.h" #include "ResultTableModelUtil.h" #include "PgLabItemDelegate.h" +#include "IntegerRange.h" +#include #include +#include +#include +#include CrudTab::CrudTab(MainWindow *parent) @@ -22,7 +27,8 @@ CrudTab::CrudTab(MainWindow *parent) m_crudModel = new CrudModel(parent); ui->tableView->setModel(m_crudModel); - ui->tableView->setSelectionMode(QAbstractItemView::SingleSelection); + ui->tableView->setSelectionMode(QAbstractItemView::ExtendedSelection); + ui->tableView->setSelectionBehavior(QAbstractItemView::SelectRows); ui->tableView->addAction(ui->actionRemove_rows); auto horizontal_header = ui->tableView->horizontalHeader(); @@ -30,9 +36,6 @@ CrudTab::CrudTab(MainWindow *parent) connect(horizontal_header, &QHeaderView::customContextMenuRequested, this, &CrudTab::headerCustomContextMenu); - //auto selection_model = ui->tableView->selectionModel(); -// connect(ui->tableView->selectionModel(), &QItemSelectionModel::currentRowChanged, this, -// &CrudTab::tableView_currentRowChanged); } CrudTab::~CrudTab() @@ -44,21 +47,9 @@ void CrudTab::setConfig(std::shared_ptr db, const PgClass &table) { m_db = db; m_table = table; -// m_catalog = cat; -// m_tablesModel->setCatalog(cat); -// ui->tableListTable->resizeColumnsToContents(); -// m_namespaceFilterWidget->init(cat->namespaces()); - -// auto highlighter = new SqlSyntaxHighlighter(ui->constraintSqlEdit->document()); -// highlighter->setTypes(*cat->types()); m_crudModel->setConfig(db, table); } -//void CrudTab::tableView_currentRowChanged(const QModelIndex ¤t, const QModelIndex &previous) -//{ - -//} - void CrudTab::refresh() { m_crudModel->loadData(); @@ -66,14 +57,37 @@ void CrudTab::refresh() void CrudTab::on_actionRemove_rows_triggered() { - // determine selection + std::set> row_ranges; + auto selection = ui->tableView->selectionModel()->selection(); + for (auto range : selection) { + row_ranges.emplace(range.top(), range.height()); + } + std::set> merged_ranges; + merge_ranges(row_ranges.begin(), row_ranges.end(), std::inserter(merged_ranges, merged_ranges.begin())); -// ui->tableView->currentIndex() -// selectionModel()-> + QString msg = tr("Are you certain you want to remove the following row(s)?"); + msg += "\n"; + bool first = true; + for (auto range : merged_ranges) { + if (first) first = false; + else msg += ", "; - //ui->tableView->selectionModel()->selectedRows() - m_crudModel->removeRow(ui->tableView->currentIndex().row()); - //removeRows(); + auto s = range.start() + 1, e = range.end(); + if (s == e) + msg += QString("%1").arg(s); + else + msg += QString("%1 through %2").arg(s).arg(e); + + msg += " "; + } + auto res = QMessageBox::question(this, "pgLab", msg, QMessageBox::Yes, QMessageBox::No); + + if (res == QMessageBox::Yes) { + auto [res, msg] = m_crudModel->removeRows(merged_ranges); + if (!res) { + QMessageBox::critical(this, "pgLab", msg, QMessageBox::Close); + } + } } std::vector CrudTab::getToolbarActions() @@ -83,6 +97,8 @@ std::vector CrudTab::getToolbarActions() action->setShortcut(QKeySequence(Qt::Key_F5)); connect(action, &QAction::triggered, this, &CrudTab::refresh); actions.push_back(action); + + actions.push_back(ui->actionRemove_rows); } return actions; } diff --git a/pglab/CustomDataRole.h b/pglab/CustomDataRole.h index 590f08a..849b678 100644 --- a/pglab/CustomDataRole.h +++ b/pglab/CustomDataRole.h @@ -3,10 +3,21 @@ #include +/// Returned by a model when asked for CustomReferencedTypeRole +/// +/// Models will probably only be asked this for columns for which they returned +/// Oid_Oid for the CustomDataTypeRole +enum class ReferencedType { + PgType, + PgNamespace, + PgRole +}; + enum CustomDataRole { - CustomDataTypeRole = Qt::UserRole, + CustomDataTypeRole = Qt::UserRole, ///< Requist the basic type of the value + CustomReferencedTypeRole, ///< // Add other enum before this one is we might want to have multiple hidden values - FirstHiddenValue, + FirstHiddenValue, ///< Used to request value from a model which is not handed to the view }; #endif // CUSTOMDATAROLE_H diff --git a/pgsql/Pgsql_Connection.cpp b/pgsql/Pgsql_Connection.cpp index fef9b64..58563d8 100644 --- a/pgsql/Pgsql_Connection.cpp +++ b/pgsql/Pgsql_Connection.cpp @@ -1,5 +1,6 @@ #include "Pgsql_Connection.h" #include "Pgsql_declare.h" +#include "Pgsql_PgException.h" #include "Pgsql_Params.h" #include #include @@ -107,6 +108,7 @@ std::string Connection::getErrorMessage() const Result Connection::query(const char * command) { PGresult *result = PQexec(conn, command); + throwError(result); return Result(result); } @@ -114,6 +116,7 @@ Result Connection::queryParam(const char * command, const Params ¶ms) { PGresult *result = PQexecParams(conn, command, params.size(), params.types(), params.values(), params.lengths(), params.formats(), 0); + throwError(result); return Result(result); } @@ -142,6 +145,7 @@ std::shared_ptr Connection::getResult() { PGresult *r = PQgetResult(conn); if (r) { + throwError(r); return std::make_shared(r); } else { @@ -225,3 +229,32 @@ QString Connection::getDBName() const { return QString::fromUtf8(PQdb(conn)); } + +void Connection::throwError(PGresult *result) const +{ + auto state = PQresultStatus(result); + if (state == PGRES_BAD_RESPONSE) { + // communication problem + } + else if (state == PGRES_FATAL_ERROR) { + auto details = Pgsql::ErrorDetails::createErrorDetailsFromPGresult(result); + throw PgResultError(details); + } +} + +//PGRES_EMPTY_QUERY = 0, /* empty query string was executed */ +//PGRES_COMMAND_OK, /* a query command that doesn't return +// * anything was executed properly by the +// * backend */ +//PGRES_TUPLES_OK, /* a query command that returns tuples was +// * executed properly by the backend, PGresult +// * contains the result tuples */ +//PGRES_COPY_OUT, /* Copy Out data transfer in progress */ +//PGRES_COPY_IN, /* Copy In data transfer in progress */ +//PGRES_BAD_RESPONSE, /* an unexpected response was recv'd from the +// * backend */ +//PGRES_NONFATAL_ERROR, /* notice or warning message */ +//PGRES_FATAL_ERROR, /* query failed */ +//PGRES_COPY_BOTH, /* Copy In/Out data transfer in progress */ +//PGRES_SINGLE_TUPLE /* single tuple from larger resultset */ + diff --git a/pgsql/Pgsql_Connection.h b/pgsql/Pgsql_Connection.h index 4f564ac..a8dd048 100644 --- a/pgsql/Pgsql_Connection.h +++ b/pgsql/Pgsql_Connection.h @@ -114,6 +114,7 @@ namespace Pgsql { PGconn *conn = nullptr; std::function notifyReceiver; + void throwError(PGresult *result) const; static void notifyReceiveFunc(void *arg, const PGresult *result); }; diff --git a/pgsql/Pgsql_PgException.cpp b/pgsql/Pgsql_PgException.cpp index 381daae..40a65e6 100644 --- a/pgsql/Pgsql_PgException.cpp +++ b/pgsql/Pgsql_PgException.cpp @@ -1 +1,54 @@ -#include "Pgsql_PgException.h" +#include "Pgsql_PgException.h" + +namespace Pgsql { + + ResultCode::ResultCode(std::string result_code) + : m_resultCode(std::move(result_code)) + { + assert(m_resultCode.length() == 5); + } + + std::string ResultCode::getClass() const + { + return m_resultCode.substr(1,2); + } + + /** Helper to easily check the class of the error + * + */ + bool ResultCode::isClass(const std::string_view cls) + { + return m_resultCode.compare(1, 2, cls); + } + + const std::string& ResultCode::getSpecific() const + { + return m_resultCode; + } + + + PgException::PgException(const char *msg) + : std::runtime_error(msg) + {} + + PgException::PgException(std::string msg) + : std::runtime_error(msg) + {} + + + PgResultError::PgResultError(const Pgsql::ErrorDetails &details) + : PgException(details.errorMessage) + , m_details(details) + {} + + ResultCode PgResultError::getResultCode() const + { + return ResultCode(m_details.state); + } + + const Pgsql::ErrorDetails& PgResultError::details() const + { + return m_details; + } + +} diff --git a/pgsql/Pgsql_PgException.h b/pgsql/Pgsql_PgException.h index e8a8f9f..685a4ab 100644 --- a/pgsql/Pgsql_PgException.h +++ b/pgsql/Pgsql_PgException.h @@ -1,6 +1,7 @@ #ifndef PGEXCEPTION_H #define PGEXCEPTION_H +#include "Pgsql_ErrorDetails.h" #include #include #include @@ -9,68 +10,39 @@ namespace Pgsql { class ResultCode { public: - explicit ResultCode(std::string result_code) - : m_resultCode(std::move(result_code)) - { - assert(m_resultCode.length() == 5); - } - - std::string getClass() const - { - return m_resultCode.substr(1,2); - } - - /** Helper to easily check the class of the error - * - */ - bool isClass(const std::string_view cls) - { - return m_resultCode.compare(1, 2, cls); - } - - const std::string& getSpecific() const - { - return m_resultCode; - } + explicit ResultCode(std::string result_code); + std::string getClass() const; + bool isClass(const std::string_view cls); + const std::string& getSpecific() const; private: std::string m_resultCode; }; class PgException: public std::runtime_error { public: - PgException(const char *msg) - : std::runtime_error(msg) - {} - PgException(std::string msg) - : std::runtime_error(msg) - {} - - + PgException(const char *msg); + PgException(std::string msg); private: - }; class PgResultError: public PgException { public: - PgResultError(std::string msg, std::string result_code) - : PgException(std::move(msg)) - , m_resultCode(result_code) - {} - - ResultCode getResultCode() const { return m_resultCode; } + PgResultError(const Pgsql::ErrorDetails &details); + ResultCode getResultCode() const; + const Pgsql::ErrorDetails& details() const; private: - ResultCode m_resultCode; + Pgsql::ErrorDetails m_details; }; - class PgConnectionError: public PgResultError { - public: - PgConnectionError(const std::string &msg, std::string result_code) - : PgResultError(msg, std::move(result_code)) - {} +// class PgConnectionError: public PgException { +// public: +// PgConnectionError(const std::string &msg, std::string result_code) +// : PgResultError(msg, std::move(result_code)) +// {} - private: +// private: - }; +// };