[editor] load map edits refactoring

1 files changed, 150 insertions, 102 deletions

diff --git a/indexer/osm_editor.cpp b/indexer/osm_editor.cpp
index 7d4998e379..ec988b4e76 100644
--- a/indexer/osm_editor.cpp
+++ b/indexer/osm_editor.cpp
@@ -66,6 +66,48 @@ constexpr char const * kRelationsAreNotSupported = "Relations are not supported
 constexpr char const * kNeedsRetry = "Needs Retry";
 constexpr char const * kWrongMatch = "Matched feature has no tags";
 
+struct XmlSection
+{
+  osm::Editor::FeatureStatus m_status;
+  std::string m_sectionName;
+};
+
+array<XmlSection, 4> const kXmlSections =
+{{
+   {osm::Editor::FeatureStatus::Deleted, kDeleteSection},
+   {osm::Editor::FeatureStatus::Modified, kModifySection},
+   {osm::Editor::FeatureStatus::Obsolete, kObsoleteSection},
+   {osm::Editor::FeatureStatus::Created, kCreateSection}
+}};
+
+struct LogHelper
+{
+  LogHelper(MwmSet::MwmId const & mwmId) : m_mwmId(mwmId) {}
+  ~LogHelper()
+  {
+    LOG(LINFO, ("For", m_mwmId, ". Was loaded", m_modified, "modified,", m_created, "created,",
+                m_deleted, "deleted and", m_obsolete, "obsolete features."));
+  }
+
+  void Increment(osm::Editor::FeatureStatus status)
+  {
+    switch (status)
+    {
+    case osm::Editor::FeatureStatus::Deleted: ++m_deleted; break;
+    case osm::Editor::FeatureStatus::Modified: ++m_modified; break;
+    case osm::Editor::FeatureStatus::Obsolete: ++m_obsolete; break;
+    case osm::Editor::FeatureStatus::Created: ++m_created; break;
+    case osm::Editor::FeatureStatus::Untouched: ASSERT(false, ());
+    }
+  }
+
+  int m_deleted = 0;
+  int m_obsolete = 0;
+  int m_modified = 0;
+  int m_created = 0;
+  MwmSet::MwmId const & m_mwmId;
+};
+
 bool NeedsUpload(string const & uploadStatus)
 {
   return uploadStatus != kUploaded &&
@@ -152,7 +194,7 @@ void Editor::SetDefaultStorage()
   m_storage = make_unique<editor::LocalStorage>();
 }
 
