From 7a070b3e7f7227aeb8626a9254e561192c21c3fd Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Sun, 3 Nov 2024 14:16:56 -0500 Subject: [PATCH] Move MarkerManager to using an ID system for markers, instead of index --- .../source/scwx/qt/manager/marker_manager.cpp | 85 +++++++++++++++++-- .../source/scwx/qt/manager/marker_manager.hpp | 16 ++-- scwx-qt/source/scwx/qt/map/marker_layer.cpp | 16 ++-- scwx-qt/source/scwx/qt/model/marker_model.cpp | 66 +++++++++++--- scwx-qt/source/scwx/qt/model/marker_model.hpp | 8 +- scwx-qt/source/scwx/qt/types/marker_types.hpp | 3 + .../scwx/qt/ui/marker_settings_widget.cpp | 32 ++++--- .../scwx/qt/model/marker_model.test.cpp | 81 +++++++++++++----- 8 files changed, 233 insertions(+), 74 deletions(-) diff --git a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp index c8ceda09..e91f03fb 100644 --- a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp +++ b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp @@ -38,6 +38,8 @@ public: std::string markerSettingsPath_ {""}; std::vector> markerRecords_ {}; + std::unordered_map idToIndex_ {}; + MarkerManager* self_; @@ -48,6 +50,10 @@ public: void ReadMarkerSettings(); void WriteMarkerSettings(); std::shared_ptr GetMarkerByName(const std::string& name); + + void InitalizeIds(); + types::MarkerId NewId(); + types::MarkerId lastId_; }; class MarkerManager::Impl::MarkerRecord @@ -88,6 +94,16 @@ public: } }; +void MarkerManager::Impl::InitalizeIds() +{ + lastId_ = 0; +} + +types::MarkerId MarkerManager::Impl::NewId() +{ + return ++lastId_; +} + void MarkerManager::Impl::InitializeMarkerSettings() { std::string appDataPath { @@ -109,6 +125,7 @@ void MarkerManager::Impl::InitializeMarkerSettings() void MarkerManager::Impl::ReadMarkerSettings() { logger_->info("Reading location marker settings"); + InitalizeIds(); boost::json::value markerJson = nullptr; { @@ -125,6 +142,7 @@ void MarkerManager::Impl::ReadMarkerSettings() // For each marker entry auto& markerArray = markerJson.as_array(); markerRecords_.reserve(markerArray.size()); + idToIndex_.reserve(markerArray.size()); for (auto& markerEntry : markerArray) { try @@ -134,8 +152,12 @@ void MarkerManager::Impl::ReadMarkerSettings() if (!record.markerInfo_.name.empty()) { + types::MarkerId id = NewId(); + size_t index = markerRecords_.size(); + record.markerInfo_.id = id; markerRecords_.emplace_back( std::make_shared(record.markerInfo_)); + idToIndex_.emplace(id, index); } } catch (const std::exception& ex) @@ -206,11 +228,17 @@ size_t MarkerManager::marker_count() return p->markerRecords_.size(); } -std::optional MarkerManager::get_marker(size_t index) +std::optional MarkerManager::get_marker(types::MarkerId id) { std::shared_lock lock(p->markerRecordLock_); + if (!p->idToIndex_.contains(id)) + { + return {}; + } + size_t index = p->idToIndex_[id]; if (index >= p->markerRecords_.size()) { + logger_->warn("id in idToIndex_ but out of range!"); return {}; } std::shared_ptr& markerRecord = @@ -218,45 +246,81 @@ std::optional MarkerManager::get_marker(size_t index) return markerRecord->toMarkerInfo(); } -void MarkerManager::set_marker(size_t index, const types::MarkerInfo& marker) +std::optional MarkerManager::get_index(types::MarkerId id) +{ + std::shared_lock lock(p->markerRecordLock_); + if (!p->idToIndex_.contains(id)) + { + return {}; + } + return p->idToIndex_[id]; +} + +void MarkerManager::set_marker(types::MarkerId id, const types::MarkerInfo& marker) { { std::unique_lock lock(p->markerRecordLock_); + if (!p->idToIndex_.contains(id)) + { + return; + } + size_t index = p->idToIndex_[id]; if (index >= p->markerRecords_.size()) { + logger_->warn("id in idToIndex_ but out of range!"); return; } std::shared_ptr& markerRecord = p->markerRecords_[index]; markerRecord->markerInfo_ = marker; } - Q_EMIT MarkerChanged(index); + Q_EMIT MarkerChanged(id); Q_EMIT MarkersUpdated(); } void MarkerManager::add_marker(const types::MarkerInfo& marker) { + types::MarkerId id; { std::unique_lock lock(p->markerRecordLock_); + id = p->NewId(); + size_t index = p->markerRecords_.size(); + p->idToIndex_.emplace(id, index); p->markerRecords_.emplace_back(std::make_shared(marker)); + p->markerRecords_[index]->markerInfo_.id = id; } - Q_EMIT MarkerAdded(); + Q_EMIT MarkerAdded(id); Q_EMIT MarkersUpdated(); } -void MarkerManager::remove_marker(size_t index) +void MarkerManager::remove_marker(types::MarkerId id) { { std::unique_lock lock(p->markerRecordLock_); + if (!p->idToIndex_.contains(id)) + { + return; + } + size_t index = p->idToIndex_[id]; if (index >= p->markerRecords_.size()) { + logger_->warn("id in idToIndex_ but out of range!"); return; } p->markerRecords_.erase(std::next(p->markerRecords_.begin(), index)); + p->idToIndex_.erase(id); + + for (auto& pair : p->idToIndex_) + { + if (pair.second > index) + { + p->idToIndex_[pair.first] = pair.second - 1; + } + } } - Q_EMIT MarkerRemoved(index); + Q_EMIT MarkerRemoved(id); Q_EMIT MarkersUpdated(); } @@ -292,6 +356,15 @@ void MarkerManager::move_marker(size_t from, size_t to) Q_EMIT MarkersUpdated(); } +void MarkerManager::for_each(std::function func) +{ + std::shared_lock lock(p->markerRecordLock_); + for (auto marker : p->markerRecords_) + { + func(marker->markerInfo_); + } +} + // Only use for testing void MarkerManager::set_marker_settings_path(const std::string& path) { diff --git a/scwx-qt/source/scwx/qt/manager/marker_manager.hpp b/scwx-qt/source/scwx/qt/manager/marker_manager.hpp index 2166725f..37ec1c31 100644 --- a/scwx-qt/source/scwx/qt/manager/marker_manager.hpp +++ b/scwx-qt/source/scwx/qt/manager/marker_manager.hpp @@ -12,6 +12,7 @@ namespace qt namespace manager { +typedef void MarkerForEachFunc(const types::MarkerInfo&); class MarkerManager : public QObject { Q_OBJECT @@ -21,12 +22,15 @@ public: ~MarkerManager(); size_t marker_count(); - std::optional get_marker(size_t index); - void set_marker(size_t index, const types::MarkerInfo& marker); + std::optional get_marker(types::MarkerId id); + std::optional get_index(types::MarkerId id); + void set_marker(types::MarkerId id, const types::MarkerInfo& marker); void add_marker(const types::MarkerInfo& marker); - void remove_marker(size_t index); + void remove_marker(types::MarkerId id); void move_marker(size_t from, size_t to); + void for_each(std::function func); + // Only use for testing void set_marker_settings_path(const std::string& path); @@ -35,9 +39,9 @@ public: signals: void MarkersInitialized(size_t count); void MarkersUpdated(); - void MarkerChanged(size_t index); - void MarkerAdded(); - void MarkerRemoved(size_t index); + void MarkerChanged(types::MarkerId id); + void MarkerAdded(types::MarkerId id); + void MarkerRemoved(types::MarkerId id); private: class Impl; diff --git a/scwx-qt/source/scwx/qt/map/marker_layer.cpp b/scwx-qt/source/scwx/qt/map/marker_layer.cpp index ab97322f..0480a63e 100644 --- a/scwx-qt/source/scwx/qt/map/marker_layer.cpp +++ b/scwx-qt/source/scwx/qt/map/marker_layer.cpp @@ -55,17 +55,13 @@ void MarkerLayer::Impl::ReloadMarkers() geoIcons_->StartIcons(); - for (size_t i = 0; i < markerManager->marker_count(); i++) - { - std::optional marker = markerManager->get_marker(i); - if (!marker) + markerManager->for_each( + [this](const types::MarkerInfo& marker) { - break; - } - std::shared_ptr icon = geoIcons_->AddIcon(); - geoIcons_->SetIconTexture(icon, markerIconName_, 0); - geoIcons_->SetIconLocation(icon, marker->latitude, marker->longitude); - } + std::shared_ptr icon = geoIcons_->AddIcon(); + geoIcons_->SetIconTexture(icon, markerIconName_, 0); + geoIcons_->SetIconLocation(icon, marker.latitude, marker.longitude); + }); geoIcons_->FinishIcons(); Q_EMIT self_->NeedsRendering(); diff --git a/scwx-qt/source/scwx/qt/model/marker_model.cpp b/scwx-qt/source/scwx/qt/model/marker_model.cpp index 8bcd93fa..97cafcb1 100644 --- a/scwx-qt/source/scwx/qt/model/marker_model.cpp +++ b/scwx-qt/source/scwx/qt/model/marker_model.cpp @@ -5,6 +5,8 @@ #include #include +#include + #include namespace scwx @@ -30,6 +32,7 @@ public: ~Impl() = default; std::shared_ptr markerManager_ { manager::MarkerManager::Instance()}; + std::vector markerIds_; }; MarkerModel::MarkerModel(QObject* parent) : @@ -63,7 +66,7 @@ int MarkerModel::rowCount(const QModelIndex& parent) const { return parent.isValid() ? 0 : - static_cast(p->markerManager_->marker_count()); + static_cast(p->markerIds_.size()); } int MarkerModel::columnCount(const QModelIndex& parent) const @@ -95,15 +98,19 @@ QVariant MarkerModel::data(const QModelIndex& index, int role) const static const char COORDINATE_FORMAT = 'g'; static const int COORDINATE_PRECISION = 10; - if (!index.isValid() || index.row() < 0) + if (!index.isValid() || index.row() < 0 || + static_cast(index.row()) >= p->markerIds_.size()) { + logger_->debug("Failed to get data index {}", index.row()); return QVariant(); } + types::MarkerId id = p->markerIds_[index.row()]; std::optional markerInfo = - p->markerManager_->get_marker(index.row()); + p->markerManager_->get_marker(id); if (!markerInfo) { + logger_->debug("Failed to get data index {} id {}", index.row(), id); return QVariant(); } @@ -154,6 +161,16 @@ QVariant MarkerModel::data(const QModelIndex& index, int role) const return QVariant(); } +std::optional MarkerModel::getId(int index) +{ + if (index < 0 || static_cast(index) >= p->markerIds_.size()) + { + return {}; + } + + return p->markerIds_[index]; +} + QVariant MarkerModel::headerData(int section, Qt::Orientation orientation, int role) const @@ -186,12 +203,16 @@ bool MarkerModel::setData(const QModelIndex& index, const QVariant& value, int role) { - if (!index.isValid() || index.row() < 0) + + if (!index.isValid() || index.row() < 0 || + static_cast(index.row()) >= p->markerIds_.size()) { return false; } + + types::MarkerId id = p->markerIds_[index.row()]; std::optional markerInfo = - p->markerManager_->get_marker(index.row()); + p->markerManager_->get_marker(id); if (!markerInfo) { return false; @@ -205,7 +226,7 @@ bool MarkerModel::setData(const QModelIndex& index, { QString str = value.toString(); markerInfo->name = str.toStdString(); - p->markerManager_->set_marker(index.row(), *markerInfo); + p->markerManager_->set_marker(id, *markerInfo); result = true; } break; @@ -219,7 +240,7 @@ bool MarkerModel::setData(const QModelIndex& index, if (!str.isEmpty() && ok && -90 <= latitude && latitude <= 90) { markerInfo->latitude = latitude; - p->markerManager_->set_marker(index.row(), *markerInfo); + p->markerManager_->set_marker(id, *markerInfo); result = true; } } @@ -234,7 +255,7 @@ bool MarkerModel::setData(const QModelIndex& index, if (!str.isEmpty() && ok && -180 <= longitude && longitude <= 180) { markerInfo->longitude = longitude; - p->markerManager_->set_marker(index.row(), *markerInfo); + p->markerManager_->set_marker(id, *markerInfo); result = true; } } @@ -260,32 +281,49 @@ void MarkerModel::HandleMarkersInitialized(size_t count) } const int index = static_cast(count - 1); + p->markerIds_.reserve(count); beginInsertRows(QModelIndex(), 0, index); + p->markerManager_->for_each( + [this](const types::MarkerInfo& info) + { + p->markerIds_.push_back(info.id); + }); endInsertRows(); } -void MarkerModel::HandleMarkerAdded() +void MarkerModel::HandleMarkerAdded(types::MarkerId id) { - const int newIndex = static_cast(p->markerManager_->marker_count() - 1); + std::optional index = p->markerManager_->get_index(id); + const int newIndex = static_cast(*index); beginInsertRows(QModelIndex(), newIndex, newIndex); + p->markerIds_.emplace_back(id); endInsertRows(); } -void MarkerModel::HandleMarkerChanged(size_t index) +void MarkerModel::HandleMarkerChanged(types::MarkerId id) { - const int changedIndex = static_cast(index); + std::optional index = p->markerManager_->get_index(id); + const int changedIndex = static_cast(*index); + QModelIndex topLeft = createIndex(changedIndex, kFirstColumn); QModelIndex bottomRight = createIndex(changedIndex, kLastColumn); Q_EMIT dataChanged(topLeft, bottomRight); } -void MarkerModel::HandleMarkerRemoved(size_t index) +void MarkerModel::HandleMarkerRemoved(types::MarkerId id) { - const int removedIndex = static_cast(index); + auto it = std::find(p->markerIds_.begin(), p->markerIds_.end(), id); + if (it == p->markerIds_.end()) + { + return; + } + + const int removedIndex = std::distance(p->markerIds_.begin(), it); beginRemoveRows(QModelIndex(), removedIndex, removedIndex); + p->markerIds_.erase(it); endRemoveRows(); } diff --git a/scwx-qt/source/scwx/qt/model/marker_model.hpp b/scwx-qt/source/scwx/qt/model/marker_model.hpp index 85112fa1..4fc6c95c 100644 --- a/scwx-qt/source/scwx/qt/model/marker_model.hpp +++ b/scwx-qt/source/scwx/qt/model/marker_model.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include namespace scwx { @@ -37,12 +38,13 @@ public: const QVariant& value, int role = Qt::EditRole) override; + std::optional getId(int index); public slots: void HandleMarkersInitialized(size_t count); - void HandleMarkerAdded(); - void HandleMarkerChanged(size_t index); - void HandleMarkerRemoved(size_t index); + void HandleMarkerAdded(types::MarkerId id); + void HandleMarkerChanged(types::MarkerId id); + void HandleMarkerRemoved(types::MarkerId id); private: class Impl; diff --git a/scwx-qt/source/scwx/qt/types/marker_types.hpp b/scwx-qt/source/scwx/qt/types/marker_types.hpp index 0d9c575b..e3d28e26 100644 --- a/scwx-qt/source/scwx/qt/types/marker_types.hpp +++ b/scwx-qt/source/scwx/qt/types/marker_types.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include namespace scwx { @@ -8,6 +9,7 @@ namespace qt { namespace types { +typedef std::uint64_t MarkerId; struct MarkerInfo { @@ -16,6 +18,7 @@ struct MarkerInfo { } + MarkerId id; std::string name; double latitude; double longitude; diff --git a/scwx-qt/source/scwx/qt/ui/marker_settings_widget.cpp b/scwx-qt/source/scwx/qt/ui/marker_settings_widget.cpp index 8fc1fe6a..0c2bc614 100644 --- a/scwx-qt/source/scwx/qt/ui/marker_settings_widget.cpp +++ b/scwx-qt/source/scwx/qt/ui/marker_settings_widget.cpp @@ -65,21 +65,25 @@ void MarkerSettingsWidgetImpl::ConnectSignals() { markerManager_->add_marker(types::MarkerInfo("", 0, 0)); }); - QObject::connect(self_->ui->removeButton, - &QPushButton::clicked, - self_, - [this]() - { - auto selectionModel = - self_->ui->markerView->selectionModel(); - QModelIndex selected = - selectionModel - ->selectedRows(static_cast( - model::MarkerModel::Column::Name)) - .first(); + QObject::connect( + self_->ui->removeButton, + &QPushButton::clicked, + self_, + [this]() + { + auto selectionModel = self_->ui->markerView->selectionModel(); + QModelIndex selected = selectionModel + ->selectedRows(static_cast( + model::MarkerModel::Column::Name)) + .first(); + std::optional id = markerModel_->getId(selected.row()); + if (!id) + { + return; + } - markerManager_->remove_marker(selected.row()); - }); + markerManager_->remove_marker(*id); + }); QObject::connect( self_->ui->markerView->selectionModel(), &QItemSelectionModel::selectionChanged, diff --git a/test/source/scwx/qt/model/marker_model.test.cpp b/test/source/scwx/qt/model/marker_model.test.cpp index b237e182..700ffc6e 100644 --- a/test/source/scwx/qt/model/marker_model.test.cpp +++ b/test/source/scwx/qt/model/marker_model.test.cpp @@ -65,7 +65,7 @@ void RunTest(const std::string& filename, TestFunction testFunction) initialized = false; QObject::connect(manager.get(), &manager::MarkerManager::MarkersInitialized, - []() + [](size_t count) { std::unique_lock lock(initializedMutex); initialized = true; @@ -79,6 +79,8 @@ void RunTest(const std::string& filename, TestFunction testFunction) { initializedCond.wait(lock); } + // This is not jank + model.HandleMarkersInitialized(manager->marker_count()); testFunction(manager, model); } @@ -118,9 +120,17 @@ TEST(MarkerModelTest, AddRemove) RunTest(ONE_MARKERS_FILE, [](std::shared_ptr manager, MarkerModel&) { manager->add_marker(types::MarkerInfo("Null", 0, 0)); }); - RunTest(EMPTY_MARKERS_FILE, - [](std::shared_ptr manager, MarkerModel&) - { manager->remove_marker(0); }); + RunTest( + EMPTY_MARKERS_FILE, + [](std::shared_ptr manager, MarkerModel& model) + { + std::optional id = model.getId(0); + EXPECT_TRUE(id); + if (id) + { + manager->remove_marker(*id); + } + }); std::filesystem::remove(TEMP_MARKERS_FILE); EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); @@ -165,15 +175,31 @@ TEST(MarkerModelTest, RemoveFive) { CopyFile(FIVE_MARKERS_FILE, TEMP_MARKERS_FILE); - RunTest(EMPTY_MARKERS_FILE, - [](std::shared_ptr manager, MarkerModel&) - { - manager->remove_marker(4); - manager->remove_marker(3); - manager->remove_marker(2); - manager->remove_marker(1); - manager->remove_marker(0); - }); + RunTest( + EMPTY_MARKERS_FILE, + [](std::shared_ptr manager, MarkerModel& model) + { + std::optional id; + id = model.getId(4); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(3); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(2); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(1); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(0); + EXPECT_TRUE(id); + manager->remove_marker(*id); + }); std::filesystem::remove(TEMP_MARKERS_FILE); EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); @@ -183,14 +209,27 @@ TEST(MarkerModelTest, RemoveFour) { CopyFile(FIVE_MARKERS_FILE, TEMP_MARKERS_FILE); - RunTest(ONE_MARKERS_FILE, - [](std::shared_ptr manager, MarkerModel&) - { - manager->remove_marker(4); - manager->remove_marker(3); - manager->remove_marker(2); - manager->remove_marker(1); - }); + RunTest( + ONE_MARKERS_FILE, + [](std::shared_ptr manager, MarkerModel& model) + { + std::optional id; + id = model.getId(4); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(3); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(2); + EXPECT_TRUE(id); + manager->remove_marker(*id); + + id = model.getId(1); + EXPECT_TRUE(id); + manager->remove_marker(*id); + }); std::filesystem::remove(TEMP_MARKERS_FILE); EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false);