From 92824ab302b88fa6dbff89d49252c2cfe484b6ee Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Wed, 15 Aug 2018 19:19:20 +0300 Subject: [PATCH] [editor] synchronization based on shared pointers. The editor is thread-safe for read operations now. --- base/atomic_shared_ptr.hpp | 22 + editor/config_loader.cpp | 2 +- editor/config_loader.hpp | 7 +- editor/editable_feature_source.cpp | 6 +- editor/editable_feature_source.hpp | 4 +- editor/editor_config.hpp | 32 -- editor/editor_storage.cpp | 15 +- editor/editor_storage.hpp | 11 +- editor/editor_tests/config_loader_test.cpp | 4 +- editor/editor_tests/osm_editor_test.cpp | 274 ++++++++++- editor/editor_tests/osm_editor_test.hpp | 1 + editor/editor_tests_support/helpers.cpp | 2 + editor/osm_editor.cpp | 528 +++++++++++---------- editor/osm_editor.hpp | 70 +-- indexer/data_source.cpp | 2 +- indexer/feature_source.cpp | 4 +- indexer/feature_source.hpp | 4 +- map/framework.cpp | 5 +- search/mwm_context.hpp | 2 +- 19 files changed, 638 insertions(+), 357 deletions(-) create mode 100644 base/atomic_shared_ptr.hpp diff --git a/base/atomic_shared_ptr.hpp b/base/atomic_shared_ptr.hpp new file mode 100644 index 0000000000..ce57694782 --- /dev/null +++ b/base/atomic_shared_ptr.hpp @@ -0,0 +1,22 @@ +#pragma once + +#include "base/macros.hpp" + +#include + +namespace base +{ +// Template which provides methods for concurrently using shared pointers. +template +class AtomicSharedPtr +{ +public: + AtomicSharedPtr() = default; + + void Set(std::shared_ptr value) noexcept { atomic_store(&m_wrapped, value); } + std::shared_ptr Get() const noexcept { return atomic_load(&m_wrapped); } + +private: + std::shared_ptr m_wrapped = std::make_shared(); +}; +} // namespace base diff --git a/editor/config_loader.cpp b/editor/config_loader.cpp index 1900cbeacf..71e765dbfb 100644 --- a/editor/config_loader.cpp +++ b/editor/config_loader.cpp @@ -61,7 +61,7 @@ void Waiter::Interrupt() m_event.notify_all(); } -ConfigLoader::ConfigLoader(EditorConfigWrapper & config) : m_config(config) +ConfigLoader::ConfigLoader(base::AtomicSharedPtr & config) : m_config(config) { pugi::xml_document doc; LoadFromLocal(doc); diff --git a/editor/config_loader.hpp b/editor/config_loader.hpp index 0b445b22e0..73bf1ee527 100644 --- a/editor/config_loader.hpp +++ b/editor/config_loader.hpp @@ -1,5 +1,6 @@ #pragma once +#include "base/atomic_shared_ptr.hpp" #include "base/exception.hpp" #include "base/logging.hpp" @@ -17,7 +18,7 @@ class xml_document; namespace editor { -class EditorConfigWrapper; +class EditorConfig; // Class for multithreaded interruptable waiting. class Waiter @@ -49,7 +50,7 @@ private: class ConfigLoader { public: - explicit ConfigLoader(EditorConfigWrapper & config); + explicit ConfigLoader(base::AtomicSharedPtr & config); ~ConfigLoader(); // Static methods for production and testing. @@ -64,7 +65,7 @@ private: bool SaveAndReload(pugi::xml_document const & doc); void ResetConfig(pugi::xml_document const & doc); - EditorConfigWrapper & m_config; + base::AtomicSharedPtr & m_config; Waiter m_waiter; thread m_loaderThread; diff --git a/editor/editable_feature_source.cpp b/editor/editable_feature_source.cpp index b341e9935b..ee55e84cbf 100644 --- a/editor/editable_feature_source.cpp +++ b/editor/editable_feature_source.cpp @@ -14,9 +14,9 @@ bool EditableFeatureSource::GetModifiedFeature(uint32_t index, FeatureType & fea return editor.GetEditedFeature(m_handle.GetId(), index, feature); } -void EditableFeatureSource::ForEachInRectAndScale(m2::RectD const & rect, int scale, - std::function const & fn) const +void EditableFeatureSource::ForEachAdditionalFeature(m2::RectD const & rect, int scale, + std::function const & fn) const { osm::Editor & editor = osm::Editor::Instance(); - editor.ForEachFeatureInMwmRectAndScale(m_handle.GetId(), fn, rect, scale); + editor.ForEachCreatedFeature(m_handle.GetId(), fn, rect, scale); } diff --git a/editor/editable_feature_source.hpp b/editor/editable_feature_source.hpp index e9265b48b4..63f58fe6ba 100644 --- a/editor/editable_feature_source.hpp +++ b/editor/editable_feature_source.hpp @@ -18,8 +18,8 @@ public: // FeatureSource overrides: FeatureStatus GetFeatureStatus(uint32_t index) const override; bool GetModifiedFeature(uint32_t index, FeatureType & feature) const override; - void ForEachInRectAndScale(m2::RectD const & rect, int scale, - std::function const & fn) const override; + void ForEachAdditionalFeature(m2::RectD const & rect, int scale, + std::function const & fn) const override; }; class EditableFeatureSourceFactory : public FeatureSourceFactory diff --git a/editor/editor_config.hpp b/editor/editor_config.hpp index 7e55b35549..8afa7b396e 100644 --- a/editor/editor_config.hpp +++ b/editor/editor_config.hpp @@ -2,8 +2,6 @@ #include "indexer/feature_meta.hpp" -#include "std/mutex.hpp" -#include "std/shared_ptr.hpp" #include "std/string.hpp" #include "std/vector.hpp" @@ -53,34 +51,4 @@ public: private: pugi::xml_document m_document; }; - -// Class which provides methods for EditorConfig concurrently using. -class EditorConfigWrapper -{ -public: - EditorConfigWrapper() = default; - - void Set(shared_ptr config) - { - lock_guard lock(m_mu); - m_config = config; - } - - shared_ptr Get() const - { - lock_guard lock(m_mu); - return m_config; - } - -private: - // It's possible to use atomic_{load|store} here instead of mutex, - // but seems that libstdc++4.9 doesn't support it. Need to rewrite - // this code as soon as libstdc++5 will be ready for lastest Debian - // release, or as soon as atomic_shared_ptr will be ready. - mutable mutex m_mu; - shared_ptr m_config = make_shared(); - - // Just in case someone tryes to pass EditorConfigWrapper by value instead of referense. - DISALLOW_COPY_AND_MOVE(EditorConfigWrapper); -}; } // namespace editor diff --git a/editor/editor_storage.cpp b/editor/editor_storage.cpp index 8f2a878430..d1ccf6b0ad 100644 --- a/editor/editor_storage.cpp +++ b/editor/editor_storage.cpp @@ -23,6 +23,9 @@ namespace editor bool LocalStorage::Save(xml_document const & doc) { auto const editorFilePath = GetEditorFilePath(); + + std::lock_guard guard(m_mutex); + return my::WriteToTempAndRenameToFile(editorFilePath, [&doc](string const & fileName) { return doc.save_file(fileName.data(), " " /* indent */); }); @@ -31,6 +34,9 @@ bool LocalStorage::Save(xml_document const & doc) bool LocalStorage::Load(xml_document & doc) { auto const editorFilePath = GetEditorFilePath(); + + std::lock_guard guard(m_mutex); + auto const result = doc.load_file(editorFilePath.c_str()); // Note: status_file_not_found is ok if a user has never made any edits. if (result != status_ok && result != status_file_not_found) @@ -42,9 +48,11 @@ bool LocalStorage::Load(xml_document & doc) return true; } -void LocalStorage::Reset() +bool LocalStorage::Reset() { - my::DeleteFileX(GetEditorFilePath()); + std::lock_guard guard(m_mutex); + + return my::DeleteFileX(GetEditorFilePath()); } // StorageMemory ----------------------------------------------------------------------------------- @@ -60,8 +68,9 @@ bool InMemoryStorage::Load(xml_document & doc) return true; } -void InMemoryStorage::Reset() +bool InMemoryStorage::Reset() { m_doc.reset(); + return true; } } // namespace editor diff --git a/editor/editor_storage.hpp b/editor/editor_storage.hpp index 269c6754a5..fe1a4605c7 100644 --- a/editor/editor_storage.hpp +++ b/editor/editor_storage.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include "3party/pugixml/src/pugixml.hpp" namespace editor @@ -12,7 +14,7 @@ public: virtual bool Save(pugi::xml_document const & doc) = 0; virtual bool Load(pugi::xml_document & doc) = 0; - virtual void Reset() = 0; + virtual bool Reset() = 0; }; // Class which saves/loads edits to/from local file. @@ -22,7 +24,10 @@ public: // StorageBase overrides: bool Save(pugi::xml_document const & doc) override; bool Load(pugi::xml_document & doc) override; - void Reset() override; + bool Reset() override; + +private: + std::mutex m_mutex; }; // Class which saves/loads edits to/from xml_document class instance. @@ -32,7 +37,7 @@ public: // StorageBase overrides: bool Save(pugi::xml_document const & doc) override; bool Load(pugi::xml_document & doc) override; - void Reset() override; + bool Reset() override; private: pugi::xml_document m_doc; diff --git a/editor/editor_tests/config_loader_test.cpp b/editor/editor_tests/config_loader_test.cpp index c295450e2a..abb45dd95a 100644 --- a/editor/editor_tests/config_loader_test.cpp +++ b/editor/editor_tests/config_loader_test.cpp @@ -5,6 +5,8 @@ #include "platform/platform_tests_support/scoped_file.hpp" +#include "base/atomic_shared_ptr.hpp" + #include "3party/pugixml/src/pugixml.hpp" namespace @@ -24,7 +26,7 @@ void CheckGeneralTags(pugi::xml_document const & doc) UNIT_TEST(ConfigLoader_Base) { - EditorConfigWrapper config; + base::AtomicSharedPtr config; ConfigLoader loader(config); TEST(!config.Get()->GetTypesThatCanBeAdded().empty(), ()); diff --git a/editor/editor_tests/osm_editor_test.cpp b/editor/editor_tests/osm_editor_test.cpp index 46fd140e56..722af8b7ff 100644 --- a/editor/editor_tests/osm_editor_test.cpp +++ b/editor/editor_tests/osm_editor_test.cpp @@ -53,6 +53,58 @@ public: } }; +class OptionalSaveStorage : public editor::InMemoryStorage +{ +public: + void AllowSave(bool allow) + { + m_allowSave = allow; + } + // StorageBase overrides: + bool Save(pugi::xml_document const & doc) override + { + if (m_allowSave) + return InMemoryStorage::Save(doc); + + return false; + } + + bool Reset() override + { + if (m_allowSave) + return InMemoryStorage::Reset(); + + return false; + } + +private: + bool m_allowSave = true; +}; + +class ScopedOptionalSaveStorage +{ +public: + ScopedOptionalSaveStorage() + { + auto & editor = osm::Editor::Instance(); + editor.SetStorageForTesting(unique_ptr(m_storage)); + } + + void AllowSave(bool allow) + { + m_storage->AllowSave(allow); + } + + ~ScopedOptionalSaveStorage() + { + auto & editor = osm::Editor::Instance(); + editor.SetStorageForTesting(make_unique()); + } + +private: + OptionalSaveStorage * m_storage = new OptionalSaveStorage; +}; + template void ForEachCafeAtPoint(DataSource & dataSource, m2::PointD const & mercator, TFn && fn) { @@ -123,8 +175,7 @@ uint32_t CountFeaturesInRect(MwmSet::MwmId const & mwmId, m2::RectD const & rect auto & editor = osm::Editor::Instance(); int unused = 0; uint32_t counter = 0; - editor.ForEachFeatureInMwmRectAndScale(mwmId, [&counter](uint32_t index) { ++counter; }, rect, - unused); + editor.ForEachCreatedFeature(mwmId, [&counter](uint32_t index) { ++counter; }, rect, unused); return counter; } @@ -159,7 +210,6 @@ EditorTest::EditorTest() EditorTest::~EditorTest() { - editor::tests_support::TearDownEditorForTesting(); for (auto const & file : m_mwmFiles) @@ -172,7 +222,8 @@ void EditorTest::GetFeatureTypeInfoTest() { MwmSet::MwmId mwmId; - TEST(!editor.GetFeatureTypeInfo(mwmId, 0), ()); + + TEST(!editor.GetFeatureTypeInfo((*editor.m_features.Get()), mwmId, 0), ()); } auto const mwmId = ConstructTestMwm([](TestMwmBuilder & builder) @@ -183,11 +234,13 @@ void EditorTest::GetFeatureTypeInfoTest() ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&editor](FeatureType & ft) { - TEST(!editor.GetFeatureTypeInfo(ft.GetID().m_mwmId, ft.GetID().m_index), ()); + auto const featuresBefore = editor.m_features.Get(); + TEST(!editor.GetFeatureTypeInfo(*featuresBefore, ft.GetID().m_mwmId, ft.GetID().m_index), ()); SetBuildingLevelsToOne(ft); - auto const fti = editor.GetFeatureTypeInfo(ft.GetID().m_mwmId, ft.GetID().m_index); + auto const featuresAfter = editor.m_features.Get(); + auto const fti = editor.GetFeatureTypeInfo(*featuresAfter, ft.GetID().m_mwmId, ft.GetID().m_index); TEST_NOT_EQUAL(fti, 0, ()); TEST_EQUAL(fti->m_feature.GetID(), ft.GetID(), ()); }); @@ -452,9 +505,9 @@ void EditorTest::ClearAllLocalEditsTest() osm::EditableMapObject emo; CreateCafeAtPoint({3.0, 3.0}, mwmId, emo); - TEST(!editor.m_features.empty(), ()); + TEST(!editor.m_features.Get()->empty(), ()); editor.ClearAllLocalEdits(); - TEST(editor.m_features.empty(), ()); + TEST(editor.m_features.Get()->empty(), ()); } void EditorTest::GetFeaturesByStatusTest() @@ -550,14 +603,14 @@ void EditorTest::OnMapDeregisteredTest() SetBuildingLevelsToOne(ft); }); - TEST(!editor.m_features.empty(), ()); - TEST_EQUAL(editor.m_features.size(), 2, ()); + TEST(!editor.m_features.Get()->empty(), ()); + TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); editor.OnMapDeregistered(gbMwmId.GetInfo()->GetLocalFile()); - TEST_EQUAL(editor.m_features.size(), 1, ()); - auto const editedMwmId = editor.m_features.find(rfMwmId); - bool result = (editedMwmId != editor.m_features.end()); + TEST_EQUAL(editor.m_features.Get()->size(), 1, ()); + auto const editedMwmId = editor.m_features.Get()->find(rfMwmId); + bool result = (editedMwmId != editor.m_features.Get()->end()); TEST(result, ()); } @@ -923,13 +976,13 @@ void EditorTest::LoadMapEditsTest() CreateCafeAtPoint({5.0, 5.0}, rfMwmId, emo); features.emplace_back(emo.GetID()); - editor.Save(); + editor.Save((*editor.m_features.Get())); editor.LoadEdits(); auto const fillLoaded = [&editor](vector & loadedFeatures) { loadedFeatures.clear(); - for (auto const & mwm : editor.m_features) + for (auto const & mwm : *(editor.m_features.Get())) { for (auto const & index : mwm.second) { @@ -963,12 +1016,12 @@ void EditorTest::LoadMapEditsTest() m_dataSource.DeregisterMap(m_mwmFiles.back().GetCountryFile()); TEST(RemoveMwm(newRfMwmId), ()); - TEST_EQUAL(editor.m_features.size(), 2, ()); + TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); editor.LoadEdits(); fillLoaded(loadedFeatures); - TEST_EQUAL(editor.m_features.size(), 1, ()); + TEST_EQUAL(editor.m_features.Get()->size(), 1, ()); TEST_EQUAL(loadedFeatures.size(), 2, ()); osm::EditableMapObject gbEmo; @@ -980,7 +1033,7 @@ void EditorTest::LoadMapEditsTest() editor.LoadEdits(); fillLoaded(loadedFeatures); - TEST_EQUAL(editor.m_features.size(), 1, ()); + TEST_EQUAL(editor.m_features.Get()->size(), 1, ()); TEST_EQUAL(loadedFeatures.size(), 1, ()); m_dataSource.DeregisterMap(m_mwmFiles.back().GetCountryFile()); @@ -995,7 +1048,7 @@ void EditorTest::LoadMapEditsTest() newGbMwmId.GetInfo()->m_version.SetSecondsSinceEpoch(time(nullptr) + 1); editor.LoadEdits(); - TEST(editor.m_features.empty(), ()); + TEST(editor.m_features.Get()->empty(), ()); } void EditorTest::SaveEditedFeatureTest() @@ -1036,6 +1089,185 @@ void EditorTest::SaveEditedFeatureTest() }); } +void EditorTest::SaveTransactionTest() +{ + ScopedOptionalSaveStorage optionalSaveStorage; + auto & editor = osm::Editor::Instance(); + + auto const mwmId = BuildMwm("GB", [](TestMwmBuilder & builder) + { + builder.Add(TestCafe(m2::PointD(1.0, 1.0), "London Cafe1", "en")); + builder.Add(TestCafe(m2::PointD(4.0, 4.0), "London Cafe2", "en")); + + builder.Add(TestPOI(m2::PointD(6.0, 6.0), "Corner Post", "default")); + }); + + auto const rfMwmId = BuildMwm("RF", [](TestMwmBuilder & builder) + { + builder.Add(TestCafe(m2::PointD(10.0, 10.0), "Moscow Cafe1", "en")); + }); + + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [](FeatureType & ft) + { + SetBuildingLevelsToOne(ft); + }); + + ForEachCafeAtPoint(m_dataSource, m2::PointD(10.0, 10.0), [](FeatureType & ft) + { + SetBuildingLevelsToOne(ft); + }); + + auto const features = editor.m_features.Get(); + + TEST_EQUAL(features->size(), 2, ()); + TEST_EQUAL(features->begin()->second.size(), 1, ()); + TEST_EQUAL(features->rbegin()->second.size(), 1, ()); + + optionalSaveStorage.AllowSave(false); + + { + auto saveResult = osm::Editor::SaveResult::NothingWasChanged; + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&saveResult](FeatureType & ft) + { + auto & editor = osm::Editor::Instance(); + + osm::EditableMapObject emo; + emo.SetFromFeatureType(ft); + emo.SetEditableProperties(editor.GetEditableProperties(ft)); + emo.SetBuildingLevels("5"); + + saveResult = editor.SaveEditedFeature(emo); + }); + + TEST_EQUAL(saveResult, osm::Editor::SaveResult::NoFreeSpaceError, ()); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + } + + { + auto saveResult = osm::Editor::SaveResult::NothingWasChanged; + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&saveResult](FeatureType & ft) + { + auto & editor = osm::Editor::Instance(); + + osm::EditableMapObject emo; + emo.SetFromFeatureType(ft); + emo.SetEditableProperties(editor.GetEditableProperties(ft)); + emo.SetBuildingLevels(""); + + saveResult = editor.SaveEditedFeature(emo); + }); + + TEST_EQUAL(saveResult, osm::Editor::SaveResult::SavingError, ()); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + { + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&editor](FeatureType & ft) + { + editor.DeleteFeature(ft.GetID()); + }); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + { + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&editor](FeatureType & ft) + { + editor.MarkFeatureAsObsolete(ft.GetID()); + }); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + { + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&editor](FeatureType & ft) + { + using NoteType = osm::Editor::NoteProblemType; + feature::TypesHolder typesHolder; + string defaultName; + editor.CreateNote({1.0, 1.0}, ft.GetID(), typesHolder, defaultName, NoteType::PlaceDoesNotExist, + "exploded"); + }); + + TEST_EQUAL(editor.m_notes->NotUploadedNotesCount(), 0, ()); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + { + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [&editor](FeatureType & ft) + { + editor.RollBackChanges(ft.GetID()); + }); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + { + editor.ClearAllLocalEdits(); + TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); + } + + { + editor.OnMapDeregistered(mwmId.GetInfo()->GetLocalFile()); + + auto const features = editor.m_features.Get(); + auto const mwmIt = features->find(mwmId); + + TEST(mwmIt != features->end(), ()); + TEST_EQUAL(mwmIt->second.size(), 1, ()); + + auto const featureInfo = mwmIt->second.begin()->second; + TEST_EQUAL(featureInfo.m_status, FeatureStatus::Modified, ()); + } + + optionalSaveStorage.AllowSave(true); + + editor.ClearAllLocalEdits(); + TEST(editor.m_features.Get()->empty(), ()); +} + void EditorTest::Cleanup(platform::LocalCountryFile const & map) { platform::CountryIndexes::DeleteFromDisk(map); @@ -1145,9 +1377,13 @@ UNIT_CLASS_TEST(EditorTest, CreateNoteTest) { EditorTest::CreateNoteTest(); } + UNIT_CLASS_TEST(EditorTest, LoadMapEditsTest) { EditorTest::LoadMapEditsTest(); } + UNIT_CLASS_TEST(EditorTest, SaveEditedFeatureTest) { EditorTest::SaveEditedFeatureTest(); } + +UNIT_CLASS_TEST(EditorTest, SaveTransactionTest) { EditorTest::SaveTransactionTest(); } } // namespace diff --git a/editor/editor_tests/osm_editor_test.hpp b/editor/editor_tests/osm_editor_test.hpp index 724eb8241a..d8a0369c9d 100644 --- a/editor/editor_tests/osm_editor_test.hpp +++ b/editor/editor_tests/osm_editor_test.hpp @@ -42,6 +42,7 @@ public: void CreateNoteTest(); void LoadMapEditsTest(); void SaveEditedFeatureTest(); + void SaveTransactionTest(); private: template diff --git a/editor/editor_tests_support/helpers.cpp b/editor/editor_tests_support/helpers.cpp index bd86cad2bd..bb79236dde 100644 --- a/editor/editor_tests_support/helpers.cpp +++ b/editor/editor_tests_support/helpers.cpp @@ -12,12 +12,14 @@ void SetUpEditorForTesting(unique_ptr delegate) editor.SetDelegate(move(delegate)); editor.SetStorageForTesting(make_unique()); editor.ClearAllLocalEdits(); + editor.ResetNotes(); } void TearDownEditorForTesting() { auto & editor = osm::Editor::Instance(); editor.ClearAllLocalEdits(); + editor.ResetNotes(); editor.SetDelegate({}); editor.SetDefaultStorage(); } diff --git a/editor/osm_editor.cpp b/editor/osm_editor.cpp index 40be98186f..fa82137df6 100644 --- a/editor/osm_editor.cpp +++ b/editor/osm_editor.cpp @@ -191,6 +191,7 @@ void Editor::SetDefaultStorage() { m_storage = make_unique void Editor::LoadEdits() { + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); if (!m_delegate) { LOG(LERROR, ("Can't load any map edits, delegate has not been set.")); @@ -200,14 +201,11 @@ void Editor::LoadEdits() xml_document doc; bool needRewriteEdits = false; - { - std::lock_guard lock(m_mutex); + if (!m_storage->Load(doc)) + return; - if (!m_storage->Load(doc)) - return; - - m_features.clear(); - } + m_features.Set(make_shared()); + auto loadedFeatures = make_shared(); for (auto const & mwm : doc.child(kXmlRootNode).children(kXmlMwmNode)) { @@ -224,36 +222,28 @@ void Editor::LoadEdits() continue; } - // TODO(mgsergio, AlexZ): Is it normal to have isMwmIdAlive and mapVersion - // NOT equal to mwmId.GetInfo()->GetVersion() at the same time? auto const needMigrateEdits = mapVersion != mwmId.GetInfo()->GetVersion(); needRewriteEdits = needRewriteEdits || needMigrateEdits; - // Synchronize access to m_features into LoadMwmEdits separately to prevent - // recursive lock from Delegate::ForEachFeatureAtPoint. - LoadMwmEdits(mwm, mwmId, needMigrateEdits); + LoadMwmEdits(*loadedFeatures, mwm, mwmId, needMigrateEdits); } // Save edits with new indexes and mwm version to avoid another migration on next startup. if (needRewriteEdits) - { - std::lock_guard lock(m_mutex); - Save(); - } + SaveTransaction(loadedFeatures); + else + m_features.Set(loadedFeatures); } -bool Editor::Save() const +bool Editor::Save(FeaturesContainer const & features) const { - if (m_features.empty()) - { - m_storage->Reset(); - return true; - } + if (features.empty()) + return m_storage->Reset(); xml_document doc; xml_node root = doc.append_child(kXmlRootNode); // Use format_version for possible future format changes. root.append_attribute("format_version") = 1; - for (auto & mwm : m_features) + for (auto const & mwm : features) { xml_node mwmNode = root.append_child(kXmlMwmNode); mwmNode.append_attribute("name") = mwm.first.GetInfo()->GetCountryName().c_str(); @@ -299,73 +289,90 @@ bool Editor::Save() const return m_storage->Save(doc); } +bool Editor::SaveTransaction(shared_ptr const & features) +{ + if (!Save(*features)) + return false; + + m_features.Set(features); + return true; +} + void Editor::ClearAllLocalEdits() { - std::lock_guard lock(m_mutex); + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); - m_features.clear(); - Save(); + SaveTransaction(make_shared()); Invalidate(); } void Editor::OnMapDeregistered(platform::LocalCountryFile const & localFile) { - using TFeaturePair = decltype(m_features)::value_type; + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); - std::lock_guard lock(m_mutex); - // Cannot search by MwmId because country already removed. So, search by country name. - auto const matchedMwm = - find_if(begin(m_features), end(m_features), [&localFile](TFeaturePair const & item) { - return item.first.GetInfo()->GetCountryName() == localFile.GetCountryName(); - }); + using FeaturePair = FeaturesContainer::value_type; - if (m_features.end() != matchedMwm) + auto const findMwm = [](FeaturesContainer const & src, platform::LocalCountryFile const & localFile) { - m_features.erase(matchedMwm); - Save(); + // Cannot search by MwmId because country already removed. So, search by country name. + return find_if(cbegin(src), cend(src), [&localFile](FeaturePair const & item) { + return item.first.GetInfo()->GetCountryName() == localFile.GetCountryName(); + }); + }; + + auto const features = m_features.Get(); + auto const matchedMwm = findMwm(*features, localFile); + + if (features->cend() != matchedMwm) + { + auto editableFeatures = make_shared(*features); + editableFeatures->erase(findMwm(*editableFeatures, localFile)); + + SaveTransaction(editableFeatures); } } FeatureStatus Editor::GetFeatureStatus(MwmSet::MwmId const & mwmId, uint32_t index) const { - std::lock_guard lock(m_mutex); - - return GetFeatureStatusImpl(mwmId, index); + auto const features = m_features.Get(); + return GetFeatureStatusImpl(*features, mwmId, index); } FeatureStatus Editor::GetFeatureStatus(FeatureID const & fid) const { - std::lock_guard lock(m_mutex); - - return GetFeatureStatusImpl(fid.m_mwmId, fid.m_index); + auto const features = m_features.Get(); + return GetFeatureStatusImpl(*features, fid.m_mwmId, fid.m_index); } bool Editor::IsFeatureUploaded(MwmSet::MwmId const & mwmId, uint32_t index) const { - std::lock_guard lock(m_mutex); - return IsFeatureUploadedImpl(mwmId, index); + auto const features = m_features.Get(); + return IsFeatureUploadedImpl(*features, mwmId, index); } void Editor::DeleteFeature(FeatureID const & fid) { - std::lock_guard lock(m_mutex); + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); - auto const mwm = m_features.find(fid.m_mwmId); - if (mwm != m_features.end()) + auto const features = m_features.Get(); + auto editableFeatures = make_shared(*features); + + auto const mwm = editableFeatures->find(fid.m_mwmId); + + if (mwm != editableFeatures->end()) { auto const f = mwm->second.find(fid.m_index); // Created feature is deleted by removing all traces of it. if (f != mwm->second.end() && f->second.m_status == FeatureStatus::Created) { mwm->second.erase(f); + SaveTransaction(editableFeatures); return; } } - MarkFeatureWithStatus(fid, FeatureStatus::Deleted); - - // TODO(AlexZ): Synchronize Save call/make it on a separate thread. - Save(); + MarkFeatureWithStatus(*editableFeatures, fid, FeatureStatus::Deleted); + SaveTransaction(editableFeatures); Invalidate(); } @@ -389,12 +396,14 @@ bool Editor::IsCreatedFeature(FeatureID const & fid) /// either delete it or save and mark as `Modified' depending on upload status Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) { + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + FeatureID const & fid = emo.GetID(); FeatureTypeInfo fti; - std::lock_guard lock(m_mutex); + auto const features = m_features.Get(); - auto const featureStatus = GetFeatureStatusImpl(fid); + auto const featureStatus = GetFeatureStatusImpl(*features, fid.m_mwmId, fid.m_index); ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Obsolete, ("Obsolete feature cannot be modified.")); ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Deleted, ("Unexpected feature status.")); @@ -406,8 +415,11 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) if (featureStatus == FeatureStatus::Created) { - auto & editedFeatureInfo = m_features[fid.m_mwmId][fid.m_index]; - if (AreFeaturesEqualButStreet(fti.m_feature, editedFeatureInfo.m_feature) && + auto const & editedFeatureInfo = features->at(fid.m_mwmId).at(fid.m_index); + // Temporary solution because of FeatureType does not have constant getters. + // TODO(a): Use constant reference instead of copy. + auto feature = editedFeatureInfo.m_feature; + if (AreFeaturesEqualButStreet(fti.m_feature, feature) && emo.GetStreet().m_defaultName == editedFeatureInfo.m_street) { return NothingWasChanged; @@ -426,7 +438,7 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) fti.m_feature = featureStatus == FeatureStatus::Untouched ? *originalFeaturePtr - : m_features[fid.m_mwmId][fid.m_index].m_feature; + : features->at(fid.m_mwmId).at(fid.m_index).m_feature; fti.m_feature.ReplaceBy(emo); bool const sameAsInMWM = AreFeaturesEqualButStreet(fti.m_feature, *originalFeaturePtr) && @@ -435,8 +447,11 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) if (featureStatus != FeatureStatus::Untouched) { // A feature was modified and equals to the one in editor. - auto & editedFeatureInfo = m_features[fid.m_mwmId][fid.m_index]; - if (AreFeaturesEqualButStreet(fti.m_feature, editedFeatureInfo.m_feature) && + auto const & editedFeatureInfo = features->at(fid.m_mwmId).at(fid.m_index); + // Temporary solution because of FeatureType does not have constant getters. + // TODO(a): Use constant reference instead of copy. + auto feature = editedFeatureInfo.m_feature; + if (AreFeaturesEqualButStreet(fti.m_feature, feature) && emo.GetStreet().m_defaultName == editedFeatureInfo.m_street) { return NothingWasChanged; @@ -473,32 +488,33 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) // Reset upload status so already uploaded features can be uploaded again after modification. fti.m_uploadStatus = {}; - m_features[fid.m_mwmId][fid.m_index] = move(fti); - // TODO(AlexZ): Synchronize Save call/make it on a separate thread. - bool const savedSuccessfully = Save(); + auto editableFeatures = make_shared(*features); + (*editableFeatures)[fid.m_mwmId][fid.m_index] = move(fti); + + bool const savedSuccessfully = SaveTransaction(editableFeatures); + Invalidate(); return savedSuccessfully ? SavedSuccessfully : NoFreeSpaceError; } bool Editor::RollBackChanges(FeatureID const & fid) { - std::lock_guard lock(m_mutex); + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); - if (IsFeatureUploadedImpl(fid.m_mwmId, fid.m_index)) + if (IsFeatureUploaded(fid.m_mwmId, fid.m_index)) return false; return RemoveFeature(fid); } -void Editor::ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, - FeatureIndexFunctor const & f, m2::RectD const & rect, - int /*scale*/) const +void Editor::ForEachCreatedFeature(MwmSet::MwmId const & id, FeatureIndexFunctor const & f, + m2::RectD const & rect, int /*scale*/) const { - std::unique_lock lock(m_mutex); + auto const features = m_features.Get(); - auto const mwmFound = m_features.find(id); - if (mwmFound == m_features.end()) + auto const mwmFound = features->find(id); + if (mwmFound == features->cend()) return; // Process only new (created) features. @@ -510,10 +526,8 @@ void Editor::ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, // Temporary solution because of FeatureType does not have constant getters. // TODO(a): Use constant reference instead of copy. auto feature = ftInfo.m_feature; - lock.unlock(); - if(rect.IsPointInside(feature.GetCenter())) + if (rect.IsPointInside(feature.GetCenter())) f(index.first); - lock.lock(); } } } @@ -521,9 +535,8 @@ void Editor::ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, bool Editor::GetEditedFeature(MwmSet::MwmId const & mwmId, uint32_t index, FeatureType & outFeature) const { - std::lock_guard lock(m_mutex); - - auto const * featureInfo = GetFeatureTypeInfo(mwmId, index); + auto const features = m_features.Get(); + auto const * featureInfo = GetFeatureTypeInfo(*features, mwmId, index); if (featureInfo == nullptr) return false; @@ -538,9 +551,8 @@ bool Editor::GetEditedFeature(FeatureID const & fid, FeatureType & outFeature) c bool Editor::GetEditedFeatureStreet(FeatureID const & fid, string & outFeatureStreet) const { - std::lock_guard lock(m_mutex); - - auto const * featureInfo = GetFeatureTypeInfo(fid.m_mwmId, fid.m_index); + auto const features = m_features.Get(); + auto const * featureInfo = GetFeatureTypeInfo(*features, fid.m_mwmId, fid.m_index); if (featureInfo == nullptr) return false; @@ -551,41 +563,44 @@ bool Editor::GetEditedFeatureStreet(FeatureID const & fid, string & outFeatureSt vector Editor::GetFeaturesByStatus(MwmSet::MwmId const & mwmId, FeatureStatus status) const { - std::lock_guard lock(m_mutex); + auto const features = m_features.Get(); + + vector result; + auto const matchedMwm = features->find(mwmId); + if (matchedMwm == features->cend()) + return result; - vector features; - auto const matchedMwm = m_features.find(mwmId); - if (matchedMwm == m_features.end()) - return features; for (auto const & index : matchedMwm->second) { if (index.second.m_status == status) - features.push_back(index.first); + result.push_back(index.first); } - sort(features.begin(), features.end()); - return features; + sort(result.begin(), result.end()); + return result; } EditableProperties Editor::GetEditableProperties(FeatureType & feature) const { - std::lock_guard lock(m_mutex); + auto const features = m_features.Get(); + + auto const & fid = feature.GetID(); + auto const featureStatus = GetFeatureStatusImpl(*features, fid.m_mwmId, fid.m_index); ASSERT(version::IsSingleMwm(feature.GetID().m_mwmId.GetInfo()->m_version.GetVersion()), ("Edit mode should be available only on new data")); - - ASSERT(GetFeatureStatusImpl(feature.GetID()) != FeatureStatus::Obsolete, + ASSERT(featureStatus != FeatureStatus::Obsolete, ("Edit mode should not be available on obsolete features")); // TODO(mgsergio): Check if feature is in the area where editing is disabled in the config. auto editableProperties = GetEditablePropertiesForTypes(feature::TypesHolder(feature)); // Disable opening hours editing if opening hours cannot be parsed. - if (GetFeatureStatusImpl(feature.GetID()) != FeatureStatus::Created) + if (featureStatus != FeatureStatus::Created) { - auto const originalFeaturePtr = GetOriginalFeature(feature.GetID()); + auto const originalFeaturePtr = GetOriginalFeature(fid); if (!originalFeaturePtr) { - LOG(LERROR, ("A feature with id", feature.GetID(), "cannot be loaded.")); + LOG(LERROR, ("A feature with id", fid, "cannot be loaded.")); alohalytics::LogEvent("Editor_MissingFeature_Error"); return {}; } @@ -604,7 +619,7 @@ EditableProperties Editor::GetEditableProperties(FeatureType & feature) const return editableProperties; } -// private + EditableProperties Editor::GetEditablePropertiesForTypes(feature::TypesHolder const & types) const { editor::TypeAggregatedDescription desc; @@ -618,17 +633,16 @@ bool Editor::HaveMapEditsOrNotesToUpload() const if (m_notes->NotUploadedNotesCount() != 0) return true; - std::lock_guard lock(m_mutex); - - return HaveMapEditsToUpload(); + auto const features = m_features.Get(); + return HaveMapEditsToUpload(*features); } bool Editor::HaveMapEditsToUpload(MwmSet::MwmId const & mwmId) const { - std::lock_guard lock(m_mutex); + auto const features = m_features.Get(); - auto const found = m_features.find(mwmId); - if (found != m_features.end()) + auto const found = features->find(mwmId); + if (found != features->cend()) { for (auto const & index : found->second) { @@ -648,14 +662,12 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT m_notes->Upload(OsmOAuth::ServerAuth({key, secret})); } - { - std::lock_guard lock(m_mutex); + auto const features = m_features.Get(); - if (!HaveMapEditsToUpload()) - { - LOG(LDEBUG, ("There are no local edits to upload.")); - return; - } + if (!HaveMapEditsToUpload(*features)) + { + LOG(LDEBUG, ("There are no local edits to upload.")); + return; } alohalytics::LogEvent("Editor_DataSync_started"); @@ -664,19 +676,22 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT { int uploadedFeaturesCount = 0, errorsCount = 0; ChangesetWrapper changeset({key, secret}, tags); + auto const features = m_features.Get(); + for (auto const & id : *features) { - std::unique_lock guard(m_mutex); - - for (auto & id : m_features) - { - for (auto & index : id.second) + for (auto const & index : id.second) { - FeatureTypeInfo & fti = index.second; + FeatureTypeInfo const & fti = index.second; // Do not process already uploaded features or those failed permanently. if (!NeedsUpload(fti.m_uploadStatus)) continue; + // TODO(a): Use UploadInfo as part of FeatureTypeInfo. + UploadInfo uploadInfo = {fti.m_uploadAttemptTimestamp, fti.m_uploadStatus, fti.m_uploadError}; + // Temporary solution because of FeatureType does not have constant getters. + // TODO(a): Use constant reference instead of copy. + auto featureData = fti.m_feature; string ourDebugFeatureString; try @@ -684,11 +699,10 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT switch (fti.m_status) { case FeatureStatus::Untouched: CHECK(false, ("It's impossible.")); continue; - case FeatureStatus::Obsolete: - continue; // Obsolete features will be deleted by OSMers. + case FeatureStatus::Obsolete: continue; // Obsolete features will be deleted by OSMers. case FeatureStatus::Created: { - XMLFeature feature = editor::ToXML(fti.m_feature, true); + XMLFeature feature = editor::ToXML(featureData, true); if (!fti.m_street.empty()) feature.SetTagValue(kAddrStreetTag, fti.m_street); ourDebugFeatureString = DebugPrint(feature); @@ -697,11 +711,9 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT ("Linear and area features creation is not supported yet.")); try { - auto const center = fti.m_feature.GetCenter(); + auto const center = featureData.GetCenter(); - guard.unlock(); XMLFeature osmFeature = changeset.GetMatchingNodeFeatureFromOSM(center); - guard.lock(); // If we are here, it means that object already exists at the given point. // To avoid nodes duplication, merge and apply changes to it instead of creating an new one. XMLFeature const osmFeatureCopy = osmFeature; @@ -716,24 +728,18 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT else { LOG(LDEBUG, ("Create case: uploading patched feature", osmFeature)); - guard.unlock(); changeset.Modify(osmFeature); - guard.lock(); } } catch (ChangesetWrapper::OsmObjectWasDeletedException const &) { // Object was never created by anyone else - it's safe to create it. - guard.unlock(); changeset.Create(feature); - guard.lock(); } catch (ChangesetWrapper::EmptyFeatureException const &) { // There is another node nearby, but it should be safe to create a new one. - guard.unlock(); changeset.Create(feature); - guard.lock(); } catch (...) { @@ -747,7 +753,7 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT { // Do not serialize feature's type to avoid breaking OSM data. // TODO: Implement correct types matching when we support modifying existing feature types. - XMLFeature feature = editor::ToXML(fti.m_feature, false); + XMLFeature feature = editor::ToXML(featureData, false); if (!fti.m_street.empty()) feature.SetTagValue(kAddrStreetTag, fti.m_street); ourDebugFeatureString = DebugPrint(feature); @@ -757,29 +763,27 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT { LOG(LERROR, ("A feature with id", fti.m_feature.GetID(), "cannot be loaded.")); alohalytics::LogEvent("Editor_MissingFeature_Error"); - RemoveFeatureIfExists(fti.m_feature.GetID()); + GetPlatform().RunTask(Platform::Thread::Gui, [this, fid = fti.m_feature.GetID()]() + { + RemoveFeatureIfExists(fid); + }); continue; } - guard.unlock(); XMLFeature osmFeature = GetMatchingFeatureFromOSM(changeset, *originalFeaturePtr); - guard.lock(); XMLFeature const osmFeatureCopy = osmFeature; osmFeature.ApplyPatch(feature); // Check to avoid uploading duplicates into OSM. if (osmFeature == osmFeatureCopy) { - LOG(LWARNING, ("Local changes are equal to OSM, feature has not been uploaded.", - osmFeatureCopy)); + LOG(LWARNING, ("Local changes are equal to OSM, feature has not been uploaded.", osmFeatureCopy)); // Don't delete this local change right now for user to see it in profile. // It will be automatically deleted by migration code on the next maps update. } else { LOG(LDEBUG, ("Uploading patched feature", osmFeature)); - guard.unlock(); changeset.Modify(osmFeature); - guard.lock(); } } break; @@ -790,45 +794,46 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT { LOG(LERROR, ("A feature with id", fti.m_feature.GetID(), "cannot be loaded.")); alohalytics::LogEvent("Editor_MissingFeature_Error"); - RemoveFeatureIfExists(fti.m_feature.GetID()); + GetPlatform().RunTask(Platform::Thread::Gui, [this, fid = fti.m_feature.GetID()]() + { + RemoveFeatureIfExists(fid); + }); continue; } - guard.unlock(); changeset.Delete(GetMatchingFeatureFromOSM(changeset, *originalFeaturePtr)); - guard.lock(); break; } - fti.m_uploadStatus = kUploaded; - fti.m_uploadError.clear(); + uploadInfo.m_uploadStatus = kUploaded; + uploadInfo.m_uploadError.clear(); ++uploadedFeaturesCount; } catch (ChangesetWrapper::OsmObjectWasDeletedException const & ex) { - fti.m_uploadStatus = kDeletedFromOSMServer; - fti.m_uploadError = ex.Msg(); + uploadInfo.m_uploadStatus = kDeletedFromOSMServer; + uploadInfo.m_uploadError = ex.Msg(); ++errorsCount; LOG(LWARNING, (ex.what())); } catch (ChangesetWrapper::EmptyFeatureException const & ex) { - fti.m_uploadStatus = kMatchedFeatureIsEmpty; - fti.m_uploadError = ex.Msg(); + uploadInfo.m_uploadStatus = kMatchedFeatureIsEmpty; + uploadInfo.m_uploadError = ex.Msg(); ++errorsCount; LOG(LWARNING, (ex.what())); } catch (RootException const & ex) { - fti.m_uploadStatus = kNeedsRetry; - fti.m_uploadError = ex.Msg(); + uploadInfo.m_uploadStatus = kNeedsRetry; + uploadInfo.m_uploadError = ex.Msg(); ++errorsCount; LOG(LWARNING, (ex.what())); } // TODO(AlexZ): Use timestamp from the server. - fti.m_uploadAttemptTimestamp = time(nullptr); + uploadInfo.m_uploadAttemptTimestamp = time(nullptr); - if (fti.m_uploadStatus != kUploaded) + if (uploadInfo.m_uploadStatus != kUploaded) { - ms::LatLon const ll = MercatorBounds::ToLatLon(feature::GetCenter(fti.m_feature)); + ms::LatLon const ll = MercatorBounds::ToLatLon(feature::GetCenter(featureData)); alohalytics::LogEvent( "Editor_DataSync_error", {{"type", fti.m_uploadStatus}, @@ -838,11 +843,13 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT {"mwm_version", strings::to_string(fti.m_feature.GetID().GetMwmVersion())}}, alohalytics::Location::FromLatLon(ll.lat, ll.lon)); } - // Call Save every time we modify each feature's information. - SaveUploadedInformation(fti); + GetPlatform().RunTask(Platform::Thread::Gui, [this, id = fti.m_feature.GetID(), uploadInfo]() + { + // Call Save every time we modify each feature's information. + SaveUploadedInformation(id, uploadInfo); + }); } - } // loop scope - } // unique_lock scope + } alohalytics::LogEvent("Editor_DataSync_finished", {{"errors", strings::to_string(errorsCount)}, @@ -866,20 +873,27 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT future = async(launch::async, upload, key, secret, tags, callback); } -void Editor::SaveUploadedInformation(FeatureTypeInfo const & fromUploader) +void Editor::SaveUploadedInformation(FeatureID const & fid, UploadInfo const & uploadInfo) { - FeatureID const & fid = fromUploader.m_feature.GetID(); - auto id = m_features.find(fid.m_mwmId); - if (id == m_features.end()) - return; // Rare case: feature was deleted at the time of changes uploading. + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + + auto const features = m_features.Get(); + auto editableFeatures = make_shared(*features); + + auto id = editableFeatures->find(fid.m_mwmId); + if (id == editableFeatures->end()) + return; // rare case: feature was deleted at the time of changes uploading + auto index = id->second.find(fid.m_index); if (index == id->second.end()) - return; // Rare case: feature was deleted at the time of changes uploading. + return; // rare case: feature was deleted at the time of changes uploading + auto & fti = index->second; - fti.m_uploadAttemptTimestamp = fromUploader.m_uploadAttemptTimestamp; - fti.m_uploadStatus = fromUploader.m_uploadStatus; - fti.m_uploadError = fromUploader.m_uploadError; - Save(); + fti.m_uploadAttemptTimestamp = uploadInfo.m_uploadAttemptTimestamp; + fti.m_uploadStatus = uploadInfo.m_uploadStatus; + fti.m_uploadError = uploadInfo.m_uploadError; + + SaveTransaction(editableFeatures); } bool Editor::FillFeatureInfo(FeatureStatus status, XMLFeature const & xml, FeatureID const & fid, @@ -916,48 +930,41 @@ bool Editor::FillFeatureInfo(FeatureStatus status, XMLFeature const & xml, Featu return true; } -// Macros is used to avoid code duplication. -#define GET_FEATURE_TYPE_INFO_BODY \ - do \ - { \ - /* TODO(mgsergio): machedMwm should be synchronized. */ \ - auto const matchedMwm = m_features.find(mwmId); \ - if (matchedMwm == m_features.end()) \ - return nullptr; \ - \ - auto const matchedIndex = matchedMwm->second.find(index); \ - if (matchedIndex == matchedMwm->second.end()) \ - return nullptr; \ - \ - /* TODO(AlexZ): Should we process deleted/created features as well?*/ \ - return &matchedIndex->second; \ - } while (false) - -Editor::FeatureTypeInfo const * Editor::GetFeatureTypeInfo(MwmSet::MwmId const & mwmId, +Editor::FeatureTypeInfo const * Editor::GetFeatureTypeInfo(FeaturesContainer const & features, + MwmSet::MwmId const & mwmId, uint32_t index) const { - GET_FEATURE_TYPE_INFO_BODY; + auto const matchedMwm = features.find(mwmId); + if (matchedMwm == features.cend()) + return nullptr; + + auto const matchedIndex = matchedMwm->second.find(index); + if (matchedIndex == matchedMwm->second.cend()) + return nullptr; + + /* TODO(AlexZ): Should we process deleted/created features as well?*/ + return &matchedIndex->second; } -Editor::FeatureTypeInfo * Editor::GetFeatureTypeInfo(MwmSet::MwmId const & mwmId, uint32_t index) +bool Editor::RemoveFeatureIfExists(FeatureID const & fid) { - GET_FEATURE_TYPE_INFO_BODY; -} + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); -#undef GET_FEATURE_TYPE_INFO_BODY + auto const features = m_features.Get(); + auto editableFeatures = make_shared(*features); -void Editor::RemoveFeatureIfExists(FeatureID const & fid) -{ - auto matchedMwm = m_features.find(fid.m_mwmId); - if (matchedMwm == m_features.end()) - return; + auto matchedMwm = editableFeatures->find(fid.m_mwmId); + if (matchedMwm == editableFeatures->end()) + return true; auto matchedIndex = matchedMwm->second.find(fid.m_index); if (matchedIndex != matchedMwm->second.end()) matchedMwm->second.erase(matchedIndex); if (matchedMwm->second.empty()) - m_features.erase(matchedMwm); + editableFeatures->erase(matchedMwm); + + return SaveTransaction(editableFeatures); } void Editor::Invalidate() @@ -968,33 +975,44 @@ void Editor::Invalidate() bool Editor::MarkFeatureAsObsolete(FeatureID const & fid) { - auto const featureStatus = GetFeatureStatusImpl(fid); + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + + auto const features = m_features.Get(); + auto editableFeatures = make_shared(*features); + + auto const featureStatus = GetFeatureStatusImpl(*editableFeatures, fid.m_mwmId, fid.m_index); if (featureStatus != FeatureStatus::Untouched && featureStatus != FeatureStatus::Modified) { ASSERT(false, ("Only untouched and modified features can be made obsolete")); return false; } - MarkFeatureWithStatus(fid, FeatureStatus::Obsolete); - + MarkFeatureWithStatus(*editableFeatures, fid, FeatureStatus::Obsolete); + auto const result = SaveTransaction(editableFeatures); Invalidate(); - return Save(); + + return result; } bool Editor::RemoveFeature(FeatureID const & fid) { - RemoveFeatureIfExists(fid); - Invalidate(); - return Save(); + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + + auto const result = RemoveFeatureIfExists(fid); + + if (result) + Invalidate(); + + return result; } Editor::Stats Editor::GetStats() const { - std::lock_guard lock(m_mutex); - Stats stats; LOG(LDEBUG, ("Edited features status:")); - for (auto & id : m_features) + + auto const features = m_features.Get(); + for (auto const & id : *features) { for (auto & index : id.second) { @@ -1025,14 +1043,15 @@ NewFeatureCategories Editor::GetNewFeatureCategories() const return NewFeatureCategories(*(m_config.Get())); } -FeatureID Editor::GenerateNewFeatureId(MwmSet::MwmId const & id) const +FeatureID Editor::GenerateNewFeatureId(FeaturesContainer const & features, + MwmSet::MwmId const & id) const { - DECLARE_AND_CHECK_THREAD_CHECKER("GenerateNewFeatureId is single-threaded."); - // TODO(vng): Double-check if new feature indexes should uninterruptedly continue after existing - // indexes in mwm file. + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + uint32_t featureIndex = feature::FakeFeatureIds::kEditorCreatedFeaturesStart; - auto const found = m_features.find(id); - if (found != m_features.end()) + + auto const found = features.find(id); + if (found != features.cend()) { // Scan all already created features and choose next available ID. for (auto const & feature : found->second) @@ -1056,10 +1075,8 @@ bool Editor::CreatePoint(uint32_t type, m2::PointD const & mercator, MwmSet::Mwm return false; } - std::lock_guard lock(m_mutex); - outFeature.SetMercator(mercator); - outFeature.SetID(GenerateNewFeatureId(id)); + outFeature.SetID(GenerateNewFeatureId(*(m_features.Get()), id)); outFeature.SetType(type); outFeature.SetEditableProperties(GetEditablePropertiesForTypes(outFeature.GetTypes())); // Only point type features can be created at the moment. @@ -1071,6 +1088,8 @@ void Editor::CreateNote(ms::LatLon const & latLon, FeatureID const & fid, feature::TypesHolder const & holder, string const & defaultName, NoteProblemType const type, string const & note) { + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + auto const version = GetMwmCreationTimeByMwmId(fid.m_mwmId); auto const stringVersion = my::TimestampToString(my::SecondsSinceEpochToTimeT(version)); ostringstream sstr; @@ -1083,12 +1102,12 @@ void Editor::CreateNote(ms::LatLon const & latLon, FeatureID const & fid, { case NoteProblemType::PlaceDoesNotExist: { - std::lock_guard lock(m_mutex); - sstr << kPlaceDoesNotExistMessage << endl; - auto const isCreated = GetFeatureStatusImpl(fid) == FeatureStatus::Created; + auto const features = m_features.Get(); + auto const isCreated = + GetFeatureStatusImpl(*features, fid.m_mwmId, fid.m_index) == FeatureStatus::Created; auto const createdAndUploaded = - (isCreated && IsFeatureUploadedImpl(fid.m_mwmId, fid.m_index)); + (isCreated && IsFeatureUploadedImpl(*features, fid.m_mwmId, fid.m_index)); CHECK(!isCreated || createdAndUploaded, ()); if (createdAndUploaded) @@ -1101,6 +1120,9 @@ void Editor::CreateNote(ms::LatLon const & latLon, FeatureID const & fid, case NoteProblemType::General: break; } + if (!canCreate) + return; + if (defaultName.empty()) sstr << "POI has no name" << endl; else @@ -1115,13 +1137,15 @@ void Editor::CreateNote(ms::LatLon const & latLon, FeatureID const & fid, sstr << "OSM data version: " << stringVersion << endl; - if (canCreate) - m_notes->CreateNote(latLon, sstr.str()); + m_notes->CreateNote(latLon, sstr.str()); } -void Editor::MarkFeatureWithStatus(FeatureID const & fid, FeatureStatus status) +void Editor::MarkFeatureWithStatus(FeaturesContainer & editableFeatures, FeatureID const & fid, + FeatureStatus status) { - auto & fti = m_features[fid.m_mwmId][fid.m_index]; + CHECK_THREAD_CHECKER(MainThreadChecker, ("")); + + auto & fti = editableFeatures[fid.m_mwmId][fid.m_index]; auto const originalFeaturePtr = GetOriginalFeature(fid); @@ -1177,7 +1201,8 @@ void Editor::ForEachFeatureAtPoint(FeatureTypeFn && fn, m2::PointD const & point m_delegate->ForEachFeatureAtPoint(move(fn), point); } -FeatureID Editor::GetFeatureIdByXmlFeature(XMLFeature const & xml, MwmSet::MwmId const & mwmId, +FeatureID Editor::GetFeatureIdByXmlFeature(FeaturesContainer const & features, + XMLFeature const & xml, MwmSet::MwmId const & mwmId, FeatureStatus status, bool needMigrate) const { ForEachFeaturesNearByFn forEach = [this](FeatureTypeFn && fn, m2::PointD const & point) { @@ -1187,18 +1212,16 @@ FeatureID Editor::GetFeatureIdByXmlFeature(XMLFeature const & xml, MwmSet::MwmId // TODO(mgsergio): Deleted features are not properly handled yet. if (needMigrate) { - return editor::MigrateFeatureIndex(forEach, xml, status, - [this, &mwmId] - { - std::lock_guard lock(m_mutex); - return GenerateNewFeatureId(mwmId); - }); + return editor::MigrateFeatureIndex(forEach, xml, status, [this, &mwmId, &features] { + return GenerateNewFeatureId(features, mwmId); + }); } return FeatureID(mwmId, xml.GetMWMFeatureIndex()); } -void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, bool needMigrate) +void Editor::LoadMwmEdits(FeaturesContainer & loadedFeatures, xml_node const & mwm, + MwmSet::MwmId const & mwmId, bool needMigrate) { LogHelper logHelper(mwmId); @@ -1210,7 +1233,8 @@ void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, boo { XMLFeature const xml(nodeOrWay.node()); - auto const fid = GetFeatureIdByXmlFeature(xml, mwmId, section.m_status, needMigrate); + auto const fid = + GetFeatureIdByXmlFeature(loadedFeatures, xml, mwmId, section.m_status, needMigrate); // Remove obsolete changes during migration. if (needMigrate && IsObsolete(xml, fid)) @@ -1222,10 +1246,7 @@ void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, boo logHelper.OnStatus(section.m_status); - { - std::lock_guard lock(m_mutex); - m_features[fid.m_mwmId].emplace(fid.m_index, move(fti)); - } + loadedFeatures[fid.m_mwmId].emplace(fid.m_index, move(fti)); } catch (editor::XMLFeatureError const & ex) { @@ -1241,33 +1262,9 @@ void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, boo } } -FeatureStatus Editor::GetFeatureStatusImpl(MwmSet::MwmId const & mwmId, uint32_t index) const +bool Editor::HaveMapEditsToUpload(FeaturesContainer const & features) const { - // Most popular case optimization. - if (m_features.empty()) - return FeatureStatus::Untouched; - - auto const * featureInfo = GetFeatureTypeInfo(mwmId, index); - if (featureInfo == nullptr) - return FeatureStatus::Untouched; - - return featureInfo->m_status; -} - -FeatureStatus Editor::GetFeatureStatusImpl(FeatureID const & fid) const -{ - return GetFeatureStatusImpl(fid.m_mwmId, fid.m_index); -} - -bool Editor::IsFeatureUploadedImpl(MwmSet::MwmId const & mwmId, uint32_t index) const -{ - auto const * info = GetFeatureTypeInfo(mwmId, index); - return info && info->m_uploadStatus == kUploaded; -} - -bool Editor::HaveMapEditsToUpload() const -{ - for (auto const & id : m_features) + for (auto const & id : features) { for (auto const & index : id.second) { @@ -1278,6 +1275,27 @@ bool Editor::HaveMapEditsToUpload() const return false; } +FeatureStatus Editor::GetFeatureStatusImpl(FeaturesContainer const & features, + MwmSet::MwmId const & mwmId, uint32_t index) const +{ + // Most popular case optimization. + if (features.empty()) + return FeatureStatus::Untouched; + + auto const * featureInfo = GetFeatureTypeInfo(features, mwmId, index); + if (featureInfo == nullptr) + return FeatureStatus::Untouched; + + return featureInfo->m_status; +} + +bool Editor::IsFeatureUploadedImpl(FeaturesContainer const & features, MwmSet::MwmId const & mwmId, + uint32_t index) const +{ + auto const * info = GetFeatureTypeInfo(features, mwmId, index); + return info && info->m_uploadStatus == kUploaded; +} + const char * const Editor::kPlaceDoesNotExistMessage = "The place has gone or never existed. This is an auto-generated note from MAPS.ME application: " "a user reports a POI that is visible on a map (which can be outdated), but cannot be found on " diff --git a/editor/osm_editor.hpp b/editor/osm_editor.hpp index 79dec05d13..f156a680ce 100644 --- a/editor/osm_editor.hpp +++ b/editor/osm_editor.hpp @@ -15,12 +15,12 @@ #include "geometry/rect2d.hpp" +#include "base/atomic_shared_ptr.hpp" #include "base/timer.hpp" #include "std/ctime.hpp" #include "std/function.hpp" #include "std/map.hpp" -#include "std/mutex.hpp" #include "std/string.hpp" #include "std/vector.hpp" @@ -38,6 +38,8 @@ class XMLFeature; namespace osm { +// NOTE: this class is thead-safe for read operations, +// but write operations should be called on main thread only. class Editor final : public MwmSet::Observer { friend class editor::testing::EditorTest; @@ -96,12 +98,11 @@ public: static Editor & Instance(); - inline void SetDelegate(unique_ptr delegate) { m_delegate = move(delegate); } + void SetDelegate(unique_ptr delegate) { m_delegate = move(delegate); } - inline void SetStorageForTesting(unique_ptr storage) - { - m_storage = move(storage); - } + void SetStorageForTesting(unique_ptr storage) { m_storage = move(storage); } + + void ResetNotes() { m_notes = editor::Notes::MakeNotes(); } void SetDefaultStorage(); @@ -120,10 +121,8 @@ public: void OnMapDeregistered(platform::LocalCountryFile const & localFile) override; using FeatureIndexFunctor = function; - void ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, FeatureIndexFunctor const & f, - m2::RectD const & rect, int scale) const; - - // TODO(mgsergio): Unify feature functions signatures. + void ForEachCreatedFeature(MwmSet::MwmId const & id, FeatureIndexFunctor const & f, + m2::RectD const & rect, int scale) const; /// Easy way to check if a feature was deleted, modified, created or not changed at all. FeatureStatus GetFeatureStatus(MwmSet::MwmId const & mwmId, uint32_t index) const; @@ -183,6 +182,15 @@ public: static bool IsCreatedFeature(FeatureID const & fid); private: + // TODO(a): Use this structure as part of FeatureTypeInfo. + struct UploadInfo + { + time_t m_uploadAttemptTimestamp = my::INVALID_TIME_STAMP; + /// Is empty if upload has never occured or one of k* constants above otherwise. + string m_uploadStatus; + string m_uploadError; + }; + struct FeatureTypeInfo { FeatureStatus m_status; @@ -197,9 +205,12 @@ private: string m_uploadError; }; + using FeaturesContainer = map>; + /// @returns false if fails. - bool Save() const; - void RemoveFeatureIfExists(FeatureID const & fid); + bool Save(FeaturesContainer const & features) const; + bool SaveTransaction(shared_ptr const & features); + bool RemoveFeatureIfExists(FeatureID const & fid); /// Notify framework that something has changed and should be redisplayed. void Invalidate(); @@ -207,38 +218,41 @@ private: bool MarkFeatureAsObsolete(FeatureID const & fid); bool RemoveFeature(FeatureID const & fid); - FeatureID GenerateNewFeatureId(MwmSet::MwmId const & id) const; + FeatureID GenerateNewFeatureId(FeaturesContainer const & features, + MwmSet::MwmId const & id) const; EditableProperties GetEditablePropertiesForTypes(feature::TypesHolder const & types) const; bool FillFeatureInfo(FeatureStatus status, editor::XMLFeature const & xml, FeatureID const & fid, FeatureTypeInfo & fti) const; /// @returns pointer to m_features[id][index] if exists, nullptr otherwise. - FeatureTypeInfo const * GetFeatureTypeInfo(MwmSet::MwmId const & mwmId, uint32_t index) const; - FeatureTypeInfo * GetFeatureTypeInfo(MwmSet::MwmId const & mwmId, uint32_t index); - void SaveUploadedInformation(FeatureTypeInfo const & fromUploader); + FeatureTypeInfo const * GetFeatureTypeInfo(FeaturesContainer const & features, + MwmSet::MwmId const & mwmId, uint32_t index) const; + void SaveUploadedInformation(FeatureID const & fid, UploadInfo const & fromUploader); - void MarkFeatureWithStatus(FeatureID const & fid, FeatureStatus status); + void MarkFeatureWithStatus(FeaturesContainer & editableFeatures, FeatureID const & fid, + FeatureStatus status); // These methods are just checked wrappers around Delegate. MwmSet::MwmId GetMwmIdByMapName(string const & name); unique_ptr GetOriginalFeature(FeatureID const & fid) const; string GetOriginalFeatureStreet(FeatureType & ft) const; void ForEachFeatureAtPoint(FeatureTypeFn && fn, m2::PointD const & point) const; - FeatureID GetFeatureIdByXmlFeature(editor::XMLFeature const & xml, MwmSet::MwmId const & mwmId, + FeatureID GetFeatureIdByXmlFeature(FeaturesContainer const & features, + editor::XMLFeature const & xml, MwmSet::MwmId const & mwmId, FeatureStatus status, bool needMigrate) const; - void LoadMwmEdits(pugi::xml_node const & mwm, MwmSet::MwmId const & mwmId, bool needMigrate); + void LoadMwmEdits(FeaturesContainer & loadedFeatures, pugi::xml_node const & mwm, + MwmSet::MwmId const & mwmId, bool needMigrate); - FeatureStatus GetFeatureStatusImpl(MwmSet::MwmId const & mwmId, uint32_t index) const; - FeatureStatus GetFeatureStatusImpl(FeatureID const & fid) const; + bool HaveMapEditsToUpload(FeaturesContainer const & features) const; - bool IsFeatureUploadedImpl(MwmSet::MwmId const & mwmId, uint32_t index) const; + FeatureStatus GetFeatureStatusImpl(FeaturesContainer const & features, + MwmSet::MwmId const & mwmId, uint32_t index) const; - bool HaveMapEditsToUpload() const; - - mutable std::mutex m_mutex; + bool IsFeatureUploadedImpl(FeaturesContainer const & features, MwmSet::MwmId const & mwmId, + uint32_t index) const; /// Deleted, edited and created features. - map> m_features; + base::AtomicSharedPtr m_features; unique_ptr m_delegate; @@ -246,12 +260,14 @@ private: InvalidateFn m_invalidateFn; /// Contains information about what and how can be edited. - editor::EditorConfigWrapper m_config; + base::AtomicSharedPtr m_config; editor::ConfigLoader m_configLoader; /// Notes to be sent to osm. shared_ptr m_notes; unique_ptr m_storage; + + DECLARE_THREAD_CHECKER(MainThreadChecker); }; // class Editor } // namespace osm diff --git a/indexer/data_source.cpp b/indexer/data_source.cpp index 7056d8539f..30b290fb69 100644 --- a/indexer/data_source.cpp +++ b/indexer/data_source.cpp @@ -59,7 +59,7 @@ public: // Need to do it on a per-mwm basis, because Drape relies on features in a sorted order. // Touched (created, edited) features reading. auto f = [&](uint32_t i) { m_fn(i, *src); }; - src->ForEachInRectAndScale(cov.GetRect(), scale, f); + src->ForEachAdditionalFeature(cov.GetRect(), scale, f); } private: diff --git a/indexer/feature_source.cpp b/indexer/feature_source.cpp index 254065a0a6..1231d0541b 100644 --- a/indexer/feature_source.cpp +++ b/indexer/feature_source.cpp @@ -52,7 +52,7 @@ bool FeatureSource::GetModifiedFeature(uint32_t index, FeatureType & feature) co return false; } -void FeatureSource::ForEachInRectAndScale(m2::RectD const & rect, int scale, - function const & fn) const +void FeatureSource::ForEachAdditionalFeature(m2::RectD const & rect, int scale, + function const & fn) const { } diff --git a/indexer/feature_source.hpp b/indexer/feature_source.hpp index 6e22f4d939..af0dce2843 100644 --- a/indexer/feature_source.hpp +++ b/indexer/feature_source.hpp @@ -41,8 +41,8 @@ public: virtual bool GetModifiedFeature(uint32_t index, FeatureType & feature) const; - virtual void ForEachInRectAndScale(m2::RectD const & rect, int scale, - std::function const & fn) const; + virtual void ForEachAdditionalFeature(m2::RectD const & rect, int scale, + std::function const & fn) const; protected: MwmSet::MwmHandle const & m_handle; diff --git a/map/framework.cpp b/map/framework.cpp index 88f2788624..e3e177b837 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -392,6 +392,9 @@ Framework::Framework(FrameworkParams const & params) { m_startBackgroundTime = my::Timer::LocalTime(); + // Editor should be initialized before search to use correct thread for write operations. + osm::Editor & editor = osm::Editor::Instance(); + // Restore map style before classificator loading MapStyle mapStyle = kDefaultMapStyle; string mapStyleStr; @@ -482,8 +485,6 @@ Framework::Framework(FrameworkParams const & params) LOG(LINFO, ("System languages:", languages::GetPreferred())); - osm::Editor & editor = osm::Editor::Instance(); - editor.SetDelegate(make_unique(m_model.GetDataSource())); editor.SetInvalidateFn([this](){ InvalidateRect(GetCurrentViewport()); }); editor.LoadEdits(); diff --git a/search/mwm_context.hpp b/search/mwm_context.hpp index 69daedee52..64ecad854e 100644 --- a/search/mwm_context.hpp +++ b/search/mwm_context.hpp @@ -63,7 +63,7 @@ public: { FeatureType ft; if (GetFeature(index, ft)) - fn(ft); + fn(ft); }); }