From 2c9a8a33a4152f04db4455acdaebe1d268083182 Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Sun, 20 Oct 2024 12:34:12 -0400 Subject: [PATCH 1/6] fix error when reading an empty location marker file --- scwx-qt/source/scwx/qt/model/marker_model.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scwx-qt/source/scwx/qt/model/marker_model.cpp b/scwx-qt/source/scwx/qt/model/marker_model.cpp index eb3e8bee..8bcd93fa 100644 --- a/scwx-qt/source/scwx/qt/model/marker_model.cpp +++ b/scwx-qt/source/scwx/qt/model/marker_model.cpp @@ -254,6 +254,10 @@ bool MarkerModel::setData(const QModelIndex& index, void MarkerModel::HandleMarkersInitialized(size_t count) { + if (count == 0) + { + return; + } const int index = static_cast(count - 1); beginInsertRows(QModelIndex(), 0, index); From 236d7c1e353fdf0a2f5a17984a836bf8c3287728 Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Sun, 20 Oct 2024 12:37:08 -0400 Subject: [PATCH 2/6] Added test cases for marker_model and marker_manager --- scwx-qt/source/scwx/qt/main/application.cpp | 8 + scwx-qt/source/scwx/qt/main/application.hpp | 2 + .../source/scwx/qt/manager/marker_manager.cpp | 12 +- .../source/scwx/qt/manager/marker_manager.hpp | 5 +- test/data | 2 +- .../scwx/qt/model/marker_model.test.cpp | 201 ++++++++++++++++++ test/test.cmake | 3 +- 7 files changed, 227 insertions(+), 6 deletions(-) create mode 100644 test/source/scwx/qt/model/marker_model.test.cpp diff --git a/scwx-qt/source/scwx/qt/main/application.cpp b/scwx-qt/source/scwx/qt/main/application.cpp index d851c8ec..3f62120a 100644 --- a/scwx-qt/source/scwx/qt/main/application.cpp +++ b/scwx-qt/source/scwx/qt/main/application.cpp @@ -44,6 +44,14 @@ void WaitForInitialization() } } +// Only use for test cases +void ResetInitilization() +{ + logger_->debug("Application initialization reset"); + std::unique_lock lock(initializationMutex_); + initialized_ = false; +} + } // namespace Application } // namespace main } // namespace qt diff --git a/scwx-qt/source/scwx/qt/main/application.hpp b/scwx-qt/source/scwx/qt/main/application.hpp index 9b63ffad..0237f13c 100644 --- a/scwx-qt/source/scwx/qt/main/application.hpp +++ b/scwx-qt/source/scwx/qt/main/application.hpp @@ -11,6 +11,8 @@ namespace Application void FinishInitialization(); void WaitForInitialization(); +// Only use for test cases +void ResetInitilization(); } // namespace Application } // namespace main diff --git a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp index 382aafd7..c8ceda09 100644 --- a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp +++ b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp @@ -36,7 +36,7 @@ public: explicit Impl(MarkerManager* self) : self_ {self} {} ~Impl() { threadPool_.join(); } - std::string markerSettingsPath_ {}; + std::string markerSettingsPath_ {""}; std::vector> markerRecords_ {}; MarkerManager* self_; @@ -176,14 +176,13 @@ MarkerManager::Impl::GetMarkerByName(const std::string& name) MarkerManager::MarkerManager() : p(std::make_unique(this)) { + p->InitializeMarkerSettings(); boost::asio::post(p->threadPool_, [this]() { try { - p->InitializeMarkerSettings(); - // Read Marker settings on startup main::Application::WaitForInitialization(); p->ReadMarkerSettings(); @@ -293,6 +292,13 @@ void MarkerManager::move_marker(size_t from, size_t to) Q_EMIT MarkersUpdated(); } +// Only use for testing +void MarkerManager::set_marker_settings_path(const std::string& path) +{ + p->markerSettingsPath_ = path; +} + + std::shared_ptr MarkerManager::Instance() { static std::weak_ptr markerManagerReference_ {}; diff --git a/scwx-qt/source/scwx/qt/manager/marker_manager.hpp b/scwx-qt/source/scwx/qt/manager/marker_manager.hpp index 2f073ab7..2166725f 100644 --- a/scwx-qt/source/scwx/qt/manager/marker_manager.hpp +++ b/scwx-qt/source/scwx/qt/manager/marker_manager.hpp @@ -20,13 +20,16 @@ public: explicit MarkerManager(); ~MarkerManager(); - size_t marker_count(); + size_t marker_count(); std::optional get_marker(size_t index); void set_marker(size_t index, const types::MarkerInfo& marker); void add_marker(const types::MarkerInfo& marker); void remove_marker(size_t index); void move_marker(size_t from, size_t to); + // Only use for testing + void set_marker_settings_path(const std::string& path); + static std::shared_ptr Instance(); signals: diff --git a/test/data b/test/data index 40a367ca..166c5a7b 160000 --- a/test/data +++ b/test/data @@ -1 +1 @@ -Subproject commit 40a367ca89b5b197353ca58dea547a3e3407c7f3 +Subproject commit 166c5a7bcaf8ad0a42bedf8f8dc5c4aa907e7151 diff --git a/test/source/scwx/qt/model/marker_model.test.cpp b/test/source/scwx/qt/model/marker_model.test.cpp new file mode 100644 index 00000000..b237e182 --- /dev/null +++ b/test/source/scwx/qt/model/marker_model.test.cpp @@ -0,0 +1,201 @@ +#include +#include +#include + +#include +#include +#include + +#include +#include + + +namespace scwx +{ +namespace qt +{ +namespace model +{ + +static const std::string EMPTY_MARKERS_FILE = + std::string(SCWX_TEST_DATA_DIR) + "/json/markers/markers-empty.json"; +static const std::string TEMP_MARKERS_FILE = + std::string(SCWX_TEST_DATA_DIR) + "/json/markers/markers-temp.json"; +static const std::string ONE_MARKERS_FILE = + std::string(SCWX_TEST_DATA_DIR) + "/json/markers/markers-one.json"; +static const std::string FIVE_MARKERS_FILE = + std::string(SCWX_TEST_DATA_DIR) + "/json/markers/markers-five.json"; + +static std::mutex initializedMutex {}; +static std::condition_variable initializedCond {}; +static bool initialized; + +void CompareFiles(const std::string& file1, const std::string& file2) +{ + std::ifstream ifs1 {file1}; + std::stringstream buffer1; + buffer1 << ifs1.rdbuf(); + + std::ifstream ifs2 {file2}; + std::stringstream buffer2; + buffer2 << ifs2.rdbuf(); + + EXPECT_EQ(buffer1.str(), buffer2.str()); +} + +void CopyFile(const std::string& from, const std::string& to) +{ + std::filesystem::copy_file(from, to); + CompareFiles(from, to); +} + +typedef void TestFunction(std::shared_ptr manager, + MarkerModel& model); + +void RunTest(const std::string& filename, TestFunction testFunction) +{ + { + main::Application::ResetInitilization(); + MarkerModel model = MarkerModel(); + std::shared_ptr manager = + manager::MarkerManager::Instance(); + + manager->set_marker_settings_path(TEMP_MARKERS_FILE); + + initialized = false; + QObject::connect(manager.get(), + &manager::MarkerManager::MarkersInitialized, + []() + { + std::unique_lock lock(initializedMutex); + initialized = true; + initializedCond.notify_all(); + }); + + main::Application::FinishInitialization(); + + std::unique_lock lock(initializedMutex); + while (!initialized) + { + initializedCond.wait(lock); + } + + testFunction(manager, model); + } + + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), true); + + CompareFiles(TEMP_MARKERS_FILE, filename); +} + +TEST(MarkerModelTest, CreateJson) +{ + // Verify file doesn't exist prior to test start + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); + + RunTest(EMPTY_MARKERS_FILE, + [](std::shared_ptr, MarkerModel&) {}); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +TEST(MarkerModelTest, LoadEmpty) +{ + CopyFile(EMPTY_MARKERS_FILE, TEMP_MARKERS_FILE); + + RunTest(EMPTY_MARKERS_FILE, + [](std::shared_ptr, MarkerModel&) {}); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +TEST(MarkerModelTest, AddRemove) +{ + CopyFile(EMPTY_MARKERS_FILE, TEMP_MARKERS_FILE); + + 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); }); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +TEST(MarkerModelTest, AddFive) +{ + CopyFile(EMPTY_MARKERS_FILE, TEMP_MARKERS_FILE); + + RunTest(FIVE_MARKERS_FILE, + [](std::shared_ptr manager, MarkerModel&) + { + manager->add_marker(types::MarkerInfo("Null", 0, 0)); + manager->add_marker(types::MarkerInfo("North", 90, 0)); + manager->add_marker(types::MarkerInfo("South", -90, 0)); + manager->add_marker(types::MarkerInfo("East", 0, 90)); + manager->add_marker(types::MarkerInfo("West", 0, -90)); + }); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +TEST(MarkerModelTest, AddFour) +{ + CopyFile(ONE_MARKERS_FILE, TEMP_MARKERS_FILE); + + RunTest(FIVE_MARKERS_FILE, + [](std::shared_ptr manager, MarkerModel&) + { + manager->add_marker(types::MarkerInfo("North", 90, 0)); + manager->add_marker(types::MarkerInfo("South", -90, 0)); + manager->add_marker(types::MarkerInfo("East", 0, 90)); + manager->add_marker(types::MarkerInfo("West", 0, -90)); + }); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +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); + }); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +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); + }); + + std::filesystem::remove(TEMP_MARKERS_FILE); + EXPECT_EQ(std::filesystem::exists(TEMP_MARKERS_FILE), false); +} + +} // namespace model +} // namespace qt +} // namespace scwx diff --git a/test/test.cmake b/test/test.cmake index b3cfedd2..dad5ab9b 100644 --- a/test/test.cmake +++ b/test/test.cmake @@ -25,7 +25,8 @@ set(SRC_QT_CONFIG_TESTS source/scwx/qt/config/county_database.test.cpp set(SRC_QT_MANAGER_TESTS source/scwx/qt/manager/settings_manager.test.cpp source/scwx/qt/manager/update_manager.test.cpp) set(SRC_QT_MAP_TESTS source/scwx/qt/map/map_provider.test.cpp) -set(SRC_QT_MODEL_TESTS source/scwx/qt/model/imgui_context_model.test.cpp) +set(SRC_QT_MODEL_TESTS source/scwx/qt/model/imgui_context_model.test.cpp + source/scwx/qt/model/marker_model.test.cpp) set(SRC_QT_SETTINGS_TESTS source/scwx/qt/settings/settings_container.test.cpp source/scwx/qt/settings/settings_variable.test.cpp) set(SRC_QT_UTIL_TESTS source/scwx/qt/util/q_file_input_stream.test.cpp From 7a070b3e7f7227aeb8626a9254e561192c21c3fd Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Sun, 3 Nov 2024 14:16:56 -0500 Subject: [PATCH 3/6] 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); From 35f2c85a199f6f5b19ae46f807e73fa179e66314 Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Mon, 25 Nov 2024 11:18:03 -0500 Subject: [PATCH 4/6] Some minor code changes, as well as properly using marker index --- scwx-qt/source/scwx/qt/manager/marker_manager.cpp | 2 +- scwx-qt/source/scwx/qt/model/marker_model.cpp | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp index e91f03fb..99208a10 100644 --- a/scwx-qt/source/scwx/qt/manager/marker_manager.cpp +++ b/scwx-qt/source/scwx/qt/manager/marker_manager.cpp @@ -315,7 +315,7 @@ void MarkerManager::remove_marker(types::MarkerId id) { if (pair.second > index) { - p->idToIndex_[pair.first] = pair.second - 1; + pair.second -= 1; } } } diff --git a/scwx-qt/source/scwx/qt/model/marker_model.cpp b/scwx-qt/source/scwx/qt/model/marker_model.cpp index 97cafcb1..9bd057ea 100644 --- a/scwx-qt/source/scwx/qt/model/marker_model.cpp +++ b/scwx-qt/source/scwx/qt/model/marker_model.cpp @@ -297,14 +297,19 @@ void MarkerModel::HandleMarkerAdded(types::MarkerId id) const int newIndex = static_cast(*index); beginInsertRows(QModelIndex(), newIndex, newIndex); - p->markerIds_.emplace_back(id); + auto it = std::next(p->markerIds_.begin(), newIndex); + p->markerIds_.emplace(it, id); endInsertRows(); } void MarkerModel::HandleMarkerChanged(types::MarkerId id) { - std::optional index = p->markerManager_->get_index(id); - const int changedIndex = static_cast(*index); + auto it = std::find(p->markerIds_.begin(), p->markerIds_.end(), id); + if (it == p->markerIds_.end()) + { + return; + } + const int changedIndex = std::distance(p->markerIds_.begin(), it); QModelIndex topLeft = createIndex(changedIndex, kFirstColumn); QModelIndex bottomRight = createIndex(changedIndex, kLastColumn); From ee5719cb55772ef8d411a61c6b0ca8ebd440020c Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Tue, 26 Nov 2024 08:57:10 -0500 Subject: [PATCH 5/6] Update test/data in location_markers_ids branch to match develop --- test/data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/data b/test/data index 166c5a7b..a642d730 160000 --- a/test/data +++ b/test/data @@ -1 +1 @@ -Subproject commit 166c5a7bcaf8ad0a42bedf8f8dc5c4aa907e7151 +Subproject commit a642d730bd8d6c9b291b90e61b3a3a389139f2f6 From 831e0f7d1b5cdbce69cb5f722888a63117b730c6 Mon Sep 17 00:00:00 2001 From: AdenKoperczak Date: Wed, 27 Nov 2024 12:05:04 -0500 Subject: [PATCH 6/6] Add check to optional value to avoid undefined behavior in MarkerModel --- scwx-qt/source/scwx/qt/model/marker_model.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scwx-qt/source/scwx/qt/model/marker_model.cpp b/scwx-qt/source/scwx/qt/model/marker_model.cpp index 9bd057ea..e550312e 100644 --- a/scwx-qt/source/scwx/qt/model/marker_model.cpp +++ b/scwx-qt/source/scwx/qt/model/marker_model.cpp @@ -294,6 +294,10 @@ void MarkerModel::HandleMarkersInitialized(size_t count) void MarkerModel::HandleMarkerAdded(types::MarkerId id) { std::optional index = p->markerManager_->get_index(id); + if (!index) + { + return; + } const int newIndex = static_cast(*index); beginInsertRows(QModelIndex(), newIndex, newIndex);