diff --git a/editor/editor_tests/osm_editor_test.cpp b/editor/editor_tests/osm_editor_test.cpp index 016ab78e2f..1dde414946 100644 --- a/editor/editor_tests/osm_editor_test.cpp +++ b/editor/editor_tests/osm_editor_test.cpp @@ -18,6 +18,8 @@ #include "platform/platform_tests_support/scoped_file.hpp" #include "base/file_name_utils.hpp" +#include "base/logging.hpp" +#include "base/scope_guard.hpp" #include @@ -569,8 +571,10 @@ void EditorTest::GetFeaturesByStatusTest() void EditorTest::OnMapDeregisteredTest() { auto & editor = osm::Editor::Instance(); + m_dataSource.AddObserver(editor); + SCOPE_GUARD(removeObs, [&] { m_dataSource.RemoveObserver(editor); }); - auto const gbMwmId = BuildMwm("GB", [](TestMwmBuilder & builder) + auto gbMwmId = BuildMwm("GB", [](TestMwmBuilder & builder) { TestCafe cafeLondon(m2::PointD(1.0, 1.0), "London Cafe", "en"); builder.Add(cafeLondon); @@ -582,6 +586,11 @@ void EditorTest::OnMapDeregisteredTest() builder.Add(cafeMoscow); }); + auto nzMwmId = BuildMwm("NZ", [](TestMwmBuilder & builder) + { + }); + m_dataSource.DeregisterMap(nzMwmId.GetInfo()->GetLocalFile().GetCountryFile()); + ForEachCafeAtPoint(m_dataSource, m2::PointD(1.0, 1.0), [](FeatureType & ft) { SetBuildingLevelsToOne(ft); @@ -592,17 +601,45 @@ void EditorTest::OnMapDeregisteredTest() SetBuildingLevelsToOne(ft); }); - TEST(!editor.m_features.Get()->empty(), ()); + TEST_EQUAL(editor.m_features.Get()->size(), 2, (editor.m_features.Get()->size())); + + { + platform::tests_support::AsyncGuiThread guiThread; + m_dataSource.DeregisterMap(gbMwmId.GetInfo()->GetLocalFile().GetCountryFile()); + } + // The map is deregistered but the edits are not deleted until + // LoadEdits() is called again, either on the next startup or + // on registering a new map. TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); { platform::tests_support::AsyncGuiThread guiThread; - editor.OnMapDeregistered(gbMwmId.GetInfo()->GetLocalFile()); + auto result = m_dataSource.RegisterMap(gbMwmId.GetInfo()->GetLocalFile()); + TEST_EQUAL(result.second, MwmSet::RegResult::Success, ()); + gbMwmId = result.first; + TEST(gbMwmId.IsAlive(), ()); } + // The same map was registered: the edits are still here. + TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); + { + platform::tests_support::AsyncGuiThread guiThread; + m_dataSource.DeregisterMap(gbMwmId.GetInfo()->GetLocalFile().GetCountryFile()); + } + TEST_EQUAL(editor.m_features.Get()->size(), 2, ()); + + { + platform::tests_support::AsyncGuiThread guiThread; + auto result = m_dataSource.RegisterMap(nzMwmId.GetInfo()->GetLocalFile()); + TEST_EQUAL(result.second, MwmSet::RegResult::Success, ()); + nzMwmId = result.first; + TEST(nzMwmId.IsAlive(), ()); + } + // Another map was registered: all edits are reloaded and + // the edits for the deleted map are removed. 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()); + bool const result = (editedMwmId != editor.m_features.Get()->end()); TEST(result, ()); } diff --git a/editor/osm_editor.cpp b/editor/osm_editor.cpp index 430d8056c0..733da6bc0d 100644 --- a/editor/osm_editor.cpp +++ b/editor/osm_editor.cpp @@ -244,6 +244,9 @@ bool Editor::Save(FeaturesContainer const & features) const root.append_attribute("format_version") = 1; for (auto const & mwm : features) { + if (!mwm.first.IsAlive()) + continue; + xml_node mwmNode = root.append_child(kXmlMwmNode); mwmNode.append_attribute("name") = mwm.first.GetInfo()->GetCountryName().c_str(); mwmNode.append_attribute("version") = static_cast(mwm.first.GetInfo()->GetVersion()); @@ -304,37 +307,10 @@ void Editor::ClearAllLocalEdits() void Editor::OnMapRegistered(platform::LocalCountryFile const & localFile) { + // todo(@a, @m) Reloading edits only for |localFile| should be enough. LoadEdits(); } -void Editor::OnMapDeregistered(platform::LocalCountryFile const & localFile) -{ - // Can be called on non-main-thread in case of simultaneous uploading process. - GetPlatform().RunTask(Platform::Thread::Gui, [this, localFile] - { - using FeaturePair = FeaturesContainer::value_type; - - auto const findMwm = [](FeaturesContainer const & src, platform::LocalCountryFile const & localFile) - { - // 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 { auto const features = m_features.Get(); @@ -622,6 +598,9 @@ bool Editor::HaveMapEditsOrNotesToUpload() const bool Editor::HaveMapEditsToUpload(MwmSet::MwmId const & mwmId) const { + if (!mwmId.IsAlive()) + return false; + auto const features = m_features.Get(); auto const found = features->find(mwmId); @@ -663,6 +642,9 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT for (auto const & id : *features) { + if (!id.first.IsAlive()) + continue; + for (auto const & index : id.second) { FeatureTypeInfo const & fti = index.second; @@ -1251,6 +1233,17 @@ bool Editor::HaveMapEditsToUpload(FeaturesContainer const & features) const { for (auto const & id : features) { + // Do not upload changes for a deleted map. + // todo(@a, @m) This should be unnecessary but unfortunately it's not. + // The mwm's feature is read right before the xml feature is uploaded to fill + // in missing fields such as precise feature geometry. This is an artefact + // of an old design decision; we have all the information needed for uploading + // already at the time when the xml feature is created and should not + // need to re-read it when uploading and when the corresponding mwm may have + // been deleted from disk. + if (!id.first.IsAlive()) + continue; + for (auto const & index : id.second) { if (NeedsUpload(index.second.m_uploadStatus)) diff --git a/editor/osm_editor.hpp b/editor/osm_editor.hpp index d3f45d581d..19ca945fe3 100644 --- a/editor/osm_editor.hpp +++ b/editor/osm_editor.hpp @@ -124,7 +124,6 @@ public: // MwmSet::Observer overrides: void OnMapRegistered(platform::LocalCountryFile const & localFile) override; - void OnMapDeregistered(platform::LocalCountryFile const & localFile) override; using FeatureIndexFunctor = std::function; void ForEachCreatedFeature(MwmSet::MwmId const & id, FeatureIndexFunctor const & f,