From 087a80194a54554ab0510a3c2ccc906a448d36ba Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sun, 22 Sep 2019 21:48:46 +0200 Subject: Import: keep dive and dive site tables in DiveImportedModel The DiveImportedModel and DownloadThread used the same table of dives and dive sites. This made it very hard to keep the model consistent: Every modification of the download thread would make the model inconsistent and could lead to memory corruption owing to dangling pointers. Therefore, keep a copy in the model. When updating the model, use move-semantics, i.e. move the data and reset the tables of the thread to zero elements. Since the DiveImportedModel and the DownloadThread are very tightly integrated, remove the accessor-functions of the dive and dive-site tables. They fulfilled no purpose whatsoever as they gave the same access-rights as a public field. Signed-off-by: Berthold Stoeger --- core/downloadfromdcthread.cpp | 10 -------- core/downloadfromdcthread.h | 6 ++--- qt-models/diveimportedmodel.cpp | 54 ++++++++++++++++++++--------------------- qt-models/diveimportedmodel.h | 6 ++--- 4 files changed, 31 insertions(+), 45 deletions(-) diff --git a/core/downloadfromdcthread.cpp b/core/downloadfromdcthread.cpp index 67d628d31..34f1eb748 100644 --- a/core/downloadfromdcthread.cpp +++ b/core/downloadfromdcthread.cpp @@ -308,16 +308,6 @@ DCDeviceData *DownloadThread::data() return m_data; } -struct dive_table *DownloadThread::table() -{ - return &downloadTable; -} - -struct dive_site_table *DownloadThread::sites() -{ - return &diveSiteTable; -} - QString DCDeviceData::vendor() const { return data.vendor; diff --git a/core/downloadfromdcthread.h b/core/downloadfromdcthread.h index 3a93115db..4284c2cc6 100644 --- a/core/downloadfromdcthread.h +++ b/core/downloadfromdcthread.h @@ -64,13 +64,11 @@ public: void run() override; DCDeviceData *data(); - struct dive_table *table(); - struct dive_site_table *sites(); QString error; - -private: struct dive_table downloadTable; struct dive_site_table diveSiteTable; + +private: DCDeviceData *m_data; }; diff --git a/qt-models/diveimportedmodel.cpp b/qt-models/diveimportedmodel.cpp index 1c9b0cfcd..7e70ce0a7 100644 --- a/qt-models/diveimportedmodel.cpp +++ b/qt-models/diveimportedmodel.cpp @@ -5,8 +5,8 @@ DiveImportedModel::DiveImportedModel(QObject *o) : QAbstractTableModel(o), firstIndex(0), lastIndex(-1), - diveTable(nullptr), - sitesTable(nullptr) + diveTable({ 0 }), + sitesTable({ 0 }) { connect(&thread, &QThread::finished, this, &DiveImportedModel::downloadThreadFinished); } @@ -54,7 +54,7 @@ QVariant DiveImportedModel::data(const QModelIndex &index, int role) const if (index.row() + firstIndex > lastIndex) return QVariant(); - struct dive *d = get_dive_from_table(index.row() + firstIndex, diveTable); + struct dive *d = get_dive_from_table(index.row() + firstIndex, &diveTable); if (!d) return QVariant(); @@ -131,28 +131,26 @@ void DiveImportedModel::clearTable() } void DiveImportedModel::downloadThreadFinished() -{ - repopulate(thread.table(), thread.sites()); - emit downloadFinished(); -} - -void DiveImportedModel::startDownload() -{ - thread.start(); -} - -void DiveImportedModel::repopulate(dive_table_t *table, struct dive_site_table *sites) { beginResetModel(); - diveTable = table; - sitesTable = sites; + // Move the table data from thread to model + move_dive_table(&thread.downloadTable, &diveTable); + move_dive_site_table(&thread.diveSiteTable, &sitesTable); + firstIndex = 0; - lastIndex = diveTable->nr - 1; - checkStates.resize(diveTable->nr); + lastIndex = diveTable.nr - 1; + checkStates.resize(diveTable.nr); std::fill(checkStates.begin(), checkStates.end(), true); endResetModel(); + + emit downloadFinished(); +} + +void DiveImportedModel::startDownload() +{ + thread.start(); } std::pair DiveImportedModel::consumeTables() @@ -160,10 +158,10 @@ std::pair DiveImportedModel::consumeT beginResetModel(); // Move tables to result - struct dive_table dives; - struct dive_site_table sites; - move_dive_table(diveTable, &dives); - move_dive_site_table(sitesTable, &sites); + struct dive_table dives = { 0 }; + struct dive_site_table sites = { 0 }; + move_dive_table(&diveTable, &dives); + move_dive_site_table(&sitesTable, &sites); // Reset indexes firstIndex = 0; @@ -177,38 +175,38 @@ std::pair DiveImportedModel::consumeT int DiveImportedModel::numDives() const { - return diveTable->nr; + return diveTable.nr; } // Delete non-selected dives void DiveImportedModel::deleteDeselected() { - int total = diveTable->nr; + int total = diveTable.nr; int j = 0; for (int i = 0; i < total; i++) { if (checkStates[i]) { j++; } else { beginRemoveRows(QModelIndex(), j, j); - delete_dive_from_table(diveTable, j); + delete_dive_from_table(&diveTable, j); endRemoveRows(); } } - checkStates.resize(diveTable->nr); + checkStates.resize(diveTable.nr); std::fill(checkStates.begin(), checkStates.end(), true); } // Note: this function is only used from mobile - perhaps move it there or unify. void DiveImportedModel::recordDives() { - if (diveTable->nr == 0) + if (diveTable.nr == 0) // nothing to do, just exit return; deleteDeselected(); // TODO: Might want to let the user select IMPORT_ADD_TO_NEW_TRIP - add_imported_dives(diveTable, nullptr, sitesTable, IMPORT_PREFER_IMPORTED | IMPORT_IS_DOWNLOADED); + add_imported_dives(&diveTable, nullptr, &sitesTable, IMPORT_PREFER_IMPORTED | IMPORT_IS_DOWNLOADED); } QHash DiveImportedModel::roleNames() const { diff --git a/qt-models/diveimportedmodel.h b/qt-models/diveimportedmodel.h index 468385f6a..2124daef2 100644 --- a/qt-models/diveimportedmodel.h +++ b/qt-models/diveimportedmodel.h @@ -23,6 +23,7 @@ public: QHash roleNames() const; void deleteDeselected(); std::pair consumeTables(); // Returns dives and sites and resets model. + int numDives() const; Q_INVOKABLE void recordDives(); Q_INVOKABLE void startDownload(); @@ -43,12 +44,11 @@ signals: void downloadFinished(); private: - void repopulate(dive_table_t *table, dive_site_table_t *sites); int firstIndex; int lastIndex; std::vector checkStates; // char instead of bool to avoid silly pessimization of std::vector. - struct dive_table *diveTable; - struct dive_site_table *sitesTable; + struct dive_table diveTable; + struct dive_site_table sitesTable; }; #endif -- cgit v1.2.3-70-g09d2