-void Editor::LoadMapEdits()
+void Editor::LoadEdits()
 {
   if (!m_delegate)
   {
@@ -164,121 +206,35 @@ void Editor::LoadMapEdits()
   if (!m_storage->Load(doc))
     return;
 
-  array<pair<FeatureStatus, char const *>, 4> const sections =
-  {{
-      {FeatureStatus::Deleted, kDeleteSection},
-      {FeatureStatus::Modified, kModifySection},
-      {FeatureStatus::Obsolete, kObsoleteSection},
-      {FeatureStatus::Created, kCreateSection}
-  }};
-  int deleted = 0, obsolete = 0, modified = 0, created = 0;
-
   bool needRewriteEdits = false;
 
   // TODO(mgsergio): synchronize access to m_features.
   m_features.clear();
-  for (xml_node mwm : doc.child(kXmlRootNode).children(kXmlMwmNode))
+  for (auto const & mwm : doc.child(kXmlRootNode).children(kXmlMwmNode))
   {
     string const mapName = mwm.attribute("name").as_string("");
     int64_t const mapVersion = mwm.attribute("version").as_llong(0);
-    MwmSet::MwmId const mwmId = GetMwmIdByMapName(mapName);
-    // TODO(mgsergio, AlexZ): Is it normal to have isMwmIdAlive and mapVersion
-    // NOT equal to mwmId.GetInfo()->GetVersion() at the same time?
-    auto const needMigrateEdits = !mwmId.IsAlive() || mapVersion != mwmId.GetInfo()->GetVersion();
-    needRewriteEdits |= needMigrateEdits;
+    auto const mwmId = GetMwmIdByMapName(mapName);
 
-    for (auto const & section : sections)
+    // TODO(mgsergio): A map could be renamed, we'll treat it as deleted.
+    if (!mwmId.IsAlive())
     {
-      for (auto const nodeOrWay : mwm.child(section.second).select_nodes("node|way"))
-      {
-        try
-        {
-          XMLFeature const xml(nodeOrWay.node());
-
-          // TODO(mgsergio): A map could be renamed, we'll treat it as deleted.
-          // The right thing to do is to try to migrate all changes anyway.
-          if (!mwmId.IsAlive())
-          {
-            LOG(LINFO, ("Mwm", mapName, "was deleted"));
-            goto SECTION_END;
-          }
-
-          TForEachFeaturesNearByFn forEach = [this](TFeatureTypeFn && fn,
-                                                    m2::PointD const & point) {
-            return ForEachFeatureAtPoint(move(fn), point);
-          };
-
-          // TODO(mgsergio): Deleted features are not properly handled yet.
-          auto const fid = needMigrateEdits
-                               ? editor::MigrateFeatureIndex(
-                                     forEach, xml, section.first,
-                                     [this, &mwmId] { return GenerateNewFeatureId(mwmId); })
-                               : FeatureID(mwmId, xml.GetMWMFeatureIndex());
-
-          // Remove obsolete changes during migration.
-          if (needMigrateEdits && IsObsolete(xml, fid))
-            continue;
-
-          FeatureTypeInfo fti;
-          if (section.first == FeatureStatus::Created)
-          {
-            fti.m_feature.FromXML(xml);
-          }
-          else
-          {
-            auto const originalFeaturePtr = GetOriginalFeature(fid);
-            if (!originalFeaturePtr)
-            {
-              LOG(LERROR, ("A feature with id", fid, "cannot be loaded."));
-              alohalytics::LogEvent("Editor_MissingFeature_Error");
-              goto SECTION_END;
-            }
-
-            fti.m_feature = *originalFeaturePtr;
-            fti.m_feature.ApplyPatch(xml);
-          }
+      LOG(LINFO, ("Mwm", mapName, "was deleted"));
+      needRewriteEdits = true;
+      continue;
+    }
 
-          fti.m_feature.SetID(fid);
-          fti.m_street = xml.GetTagValue(kAddrStreetTag);
+    // 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;
 
-          fti.m_modificationTimestamp = xml.GetModificationTime();
-          ASSERT_NOT_EQUAL(my::INVALID_TIME_STAMP, fti.m_modificationTimestamp, ());
-          fti.m_uploadAttemptTimestamp = xml.GetUploadTime();
-          fti.m_uploadStatus = xml.GetUploadStatus();
-          fti.m_uploadError = xml.GetUploadError();
-          fti.m_status = section.first;
-          switch (section.first)
-          {
-          case FeatureStatus::Deleted: ++deleted; break;
-          case FeatureStatus::Modified: ++modified; break;
-          case FeatureStatus::Obsolete: ++obsolete; break;
-          case FeatureStatus::Created: ++created; break;
-          case FeatureStatus::Untouched: ASSERT(false, ()); continue;
-          }
-          // Insert initialized structure at the end: exceptions are possible in above code.
-          m_features[fid.m_mwmId].emplace(fid.m_index, move(fti));
-        }
-        catch (editor::XMLFeatureError const & ex)
-        {
-          ostringstream s;
-          nodeOrWay.node().print(s, "  ");
-          LOG(LERROR, (ex.what(), "Can't create XMLFeature in section", section.second, s.str()));
-        }
-        catch (editor::MigrationError const & ex)
-        {
-          LOG(LWARNING, (ex.Msg(), "mwmId =", mwmId, XMLFeature(nodeOrWay.node())));
-        }
-      } // for nodes
-    } // for sections
- SECTION_END:
-    ;
+    LoadMwmEdits(mwm, mwmId, needMigrateEdits);
   } // for mwms
 
   // Save edits with new indexes and mwm version to avoid another migration on next startup.
   if (needRewriteEdits)
     Save();
-  LOG(LINFO, ("Loaded", modified, "modified,",
-              created, "created,", deleted, "deleted and", obsolete, "obsolete features."));
 }
 
 bool Editor::Save() const
@@ -930,6 +886,40 @@ void Editor::SaveUploadedInformation(FeatureTypeInfo const & fromUploader)
   Save();
 }
 
