From c6f9ca8009966b9c4ad917750cd612085a28afed Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sat, 4 May 2024 14:41:04 +0200 Subject: [PATCH] core: replace dive_site::dives by an std::vector<> Since this is now in C++, we don't have to use our crazy TABLE_* macros. This contains a logic change: the dives associated to a dive site are now unsorted. The old code was subtly buggy: dives were added in a sorted manner, but when the dive was edited the list was not resorted. Very unlikely that this leads to a serious problem, still not good. Signed-off-by: Berthold Stoeger --- backend-shared/exportfuncs.cpp | 2 +- commands/command_divesite.cpp | 20 +++++++++---------- core/divelist.cpp | 2 +- core/divesite.cpp | 30 ++++++++++++---------------- core/divesite.h | 6 +++--- core/save-xml.cpp | 2 +- desktop-widgets/divesitelistview.cpp | 4 ++-- desktop-widgets/mainwindow.cpp | 2 +- desktop-widgets/modeldelegates.cpp | 2 +- qt-models/divelocationmodel.cpp | 4 ++-- qt-models/divetripmodel.cpp | 6 +++--- qt-models/maplocationmodel.cpp | 4 ++-- 12 files changed, 39 insertions(+), 45 deletions(-) diff --git a/backend-shared/exportfuncs.cpp b/backend-shared/exportfuncs.cpp index 50cf45e3f..7f5a237f7 100644 --- a/backend-shared/exportfuncs.cpp +++ b/backend-shared/exportfuncs.cpp @@ -330,7 +330,7 @@ std::vector getDiveSitesToExport(bool selectedOnly) struct dive_site *ds = get_dive_site(i, divelog.sites); if (dive_site_is_empty(ds)) continue; - if (selectedOnly && !is_dive_site_selected(ds)) + if (selectedOnly && !is_dive_site_selected(*ds)) continue; res.push_back(ds); } diff --git a/commands/command_divesite.cpp b/commands/command_divesite.cpp index 76f88a175..d942ba277 100644 --- a/commands/command_divesite.cpp +++ b/commands/command_divesite.cpp @@ -23,9 +23,8 @@ static std::vector addDiveSites(std::vector &ds: sites) { // Readd the dives that belonged to this site - for (int i = 0; i < ds->dives.nr; ++i) { + for (dive *d: ds->dives) { // TODO: send dive site changed signal - struct dive *d = ds->dives.dives[i]; d->dive_site = ds.get(); changedDives.push_back(d); } @@ -55,8 +54,7 @@ static std::vector> removeDiveSites(std::vectordives.nr; ++i) { - struct dive *d = ds->dives.dives[i]; + for (dive *d: ds->dives) { d->dive_site = nullptr; changedDives.push_back(d); } @@ -157,7 +155,7 @@ PurgeUnusedDiveSites::PurgeUnusedDiveSites() setText(Command::Base::tr("purge unused dive sites")); for (int i = 0; i < divelog.sites->nr; ++i) { dive_site *ds = divelog.sites->dive_sites[i]; - if (ds->dives.nr == 0) + if (ds->dives.empty()) sitesToRemove.push_back(ds); } } @@ -362,9 +360,9 @@ void MergeDiveSites::redo() // Add them to the merged-into dive site. Thankfully, we remember // the dives in the sitesToAdd vector. for (const std::unique_ptr &site: sitesToAdd) { - for (int i = 0; i < site->dives.nr; ++i) { - add_dive_to_dive_site(site->dives.dives[i], ds); - divesChanged.push_back(site->dives.dives[i]); + for (dive *d: site->dives) { + add_dive_to_dive_site(d, ds); + divesChanged.push_back(d); } } emit diveListNotifier.divesChanged(divesChanged, DiveField::DIVESITE); @@ -378,9 +376,9 @@ void MergeDiveSites::undo() // Before readding the dive sites, unregister the corresponding dives so that they can be // readded to their old dive sites. for (const std::unique_ptr &site: sitesToAdd) { - for (int i = 0; i < site->dives.nr; ++i) { - unregister_dive_from_dive_site(site->dives.dives[i]); - divesChanged.push_back(site->dives.dives[i]); + for (dive *d: site->dives) { + unregister_dive_from_dive_site(d); + divesChanged.push_back(d); } } diff --git a/core/divelist.cpp b/core/divelist.cpp index 74a93564a..b99bbca34 100644 --- a/core/divelist.cpp +++ b/core/divelist.cpp @@ -1150,7 +1150,7 @@ void process_imported_dives(struct divelog *import_log, int flags, if (!old_ds) { /* Dive site doesn't exist. Add it to list of dive sites to be added. */ - new_ds->dives.nr = 0; /* Caller is responsible for adding dives to site */ + new_ds->dives.clear(); /* Caller is responsible for adding dives to site */ add_dive_site_to_table(new_ds, sites_to_add); continue; } diff --git a/core/divesite.cpp b/core/divesite.cpp index e2830991b..e5ba7c2ae 100644 --- a/core/divesite.cpp +++ b/core/divesite.cpp @@ -180,7 +180,6 @@ dive_site::~dive_site() free(name); free(notes); free(description); - free(dives.dives); } /* when parsing, dive sites are identified by uuid */ @@ -199,20 +198,15 @@ struct dive_site *alloc_or_get_dive_site(uint32_t uuid, struct dive_site_table * return ds; } -int nr_of_dives_at_dive_site(struct dive_site *ds) +size_t nr_of_dives_at_dive_site(const dive_site &ds) { - return ds->dives.nr; + return ds.dives.size(); } -bool is_dive_site_selected(struct dive_site *ds) +bool is_dive_site_selected(const struct dive_site &ds) { - int i; - - for (i = 0; i < ds->dives.nr; i++) { - if (ds->dives.dives[i]->selected) - return true; - } - return false; + return any_of(ds.dives.begin(), ds.dives.end(), + [](dive *dive) { return dive->selected; }); } int unregister_dive_site(struct dive_site *ds) @@ -353,7 +347,6 @@ void purge_empty_dive_sites(struct dive_site_table *ds_table) void add_dive_to_dive_site(struct dive *d, struct dive_site *ds) { - int idx; if (!d) { report_info("Warning: add_dive_to_dive_site called with NULL dive"); return; @@ -368,8 +361,7 @@ void add_dive_to_dive_site(struct dive *d, struct dive_site *ds) report_info("Warning: adding dive that already belongs to a dive site to a different site"); unregister_dive_from_dive_site(d); } - idx = dive_table_get_insertion_index(&ds->dives, d); - add_to_dive_table(&ds->dives, idx, d); + ds->dives.push_back(d); d->dive_site = ds; } @@ -377,8 +369,12 @@ struct dive_site *unregister_dive_from_dive_site(struct dive *d) { struct dive_site *ds = d->dive_site; if (!ds) - return NULL; - remove_dive(d, &ds->dives); - d->dive_site = NULL; + return nullptr; + auto it = std::find(ds->dives.begin(), ds->dives.end(), d); + if (it != ds->dives.end()) + ds->dives.erase(it); + else + report_info("Warning: dive not found in divesite table, even though it should be registered there."); + d->dive_site = nullptr; return ds; } diff --git a/core/divesite.h b/core/divesite.h index 237f60085..c02226880 100644 --- a/core/divesite.h +++ b/core/divesite.h @@ -15,7 +15,7 @@ struct dive_site { uint32_t uuid = 0; char *name = nullptr; - struct dive_table dives = { 0, 0, nullptr }; + std::vector dives; location_t location = { { 9 }, { 0 } }; char *description = nullptr; char *notes = nullptr; @@ -50,8 +50,8 @@ void sort_dive_site_table(struct dive_site_table *ds_table); int add_dive_site_to_table(struct dive_site *ds, struct dive_site_table *ds_table); struct dive_site *alloc_or_get_dive_site(uint32_t uuid, struct dive_site_table *ds_table); struct dive_site *alloc_dive_site(); -int nr_of_dives_at_dive_site(struct dive_site *ds); -bool is_dive_site_selected(struct dive_site *ds); +size_t nr_of_dives_at_dive_site(const struct dive_site &ds); +bool is_dive_site_selected(const struct dive_site &ds); int unregister_dive_site(struct dive_site *ds); int register_dive_site(struct dive_site *ds); void delete_dive_site(struct dive_site *ds, struct dive_site_table *ds_table); diff --git a/core/save-xml.cpp b/core/save-xml.cpp index 5b4037875..9a5d76bd5 100644 --- a/core/save-xml.cpp +++ b/core/save-xml.cpp @@ -707,7 +707,7 @@ static void save_dives_buffer(struct membuffer *b, bool select_only, bool anonym if (dive_site_is_empty(ds)) continue; /* Only write used dive sites when exporting selected dives */ - if (select_only && !is_dive_site_selected(ds)) + if (select_only && !is_dive_site_selected(*ds)) continue; put_format(b, "uuid); diff --git a/desktop-widgets/divesitelistview.cpp b/desktop-widgets/divesitelistview.cpp index 65e8c04c9..48b3a9ab3 100644 --- a/desktop-widgets/divesitelistview.cpp +++ b/desktop-widgets/divesitelistview.cpp @@ -62,9 +62,9 @@ void DiveSiteListView::diveSiteClicked(const QModelIndex &index) MainWindow::instance()->editDiveSite(ds); break; case LocationInformationModel::REMOVE: - if (ds->dives.nr > 0 && + if (!ds->dives.empty() && QMessageBox::warning(this, tr("Delete dive site?"), - tr("This dive site has %n dive(s). Do you really want to delete it?\n", "", ds->dives.nr), + tr("This dive site has %n dive(s). Do you really want to delete it?\n", "", ds->dives.size()), QMessageBox::Yes|QMessageBox::No) == QMessageBox::No) return; Command::deleteDiveSites(QVector{ds}); diff --git a/desktop-widgets/mainwindow.cpp b/desktop-widgets/mainwindow.cpp index a38c69b7b..7a5c9a7db 100644 --- a/desktop-widgets/mainwindow.cpp +++ b/desktop-widgets/mainwindow.cpp @@ -1410,7 +1410,7 @@ void MainWindow::on_actionImportDiveSites_triggered() } // The imported dive sites still have pointers to imported dives - remove them for (int i = 0; i < log.sites->nr; ++i) - log.sites->dive_sites[i]->dives.nr = 0; + log.sites->dive_sites[i]->dives.clear(); QString source = fileNames.size() == 1 ? fileNames[0] : tr("multiple files"); diff --git a/desktop-widgets/modeldelegates.cpp b/desktop-widgets/modeldelegates.cpp index d07adedec..c417d275d 100644 --- a/desktop-widgets/modeldelegates.cpp +++ b/desktop-widgets/modeldelegates.cpp @@ -487,7 +487,7 @@ void LocationFilterDelegate::paint(QPainter *painter, const QStyleOptionViewItem } else { int distanceMeters = get_distance(&ds->location, ¤tLocation); QString distance = distance_string(distanceMeters); - int nr = nr_of_dives_at_dive_site(ds); + size_t nr = nr_of_dives_at_dive_site(*ds); bottomText += tr(" (~%1 away").arg(distance); bottomText += tr(", %n dive(s) here)", "", nr); } diff --git a/qt-models/divelocationmodel.cpp b/qt-models/divelocationmodel.cpp index ffa8fe410..da6c12885 100644 --- a/qt-models/divelocationmodel.cpp +++ b/qt-models/divelocationmodel.cpp @@ -92,7 +92,7 @@ QVariant LocationInformationModel::getDiveSiteData(const struct dive_site *ds, i switch(column) { case DIVESITE: return QVariant::fromValue((dive_site *)ds); // Not nice: casting away const case NAME: return QString(ds->name); - case NUM_DIVES: return ds->dives.nr; + case NUM_DIVES: return static_cast(ds->dives.size()); case LOCATION: return "TODO"; case DESCRIPTION: return QString(ds->description); case NOTES: return QString(ds->name); @@ -207,7 +207,7 @@ bool DiveSiteSortedModel::lessThan(const QModelIndex &i1, const QModelIndex &i2) QString::localeAwareCompare(QString(ds1->name), QString(ds2->name)) < 0; // TODO: avoid copy } case LocationInformationModel::NUM_DIVES: { - int cmp = ds1->dives.nr - ds2->dives.nr; + int cmp = static_cast(ds1->dives.size()) - static_cast(ds2->dives.size()); // Since by default nr dives is descending, invert sort direction of names, such that // the names are listed as ascending. return cmp != 0 ? cmp < 0 : diff --git a/qt-models/divetripmodel.cpp b/qt-models/divetripmodel.cpp index 0ddc0e8bb..7a90612e4 100644 --- a/qt-models/divetripmodel.cpp +++ b/qt-models/divetripmodel.cpp @@ -1199,10 +1199,10 @@ void DiveTripModelTree::divesDeletedInternal(dive_trip *trip, bool deleteTrip, c static QVector getDivesForSite(struct dive_site *ds) { QVector diveSiteDives; - diveSiteDives.reserve(ds->dives.nr); + diveSiteDives.reserve(ds->dives.size()); - for (int i = 0; i < ds->dives.nr; ++i) - diveSiteDives.push_back(ds->dives.dives[i]); + for (dive *d: ds->dives) + diveSiteDives.push_back(d); return diveSiteDives; } diff --git a/qt-models/maplocationmodel.cpp b/qt-models/maplocationmodel.cpp index 96ef0d960..3cbfbdcc3 100644 --- a/qt-models/maplocationmodel.cpp +++ b/qt-models/maplocationmodel.cpp @@ -120,13 +120,13 @@ const QVector &MapLocationModel::selectedDs() const static bool hasVisibleDive(const dive_site *ds) { - return std::any_of(&ds->dives.dives[0], &ds->dives.dives[ds->dives.nr], + return std::any_of(ds->dives.begin(), ds->dives.end(), [] (const dive *d) { return !d->hidden_by_filter; }); } static bool hasSelectedDive(const dive_site *ds) { - return std::any_of(&ds->dives.dives[0], &ds->dives.dives[ds->dives.nr], + return std::any_of(ds->dives.begin(), ds->dives.end(), [] (const dive *d) { return d->selected; }); }