[editor] Postponed removing edits for a deleted map.

Editor used to subscribe to the now-deleted OnMapUpdated event
in the MwmSet. The semantics of OnMapUpdated(new,old) has always been
OnMapDeregistered(old)+OnMapRegistered(new) (the order is unspecified)
but the Editor treated it differently: i.e. in the Editor, an Update
was treated as a separate event non-equivalent to the combination of
Register and Deregister.

After the change in the implementation of the MwmSet, the OnMapUpdated
call is gone and, as per MwmSet's documentation, is replaced with
OnMapDeregistered+OnMapRegistered. This changes the behaviour of
the Editor.

Before, the Editor did nothing on a Registered event (all edits for the
existing maps were loaded at startup), reloaded all edits on an Update
event (including the edits for the non-updated maps), and deleted the
edits for the deleted map on a Deregistered event. The reasons for
deletion of the edits are unclear but presumably we wanted to warn
the user to not delete the map until the edits have been uploaded, and
also some bugs were observed when redownloading a map for which edits had
not been deleted.

Now, the Editor loads the edits for a newly Registered map and does
nothing when a map is Deregistered. The changes for a deleted map will be
deleted on the next startup or when another map is downloaded (or updated).
The message we show to the user is not changed and it still says that the
edits are going to be deleted.

Note that with current design we need the mwm file to be present if we
want to upload the changes. Therefore, the collection-deletion of edits
for showing them on map is not decoupled from the collection-deletion
for the purpose of uploading them to OSM.

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 <memory>
 
@@ -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, ());
 }
 

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<long long>(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<FeaturesContainer>(*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))

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(uint32_t)>;
   void ForEachCreatedFeature(MwmSet::MwmId const & id, FeatureIndexFunctor const & f,