+bool Editor::FillFeatureInfo(FeatureStatus status, XMLFeature const & xml, FeatureID const & fid,
+                             FeatureTypeInfo & fti) const
+{
+  if (status == FeatureStatus::Created)
+  {
+    fti.m_feature.FromXML(xml);
+  }
+  else
+  {
+    auto const originalFeaturePtr = GetOriginalFeature(fid);
+    if (!originalFeaturePtr)
+    {
+      LOG(LERROR, ("A feature with id", fid, "cannot be loaded."));
+      alohalytics::LogEvent("Editor_MissingFeature_Error");
+      return false;
+    }
+
+    fti.m_feature = *originalFeaturePtr;
+    fti.m_feature.ApplyPatch(xml);
+  }
+
+  fti.m_feature.SetID(fid);
+  fti.m_street = xml.GetTagValue(kAddrStreetTag);
+
+  fti.m_modificationTimestamp = xml.GetModificationTime();
+  ASSERT_NOT_EQUAL(my::INVALID_TIME_STAMP, fti.m_modificationTimestamp, ());
+  fti.m_uploadAttemptTimestamp = xml.GetUploadTime();
+  fti.m_uploadStatus = xml.GetUploadStatus();
+  fti.m_uploadError = xml.GetUploadError();
+  fti.m_status = status;
+
+  return true;
+}
+
 // Macros is used to avoid code duplication.
 #define GET_FEATURE_TYPE_INFO_BODY                                        \
   do                                                                      \
@@ -1037,7 +1027,7 @@ NewFeatureCategories Editor::GetNewFeatureCategories() const
   return NewFeatureCategories(*(m_config.Get()));
 }
 
-FeatureID Editor::GenerateNewFeatureId(MwmSet::MwmId const & id)
+FeatureID Editor::GenerateNewFeatureId(MwmSet::MwmId const & id) const
 {
   DECLARE_AND_ASSERT_THREAD_CHECKER("GenerateNewFeatureId is single-threaded.");
   // TODO(vng): Double-check if new feature indexes should uninterruptedly continue after existing indexes in mwm file.
@@ -1176,7 +1166,7 @@ string Editor::GetOriginalFeatureStreet(FeatureType & ft) const
   return m_delegate->GetOriginalFeatureStreet(ft);
 }
 
-void Editor::ForEachFeatureAtPoint(TFeatureTypeFn && fn, m2::PointD const & point) const
+void Editor::ForEachFeatureAtPoint(FeatureTypeFn && fn, m2::PointD const & point) const
 {
   if (!m_delegate)
   {
@@ -1186,6 +1176,64 @@ void Editor::ForEachFeatureAtPoint(TFeatureTypeFn && fn, m2::PointD const & poin
   m_delegate->ForEachFeatureAtPoint(move(fn), point);
 }
 
+FeatureID Editor::GetFeatureIdByXmlFeature(XMLFeature const & xml, MwmSet::MwmId const & mwmId,
+                                           FeatureStatus status, bool needMigrate) const
+{
+  ForEachFeaturesNearByFn forEach = [this](FeatureTypeFn && fn, m2::PointD const & point)
+  {
+    return ForEachFeatureAtPoint(move(fn), point);
+  };
+
+  // TODO(mgsergio): Deleted features are not properly handled yet.
+  if (needMigrate)
+  {
+    return editor::MigrateFeatureIndex(forEach, xml, status,
+                                       [this, &mwmId] { return GenerateNewFeatureId(mwmId); });
+  }
+
+  return FeatureID(mwmId, xml.GetMWMFeatureIndex());
+}
+
+void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, bool needMigrate)
+{
+  LogHelper logHelper(mwmId);
+
+  for (auto const & section : kXmlSections)
+  {
+    for (auto const & nodeOrWay : mwm.child(section.m_sectionName.c_str()).select_nodes("node|way"))
+    {
+      try
+      {
+        XMLFeature const xml(nodeOrWay.node());
+
+        auto const fid = GetFeatureIdByXmlFeature(xml, mwmId, section.m_status, needMigrate);
+
+        // Remove obsolete changes during migration.
+        if (needMigrate && IsObsolete(xml, fid))
+          continue;
+
+        FeatureTypeInfo fti;
+        if (!FillFeatureInfo(section.m_status, xml, fid, fti))
+          continue;
+
+        logHelper.Increment(section.m_status);
+
+        m_features[fid.m_mwmId].emplace(fid.m_index, move(fti));
+      }
+      catch (editor::XMLFeatureError const & ex)
+      {
+        ostringstream s;
+        nodeOrWay.node().print(s, "  ");
+        LOG(LERROR, (ex.what(), "mwmId =", mwmId, "in section", section.m_sectionName, s.str()));
+      }
+      catch (editor::MigrationError const & ex)
+      {
+        LOG(LWARNING, (ex.what(), "mwmId =", mwmId, XMLFeature(nodeOrWay.node())));
+      }
+    }
+  }
+}
+
 string DebugPrint(Editor::FeatureStatus fs)
 {
   switch (fs)