diff --git a/indexer/edits_migration.cpp b/indexer/edits_migration.cpp index bf56ec971f..298bee88f4 100644 --- a/indexer/edits_migration.cpp +++ b/indexer/edits_migration.cpp @@ -12,7 +12,7 @@ namespace editor { -FeatureID MigrateNodeFeatureIndex(osm::Editor::TForEachFeaturesNearByFn & forEach, +FeatureID MigrateNodeFeatureIndex(osm::Editor::ForEachFeaturesNearByFn & forEach, XMLFeature const & xml, osm::Editor::FeatureStatus const featureStatus, TGenerateIDFn const & generateID) @@ -44,7 +44,7 @@ FeatureID MigrateNodeFeatureIndex(osm::Editor::TForEachFeaturesNearByFn & forEac } FeatureID MigrateWayFeatureIndex( - osm::Editor::TForEachFeaturesNearByFn & forEach, XMLFeature const & xml, + osm::Editor::ForEachFeaturesNearByFn & forEach, XMLFeature const & xml, osm::Editor::FeatureStatus const /* Unused for now (we don't create/delete area features)*/, TGenerateIDFn const & /*Unused for the same reason*/) { @@ -108,7 +108,7 @@ FeatureID MigrateWayFeatureIndex( return feature->GetID(); } -FeatureID MigrateFeatureIndex(osm::Editor::TForEachFeaturesNearByFn & forEach, +FeatureID MigrateFeatureIndex(osm::Editor::ForEachFeaturesNearByFn & forEach, XMLFeature const & xml, osm::Editor::FeatureStatus const featureStatus, TGenerateIDFn const & generateID) diff --git a/indexer/edits_migration.hpp b/indexer/edits_migration.hpp index c36c74e697..26e1232c1e 100644 --- a/indexer/edits_migration.hpp +++ b/indexer/edits_migration.hpp @@ -17,7 +17,7 @@ using TGenerateIDFn = function; /// Tries to match xml feature with one on a new mwm and retruns FeatrueID /// of a found feature, thows MigrationError if migration fails. -FeatureID MigrateFeatureIndex(osm::Editor::TForEachFeaturesNearByFn & forEach, +FeatureID MigrateFeatureIndex(osm::Editor::ForEachFeaturesNearByFn & forEach, XMLFeature const & xml, osm::Editor::FeatureStatus const featureStatus, TGenerateIDFn const & generateID); diff --git a/indexer/indexer_tests/osm_editor_test.cpp b/indexer/indexer_tests/osm_editor_test.cpp index a7c9f98218..dc0a7b8f19 100644 --- a/indexer/indexer_tests/osm_editor_test.cpp +++ b/indexer/indexer_tests/osm_editor_test.cpp @@ -438,7 +438,7 @@ void EditorTest::IsFeatureUploadedTest() pugi::xml_document doc; GenerateUploadedFeature(mwmId, emo, doc); editor.m_storage->Save(doc); - editor.LoadMapEdits(); + editor.LoadEdits(); TEST(editor.IsFeatureUploaded(emo.GetID().m_mwmId, emo.GetID().m_index), ()); } @@ -756,7 +756,7 @@ void EditorTest::GetStatsTest() pugi::xml_document doc; GenerateUploadedFeature(mwmId, emo, doc); editor.m_storage->Save(doc); - editor.LoadMapEdits(); + editor.LoadEdits(); stats = editor.GetStats(); TEST_EQUAL(stats.m_edits.size(), 1, ()); @@ -931,7 +931,7 @@ void EditorTest::LoadMapEditsTest() features.emplace_back(emo.GetID()); editor.Save(); - editor.LoadMapEdits(); + editor.LoadEdits(); auto const fillLoaded = [&editor](vector & loadedFeatures) { @@ -962,7 +962,7 @@ void EditorTest::LoadMapEditsTest() builder.Add(TestCafe(m2::PointD(6.0, 6.0), "Moscow Cafe4", "en")); }, 1); - editor.LoadMapEdits(); + editor.LoadEdits(); fillLoaded(loadedFeatures); TEST_EQUAL(features.size(), loadedFeatures.size(), ()); @@ -972,7 +972,7 @@ void EditorTest::LoadMapEditsTest() TEST_EQUAL(editor.m_features.size(), 2, ()); - editor.LoadMapEdits(); + editor.LoadEdits(); fillLoaded(loadedFeatures); TEST_EQUAL(editor.m_features.size(), 1, ()); @@ -984,7 +984,7 @@ void EditorTest::LoadMapEditsTest() pugi::xml_document doc; GenerateUploadedFeature(gbMwmId, gbEmo, doc); editor.m_storage->Save(doc); - editor.LoadMapEdits(); + editor.LoadEdits(); fillLoaded(loadedFeatures); TEST_EQUAL(editor.m_features.size(), 1, ()); @@ -1001,7 +1001,7 @@ void EditorTest::LoadMapEditsTest() newGbMwmId.GetInfo()->m_version.SetSecondsSinceEpoch(time(nullptr) + 1); - editor.LoadMapEdits(); + editor.LoadEdits(); TEST(editor.m_features.empty(), ()); } 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 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(); } -void Editor::LoadMapEdits() +void Editor::LoadEdits() { if (!m_delegate) { @@ -164,121 +206,35 @@ void Editor::LoadMapEdits() if (!m_storage->Load(doc)) return; - array, 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); + auto const mwmId = GetMwmIdByMapName(mapName); + + // TODO(mgsergio): A map could be renamed, we'll treat it as deleted. + if (!mwmId.IsAlive()) + { + LOG(LINFO, ("Mwm", mapName, "was deleted")); + needRewriteEdits = true; + 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 = !mwmId.IsAlive() || mapVersion != mwmId.GetInfo()->GetVersion(); - needRewriteEdits |= needMigrateEdits; + auto const needMigrateEdits = mapVersion != mwmId.GetInfo()->GetVersion(); + needRewriteEdits = needRewriteEdits || needMigrateEdits; - for (auto const & section : sections) - { - 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); - } - - 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 = 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) diff --git a/indexer/osm_editor.hpp b/indexer/osm_editor.hpp index f83c81e1f0..98903d431d 100644 --- a/indexer/osm_editor.hpp +++ b/indexer/osm_editor.hpp @@ -28,8 +28,12 @@ namespace editor namespace testing { class EditorTest; -} // namespace testing -} // namespace editor +} +} +namespace editor +{ +class XMLFeature; +} class Index; @@ -42,14 +46,9 @@ class Editor final : public MwmSet::Observer Editor(); public: - using TFeatureTypeFn = function; // Mimics Framework::TFeatureTypeFn. - - using TMwmIdByMapNameFn = function; - using TInvalidateFn = function; - using TFeatureLoaderFn = function (FeatureID const & /*fid*/)>; - using TFeatureOriginalStreetFn = function; - using TForEachFeaturesNearByFn = - function; + using FeatureTypeFn = function; + using InvalidateFn = function; + using ForEachFeaturesNearByFn = function; struct Delegate { @@ -58,7 +57,7 @@ public: virtual MwmSet::MwmId GetMwmIdByMapName(string const & name) const = 0; virtual unique_ptr GetOriginalFeature(FeatureID const & fid) const = 0; virtual string GetOriginalFeatureStreet(FeatureType & ft) const = 0; - virtual void ForEachFeatureAtPoint(TFeatureTypeFn && fn, m2::PointD const & point) const = 0; + virtual void ForEachFeatureAtPoint(FeatureTypeFn && fn, m2::PointD const & point) const = 0; }; enum class UploadResult @@ -91,16 +90,16 @@ public: void SetDefaultStorage(); - void SetInvalidateFn(TInvalidateFn const & fn) { m_invalidateFn = fn; } + void SetInvalidateFn(InvalidateFn const & fn) { m_invalidateFn = fn; } - void LoadMapEdits(); + void LoadEdits(); /// Resets editor to initial state: no any edits or created/deleted features. void ClearAllLocalEdits(); void OnMapUpdated(platform::LocalCountryFile const &, platform::LocalCountryFile const &) override { - LoadMapEdits(); + LoadEdits(); } void OnMapDeregistered(platform::LocalCountryFile const & localFile) override; @@ -215,7 +214,7 @@ private: bool MarkFeatureAsObsolete(FeatureID const & fid); bool RemoveFeature(FeatureID const & fid); - FeatureID GenerateNewFeatureId(MwmSet::MwmId const & id); + FeatureID GenerateNewFeatureId(MwmSet::MwmId const & id) const; EditableProperties GetEditablePropertiesForTypes(feature::TypesHolder const & types) const; struct FeatureTypeInfo @@ -231,6 +230,8 @@ private: string m_uploadStatus; string m_uploadError; }; + 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); @@ -242,7 +243,10 @@ private: MwmSet::MwmId GetMwmIdByMapName(string const & name); unique_ptr GetOriginalFeature(FeatureID const & fid) const; string GetOriginalFeatureStreet(FeatureType & ft) const; - void ForEachFeatureAtPoint(TFeatureTypeFn && fn, m2::PointD const & point) const; + void ForEachFeatureAtPoint(FeatureTypeFn && fn, m2::PointD const & point) const; + FeatureID GetFeatureIdByXmlFeature(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); // TODO(AlexZ): Synchronize multithread access. /// Deleted, edited and created features. @@ -251,7 +255,7 @@ private: unique_ptr m_delegate; /// Invalidate map viewport after edits. - TInvalidateFn m_invalidateFn; + InvalidateFn m_invalidateFn; /// Contains information about what and how can be edited. editor::EditorConfigWrapper m_config; diff --git a/map/framework.cpp b/map/framework.cpp index 816c1fb46f..4e747f8ff1 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -475,7 +475,7 @@ Framework::Framework(FrameworkParams const & params) editor.SetDelegate(make_unique(m_model.GetIndex())); editor.SetInvalidateFn([this](){ InvalidateRect(GetCurrentViewport()); }); - editor.LoadMapEdits(); + editor.LoadEdits(); m_model.GetIndex().AddObserver(editor); diff --git a/search/editor_delegate.cpp b/search/editor_delegate.cpp index 95e41c963d..e2584b4524 100644 --- a/search/editor_delegate.cpp +++ b/search/editor_delegate.cpp @@ -34,7 +34,7 @@ string EditorDelegate::GetOriginalFeatureStreet(FeatureType & ft) const return {}; } -void EditorDelegate::ForEachFeatureAtPoint(osm::Editor::TFeatureTypeFn && fn, +void EditorDelegate::ForEachFeatureAtPoint(osm::Editor::FeatureTypeFn && fn, m2::PointD const & point) const { auto const kToleranceMeters = 1e-2; diff --git a/search/editor_delegate.hpp b/search/editor_delegate.hpp index c523cf2372..5e86785d9c 100644 --- a/search/editor_delegate.hpp +++ b/search/editor_delegate.hpp @@ -15,7 +15,7 @@ public: MwmSet::MwmId GetMwmIdByMapName(string const & name) const override; unique_ptr GetOriginalFeature(FeatureID const & fid) const override; string GetOriginalFeatureStreet(FeatureType & ft) const override; - void ForEachFeatureAtPoint(osm::Editor::TFeatureTypeFn && fn, + void ForEachFeatureAtPoint(osm::Editor::FeatureTypeFn && fn, m2::PointD const & point) const override; private: