From fc725d0803a866836ebe595d51a1da566096d599 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sat, 11 May 2024 13:21:53 +0200 Subject: [PATCH] map: use value semantics for MapLocation This makes memory management more simple, as not explicit deletion is necessary. A rather large commit, because changing QVector<> to std::vector<> is propagated up the call chain. Adds a new range_contains() helper function for collection types such as std::vector<>. I didn't want to call it contains(), since we already have a contains function for strings and let's keep argument overloading simple. Signed-off-by: Berthold Stoeger --- backend-shared/exportfuncs.cpp | 4 +- core/divefilter.cpp | 11 ++-- core/divefilter.h | 8 +-- core/range.h | 7 +++ core/subsurface-string.h | 1 + desktop-widgets/divelistview.cpp | 7 +-- desktop-widgets/divelogexportdialog.cpp | 2 +- desktop-widgets/divesitelistview.cpp | 6 +-- desktop-widgets/divesitelistview.h | 2 +- desktop-widgets/locationinformation.cpp | 2 +- desktop-widgets/mapwidget.cpp | 6 +-- desktop-widgets/mapwidget.h | 3 +- map-widget/qmlmapwidgethelper.cpp | 10 ++-- map-widget/qmlmapwidgethelper.h | 2 +- mobile-widgets/qmlmanager.cpp | 4 +- qt-models/maplocationmodel.cpp | 70 ++++++++++++------------- qt-models/maplocationmodel.h | 14 ++--- 17 files changed, 84 insertions(+), 75 deletions(-) diff --git a/backend-shared/exportfuncs.cpp b/backend-shared/exportfuncs.cpp index ff98041d3..bd1479a46 100644 --- a/backend-shared/exportfuncs.cpp +++ b/backend-shared/exportfuncs.cpp @@ -318,9 +318,7 @@ std::vector getDiveSitesToExport(bool selectedOnly) if (selectedOnly && DiveFilter::instance()->diveSiteMode()) { // Special case in dive site mode: export all selected dive sites, // not the dive sites of selected dives. - QVector sites = DiveFilter::instance()->filteredDiveSites(); - res.reserve(sites.size()); - for (const dive_site *ds: sites) + for (auto ds: DiveFilter::instance()->filteredDiveSites()) res.push_back(ds); return res; } diff --git a/core/divefilter.cpp b/core/divefilter.cpp index 3bc1ba9fd..6cca6a12d 100644 --- a/core/divefilter.cpp +++ b/core/divefilter.cpp @@ -5,6 +5,7 @@ #include "divelog.h" #include "gettextfromc.h" #include "qthelper.h" +#include "range.h" #include "selection.h" #include "subsurface-qt/divelistnotifier.h" #if !defined(SUBSURFACE_MOBILE) && !defined(SUBSURFACE_DOWNLOADER) @@ -61,7 +62,7 @@ ShownChange DiveFilter::update(const QVector &dives) const std::vector removeFromSelection; for (dive *d: dives) { // There are three modes: divesite, fulltext, normal - bool newStatus = doDS ? dive_sites.contains(d->dive_site) : + bool newStatus = doDS ? range_contains(dive_sites, d->dive_site) : doFullText ? fulltext_dive_matches(d, filterData.fullText, filterData.fulltextStringMode) && showDive(d) : showDive(d); updateDiveStatus(d, newStatus, res, removeFromSelection); @@ -91,7 +92,7 @@ ShownChange DiveFilter::updateAll() const // There are three modes: divesite, fulltext, normal if (diveSiteMode()) { for_each_dive(i, d) { - bool newStatus = dive_sites.contains(d->dive_site); + bool newStatus = range_contains(dive_sites, d->dive_site); updateDiveStatus(d, newStatus, res, removeFromSelection); } } else if (filterData.fullText.doit()) { @@ -142,7 +143,7 @@ bool DiveFilter::showDive(const struct dive *d) const } #if !defined(SUBSURFACE_MOBILE) && !defined(SUBSURFACE_DOWNLOADER) -void DiveFilter::startFilterDiveSites(QVector ds) +void DiveFilter::startFilterDiveSites(std::vector ds) { if (++diveSiteRefCount > 1) { setFilterDiveSite(std::move(ds)); @@ -169,7 +170,7 @@ void DiveFilter::stopFilterDiveSites() #endif } -void DiveFilter::setFilterDiveSite(QVector ds) +void DiveFilter::setFilterDiveSite(std::vector ds) { // If the filter didn't change, return early to avoid a full // map reload. For a well-defined comparison, sort the vector first. @@ -185,7 +186,7 @@ void DiveFilter::setFilterDiveSite(QVector ds) MainWindow::instance()->diveList->expandAll(); } -const QVector &DiveFilter::filteredDiveSites() const +const std::vector &DiveFilter::filteredDiveSites() const { return dive_sites; } diff --git a/core/divefilter.h b/core/divefilter.h index 403e2871a..076b9249f 100644 --- a/core/divefilter.h +++ b/core/divefilter.h @@ -45,9 +45,9 @@ public: bool diveSiteMode() const; // returns true if we're filtering on dive site (on mobile always returns false) std::vector visibleDives() const; #ifndef SUBSURFACE_MOBILE - const QVector &filteredDiveSites() const; - void startFilterDiveSites(QVector ds); - void setFilterDiveSite(QVector ds); + const std::vector &filteredDiveSites() const; + void startFilterDiveSites(std::vector ds); + void setFilterDiveSite(std::vector ds); void stopFilterDiveSites(); #endif void setFilter(const FilterData &data); @@ -62,7 +62,7 @@ private: void updateDiveStatus(dive *d, bool newStatus, ShownChange &change, std::vector &removeFromSelection) const; - QVector dive_sites; + std::vector dive_sites; FilterData filterData; mutable int shown_dives; diff --git a/core/range.h b/core/range.h index ad38b9dc2..7c7050105 100644 --- a/core/range.h +++ b/core/range.h @@ -98,4 +98,11 @@ int index_of_if(const Range &range, Func f) return it == std::end(range) ? -1 : it - std::begin(range); } +// Not really appropriate here, but oh my. +template +bool range_contains(const Range &v, const Element &item) +{ + return std::find(v.begin(), v.end(), item) != v.end(); +} + #endif diff --git a/core/subsurface-string.h b/core/subsurface-string.h index 80a6f7bab..8750673c2 100644 --- a/core/subsurface-string.h +++ b/core/subsurface-string.h @@ -2,6 +2,7 @@ #ifndef SUBSURFACE_STRING_H #define SUBSURFACE_STRING_H +#include #include #include #include diff --git a/desktop-widgets/divelistview.cpp b/desktop-widgets/divelistview.cpp index 495f6f597..978c86e95 100644 --- a/desktop-widgets/divelistview.cpp +++ b/desktop-widgets/divelistview.cpp @@ -22,6 +22,7 @@ #include "commands/command_base.h" #include "core/errorhelper.h" #include "core/qthelper.h" +#include "core/range.h" #include "core/trip.h" #include "desktop-widgets/divelistview.h" #include "core/metrics.h" @@ -439,13 +440,13 @@ void DiveListView::selectDiveSitesOnMap(const std::vector &dives) // the dive-site selection is controlled by the filter not // by the selected dives. if (!DiveFilter::instance()->diveSiteMode()) { - QVector selectedSites; + std::vector selectedSites; selectedSites.reserve(dives.size()); for (dive *d: dives) { - if (!d->hidden_by_filter && d->dive_site && !selectedSites.contains(d->dive_site)) + if (!d->hidden_by_filter && d->dive_site && !range_contains(selectedSites, d->dive_site)) selectedSites.push_back(d->dive_site); } - MapWidget::instance()->setSelected(selectedSites); + MapWidget::instance()->setSelected(std::move(selectedSites)); } #endif } diff --git a/desktop-widgets/divelogexportdialog.cpp b/desktop-widgets/divelogexportdialog.cpp index befe03bdd..a7cc4b4fb 100644 --- a/desktop-widgets/divelogexportdialog.cpp +++ b/desktop-widgets/divelogexportdialog.cpp @@ -207,7 +207,7 @@ void DiveLogExportDialog::on_buttonBox_accepted() if (!filename.contains('.')) filename.append(".xml"); QByteArray bt = QFile::encodeName(filename); - std::vector sites = getDiveSitesToExport(ui->exportSelected->isChecked()); + auto sites = getDiveSitesToExport(ui->exportSelected->isChecked()); save_dive_sites_logic(bt.data(), sites.data(), (int)sites.size(), ui->anonymize->isChecked()); } } else if (ui->exportImageDepths->isChecked()) { diff --git a/desktop-widgets/divesitelistview.cpp b/desktop-widgets/divesitelistview.cpp index 6ec66cdd3..b1881ee43 100644 --- a/desktop-widgets/divesitelistview.cpp +++ b/desktop-widgets/divesitelistview.cpp @@ -115,14 +115,14 @@ void DiveSiteListView::on_filterText_textChanged(const QString &text) model->setFilter(text); } -QVector DiveSiteListView::selectedDiveSites() +std::vector DiveSiteListView::selectedDiveSites() { const QModelIndexList indices = ui.diveSites->view()->selectionModel()->selectedRows(); - QVector sites; + std::vector sites; sites.reserve(indices.size()); for (const QModelIndex &idx: indices) { struct dive_site *ds = model->getDiveSite(idx); - sites.append(ds); + sites.push_back(ds); } return sites; } diff --git a/desktop-widgets/divesitelistview.h b/desktop-widgets/divesitelistview.h index 3457ac5e3..eeea2a71f 100644 --- a/desktop-widgets/divesitelistview.h +++ b/desktop-widgets/divesitelistview.h @@ -22,7 +22,7 @@ private slots: private: Ui::DiveSiteListView ui; DiveSiteSortedModel *model; - QVector selectedDiveSites(); + std::vector selectedDiveSites(); void hideEvent(QHideEvent *) override; void showEvent(QShowEvent *) override; }; diff --git a/desktop-widgets/locationinformation.cpp b/desktop-widgets/locationinformation.cpp index a04cb4a8e..473baba74 100644 --- a/desktop-widgets/locationinformation.cpp +++ b/desktop-widgets/locationinformation.cpp @@ -260,7 +260,7 @@ void LocationInformationWidget::initFields(dive_site *ds) filter_model.set(ds, ds->location); updateLabels(); enableLocationButtons(dive_site_has_gps_location(ds)); - DiveFilter::instance()->startFilterDiveSites(QVector{ ds }); + DiveFilter::instance()->startFilterDiveSites(std::vector{ ds }); filter_model.invalidate(); } else { filter_model.set(0, location_t()); diff --git a/desktop-widgets/mapwidget.cpp b/desktop-widgets/mapwidget.cpp index 3d8e9b3ca..81fe374b9 100644 --- a/desktop-widgets/mapwidget.cpp +++ b/desktop-widgets/mapwidget.cpp @@ -79,10 +79,10 @@ bool MapWidget::editMode() const return isReady && m_mapHelper->editMode(); } -void MapWidget::setSelected(const QVector &divesites) +void MapWidget::setSelected(std::vector divesites) { CHECK_IS_READY_RETURN_VOID(); - m_mapHelper->setSelected(divesites); + m_mapHelper->setSelected(std::move(divesites)); m_mapHelper->centerOnSelectedDiveSite(); } @@ -97,7 +97,7 @@ void MapWidget::selectedDivesChanged(const QList &list) if (dive *d = get_dive(idx)) selection.push_back(d); } - setSelection(selection, current_dive, -1); + setSelection(std::move(selection), current_dive, -1); } void MapWidget::coordinatesChanged(struct dive_site *ds, const location_t &location) diff --git a/desktop-widgets/mapwidget.h b/desktop-widgets/mapwidget.h index 123b7ad52..09ce7f006 100644 --- a/desktop-widgets/mapwidget.h +++ b/desktop-widgets/mapwidget.h @@ -4,6 +4,7 @@ #include "core/units.h" #include "core/subsurface-qt/divelistnotifier.h" +#include #include #include @@ -23,7 +24,7 @@ public: static MapWidget *instance(); void reload(); - void setSelected(const QVector &divesites); + void setSelected(std::vector divesites); bool editMode() const; public slots: diff --git a/map-widget/qmlmapwidgethelper.cpp b/map-widget/qmlmapwidgethelper.cpp index e5c4ed9b5..e510c28ee 100644 --- a/map-widget/qmlmapwidgethelper.cpp +++ b/map-widget/qmlmapwidgethelper.cpp @@ -46,18 +46,18 @@ void MapWidgetHelper::centerOnDiveSite(struct dive_site *ds) } } -void MapWidgetHelper::setSelected(const QVector &divesites) +void MapWidgetHelper::setSelected(const std::vector divesites) { - m_mapLocationModel->setSelected(divesites); + m_mapLocationModel->setSelected(std::move(divesites)); m_mapLocationModel->selectionChanged(); updateEditMode(); } void MapWidgetHelper::centerOnSelectedDiveSite() { - QVector selDS = m_mapLocationModel->selectedDs(); + std::vector selDS = m_mapLocationModel->selectedDs(); - if (selDS.isEmpty()) { + if (selDS.empty()) { // no selected dives with GPS coordinates QMetaObject::invokeMethod(m_map, "deselectMapLocation"); return; @@ -128,7 +128,7 @@ void MapWidgetHelper::selectedLocationChanged(struct dive_site *ds_in) if (!ds_in) return; - MapLocation *location = m_mapLocationModel->getMapLocation(ds_in); + const MapLocation *location = m_mapLocationModel->getMapLocation(ds_in); if (!location) return; QGeoCoordinate locationCoord = location->coordinate; diff --git a/map-widget/qmlmapwidgethelper.h b/map-widget/qmlmapwidgethelper.h index 9aa1d8723..5a92d4daa 100644 --- a/map-widget/qmlmapwidgethelper.h +++ b/map-widget/qmlmapwidgethelper.h @@ -36,7 +36,7 @@ public: Q_INVOKABLE void updateCurrentDiveSiteCoordinatesFromMap(struct dive_site *ds, QGeoCoordinate coord); Q_INVOKABLE void selectVisibleLocations(); Q_INVOKABLE void selectedLocationChanged(struct dive_site *ds); - void setSelected(const QVector &divesites); + void setSelected(const std::vector divesites); QString pluginObject(); bool editMode() const; diff --git a/mobile-widgets/qmlmanager.cpp b/mobile-widgets/qmlmanager.cpp index 694d53642..e33e61880 100644 --- a/mobile-widgets/qmlmanager.cpp +++ b/mobile-widgets/qmlmanager.cpp @@ -2223,7 +2223,7 @@ void QMLManager::exportToFile(export_types type, QString dir, bool anonymize) break; case EX_DIVE_SITES_XML: { - std::vector sites = getDiveSitesToExport(false); + auto sites = getDiveSitesToExport(false); save_dive_sites_logic(qPrintable(fileName + ".xml"), sites.data(), (int)sites.size(), anonymize); break; } @@ -2271,7 +2271,7 @@ void QMLManager::shareViaEmail(export_types type, bool anonymize) case EX_DIVE_SITES_XML: fileName.replace("subsurface.log", "subsurface_divesites.xml"); { // need a block so the compiler doesn't complain about creating the sites variable here - std::vector sites = getDiveSitesToExport(false); + auto sites = getDiveSitesToExport(false); if (save_dive_sites_logic(qPrintable(fileName), sites.data(), (int)sites.size(), anonymize) == 0) { // ok, we have a file, let's send it body = "Subsurface dive site data"; diff --git a/qt-models/maplocationmodel.cpp b/qt-models/maplocationmodel.cpp index 8534e0d38..9b096ac23 100644 --- a/qt-models/maplocationmodel.cpp +++ b/qt-models/maplocationmodel.cpp @@ -4,11 +4,13 @@ #include "core/divesite.h" #include "core/divefilter.h" #include "core/divelog.h" +#include "core/range.h" #include "core/settings/qPrefDisplay.h" #if !defined(SUBSURFACE_MOBILE) && !defined(SUBSURFACE_DOWNLOADER) #include "qt-models/filtermodels.h" #include "desktop-widgets/mapwidget.h" #endif +#include #define MIN_DISTANCE_BETWEEN_DIVE_SITES_M 50.0 @@ -78,15 +80,14 @@ MapLocationModel::MapLocationModel(QObject *parent) : QAbstractListModel(parent) MapLocationModel::~MapLocationModel() { - qDeleteAll(m_mapLocations); } QVariant MapLocationModel::data(const QModelIndex & index, int role) const { - if (index.row() < 0 || index.row() >= m_mapLocations.size()) + if (index.row() < 0 || index.row() >= (int)m_mapLocations.size()) return QVariant(); - return m_mapLocations.at(index.row())->getRole(role); + return m_mapLocations[index.row()].getRole(role); } QHash MapLocationModel::roleNames() const @@ -103,17 +104,19 @@ QHash MapLocationModel::roleNames() const int MapLocationModel::rowCount(const QModelIndex&) const { - return m_mapLocations.size(); + return (int)m_mapLocations.size(); } +/* UNUSED? void MapLocationModel::add(MapLocation *location) { - beginInsertRows(QModelIndex(), m_mapLocations.size(), m_mapLocations.size()); + beginInsertRows(QModelIndex(), (int)m_mapLocations.size(), (int)m_mapLocations.size()); m_mapLocations.append(location); endInsertRows(); } +*/ -const QVector &MapLocationModel::selectedDs() const +const std::vector &MapLocationModel::selectedDs() const { return m_selectedDs; } @@ -132,22 +135,21 @@ static bool hasSelectedDive(const dive_site &ds) void MapLocationModel::selectionChanged() { - if (m_mapLocations.isEmpty()) + if (m_mapLocations.empty()) return; - for(MapLocation *m: m_mapLocations) - m->selected = m_selectedDs.contains(m->divesite); - emit dataChanged(createIndex(0, 0), createIndex(m_mapLocations.size() - 1, 0)); + for(MapLocation &m: m_mapLocations) + m.selected = range_contains(m_selectedDs, m.divesite); + emit dataChanged(createIndex(0, 0), createIndex((int)m_mapLocations.size() - 1, 0)); } void MapLocationModel::reload(QObject *map) { beginResetModel(); - qDeleteAll(m_mapLocations); m_mapLocations.clear(); m_selectedDs.clear(); - QMap locationNameMap; + std::map locationNameMap; #if defined(SUBSURFACE_MOBILE) || defined(SUBSURFACE_DOWNLOADER) bool diveSiteMode = false; @@ -170,7 +172,7 @@ void MapLocationModel::reload(QObject *map) // Dive sites that do not have a gps location are not shown in normal mode. // In dive-edit mode, selected sites are placed at the center of the map, // so that the user can drag them somewhere without having to enter coordinates. - if (!diveSiteMode || !m_selectedDs.contains(ds.get()) || !map) + if (!diveSiteMode || !range_contains(m_selectedDs, ds.get()) || !map) continue; dsCoord = map->property("center").value(); } else { @@ -178,24 +180,24 @@ void MapLocationModel::reload(QObject *map) qreal longitude = ds->location.lon.udeg * 0.000001; dsCoord = QGeoCoordinate(latitude, longitude); } - if (!diveSiteMode && hasSelectedDive(*ds) && !m_selectedDs.contains(ds.get())) - m_selectedDs.append(ds.get()); + if (!diveSiteMode && hasSelectedDive(*ds) && !range_contains(m_selectedDs, ds.get())) + m_selectedDs.push_back(ds.get()); QString name = siteMapDisplayName(ds->name); if (!diveSiteMode) { // don't add dive locations with the same name, unless they are // at least MIN_DISTANCE_BETWEEN_DIVE_SITES_M apart - if (locationNameMap.contains(name)) { - MapLocation *existingLocation = locationNameMap[name]; - QGeoCoordinate coord = existingLocation->coordinate; + auto it = locationNameMap.find(name); + if (it != locationNameMap.end()) { + const MapLocation &existingLocation = m_mapLocations[it->second]; + QGeoCoordinate coord = existingLocation.coordinate; if (dsCoord.distanceTo(coord) < MIN_DISTANCE_BETWEEN_DIVE_SITES_M) continue; } } - bool selected = m_selectedDs.contains(ds.get()); - MapLocation *location = new MapLocation(ds.get(), dsCoord, name, selected); - m_mapLocations.append(location); + bool selected = range_contains(m_selectedDs, ds.get()); + m_mapLocations.emplace_back(ds.get(), dsCoord, name, selected); if (!diveSiteMode) - locationNameMap[name] = location; + locationNameMap[name] = m_mapLocations.size() - 1; } endResetModel(); @@ -205,19 +207,19 @@ void MapLocationModel::setSelected(struct dive_site *ds) { m_selectedDs.clear(); if (ds) - m_selectedDs.append(ds); + m_selectedDs.push_back(ds); } -void MapLocationModel::setSelected(const QVector &divesites) +void MapLocationModel::setSelected(const std::vector &divesites) { m_selectedDs = divesites; } MapLocation *MapLocationModel::getMapLocation(const struct dive_site *ds) { - for (MapLocation *location: m_mapLocations) { - if (ds == location->divesite) - return location; + for (MapLocation &location: m_mapLocations) { + if (location.divesite == ds) + return &location; } return nullptr; } @@ -225,12 +227,9 @@ MapLocation *MapLocationModel::getMapLocation(const struct dive_site *ds) void MapLocationModel::diveSiteChanged(struct dive_site *ds, int field) { // Find dive site - int row; - for (row = 0; row < m_mapLocations.size(); ++row) { - if (m_mapLocations[row]->divesite == ds) - break; - } - if (row == m_mapLocations.size()) + auto it = std::find_if(m_mapLocations.begin(), m_mapLocations.end(), + [ds](auto &entry) { return entry.divesite == ds; }); + if (it == m_mapLocations.end()) return; switch (field) { @@ -239,15 +238,16 @@ void MapLocationModel::diveSiteChanged(struct dive_site *ds, int field) const qreal latitude_r = ds->location.lat.udeg * 0.000001; const qreal longitude_r = ds->location.lon.udeg * 0.000001; QGeoCoordinate coord(latitude_r, longitude_r); - m_mapLocations[row]->coordinate = coord; + it->coordinate = coord; } break; case LocationInformationModel::NAME: - m_mapLocations[row]->name = siteMapDisplayName(ds->name); + it->name = siteMapDisplayName(ds->name); break; default: break; } + int row = static_cast(it - m_mapLocations.begin()); emit dataChanged(createIndex(row, 0), createIndex(row, 0)); } diff --git a/qt-models/maplocationmodel.h b/qt-models/maplocationmodel.h index 8cc5b7269..83b184676 100644 --- a/qt-models/maplocationmodel.h +++ b/qt-models/maplocationmodel.h @@ -3,8 +3,8 @@ #define MAPLOCATIONMODEL_H #include "core/subsurface-qt/divelistnotifier.h" +#include #include -#include #include #include #include @@ -13,7 +13,7 @@ class MapLocation { public: - explicit MapLocation(struct dive_site *ds, QGeoCoordinate coord, QString name, bool selected); + MapLocation(struct dive_site *ds, QGeoCoordinate coord, QString name, bool selected); QVariant getRole(int role) const; @@ -46,9 +46,9 @@ public: // If map is not null, it will be used to place new dive sites without GPS location at the center of the map void reload(QObject *map); void selectionChanged(); - void setSelected(const QVector &divesites); - MapLocation *getMapLocation(const struct dive_site *ds); - const QVector &selectedDs() const; + void setSelected(const std::vector &divesites); + MapLocation *getMapLocation(const struct dive_site *ds); // Attention: not stable! + const std::vector &selectedDs() const; void setSelected(struct dive_site *ds); protected: @@ -58,8 +58,8 @@ private slots: void diveSiteChanged(struct dive_site *ds, int field); private: - QVector m_mapLocations; - QVector m_selectedDs; + std::vector m_mapLocations; + std::vector m_selectedDs; }; #endif