diff --git a/editor/osm_editor.cpp b/editor/osm_editor.cpp index 4fb8efe886..40be98186f 100644 --- a/editor/osm_editor.cpp +++ b/editor/osm_editor.cpp @@ -198,13 +198,17 @@ void Editor::LoadEdits() } xml_document doc; - if (!m_storage->Load(doc)) - return; - bool needRewriteEdits = false; - // TODO(mgsergio): synchronize access to m_features. - m_features.clear(); + { + std::lock_guard lock(m_mutex); + + if (!m_storage->Load(doc)) + return; + + m_features.clear(); + } + for (auto const & mwm : doc.child(kXmlRootNode).children(kXmlMwmNode)) { string const mapName = mwm.attribute("name").as_string(""); @@ -225,20 +229,20 @@ void Editor::LoadEdits() 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); - } // for mwms - + } // Save edits with new indexes and mwm version to avoid another migration on next startup. if (needRewriteEdits) + { + std::lock_guard lock(m_mutex); Save(); + } } -bool Editor::Save() +bool Editor::Save() const { - // TODO(AlexZ): Improve synchronization in Editor code. - static mutex saveMutex; - lock_guard lock(saveMutex); - if (m_features.empty()) { m_storage->Reset(); @@ -260,11 +264,14 @@ bool Editor::Save() xml_node obsolete = mwmNode.append_child(kObsoleteSection); for (auto & index : mwm.second) { - FeatureTypeInfo & fti = index.second; + FeatureTypeInfo const & fti = index.second; + // Temporary solution because of FeatureType does not have constant getters. + // TODO(a): Use constant reference instead of copy. + auto feature = fti.m_feature; // TODO: Do we really need to serialize deleted features in full details? Looks like mwm ID // and meta fields are enough. XMLFeature xf = - editor::ToXML(fti.m_feature, true /*type serializing helps during migration*/); + editor::ToXML(feature, true /*type serializing helps during migration*/); xf.SetMWMFeatureIndex(index.first); if (!fti.m_street.empty()) xf.SetTagValue(kAddrStreetTag, fti.m_street); @@ -294,6 +301,8 @@ bool Editor::Save() void Editor::ClearAllLocalEdits() { + std::lock_guard lock(m_mutex); + m_features.clear(); Save(); Invalidate(); @@ -301,10 +310,9 @@ void Editor::ClearAllLocalEdits() void Editor::OnMapDeregistered(platform::LocalCountryFile const & localFile) { - // TODO: to add some synchronization mechanism for whole Editor class - lock_guard g(m_mapDeregisteredMutex); - using TFeaturePair = decltype(m_features)::value_type; + + 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) { @@ -320,30 +328,28 @@ void Editor::OnMapDeregistered(platform::LocalCountryFile const & localFile) FeatureStatus Editor::GetFeatureStatus(MwmSet::MwmId const & mwmId, uint32_t index) const { - // Most popular case optimization. - if (m_features.empty()) - return FeatureStatus::Untouched; + std::lock_guard lock(m_mutex); - auto const * featureInfo = GetFeatureTypeInfo(mwmId, index); - if (featureInfo == nullptr) - return FeatureStatus::Untouched; - - return featureInfo->m_status; + return GetFeatureStatusImpl(mwmId, index); } FeatureStatus Editor::GetFeatureStatus(FeatureID const & fid) const { - return GetFeatureStatus(fid.m_mwmId, fid.m_index); + std::lock_guard lock(m_mutex); + + return GetFeatureStatusImpl(fid.m_mwmId, fid.m_index); } bool Editor::IsFeatureUploaded(MwmSet::MwmId const & mwmId, uint32_t index) const { - auto const * info = GetFeatureTypeInfo(mwmId, index); - return info && info->m_uploadStatus == kUploaded; + std::lock_guard lock(m_mutex); + return IsFeatureUploadedImpl(mwmId, index); } void Editor::DeleteFeature(FeatureID const & fid) { + std::lock_guard lock(m_mutex); + auto const mwm = m_features.find(fid.m_mwmId); if (mwm != m_features.end()) { @@ -386,9 +392,10 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) FeatureID const & fid = emo.GetID(); FeatureTypeInfo fti; - auto const featureStatus = GetFeatureStatus(fid.m_mwmId, fid.m_index); - ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Obsolete, - ("Obsolete feature cannot be modified.")); + std::lock_guard lock(m_mutex); + + auto const featureStatus = GetFeatureStatusImpl(fid); + ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Obsolete, ("Obsolete feature cannot be modified.")); ASSERT_NOT_EQUAL(featureStatus, FeatureStatus::Deleted, ("Unexpected feature status.")); bool const wasCreatedByUser = IsCreatedFeature(fid); @@ -476,7 +483,9 @@ Editor::SaveResult Editor::SaveEditedFeature(EditableMapObject const & emo) bool Editor::RollBackChanges(FeatureID const & fid) { - if (IsFeatureUploaded(fid.m_mwmId, fid.m_index)) + std::lock_guard lock(m_mutex); + + if (IsFeatureUploadedImpl(fid.m_mwmId, fid.m_index)) return false; return RemoveFeature(fid); @@ -484,26 +493,36 @@ bool Editor::RollBackChanges(FeatureID const & fid) void Editor::ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, FeatureIndexFunctor const & f, m2::RectD const & rect, - int /*scale*/) + int /*scale*/) const { + std::unique_lock lock(m_mutex); + auto const mwmFound = m_features.find(id); if (mwmFound == m_features.end()) return; - // TODO(AlexZ): Check that features are visible at this scale. // Process only new (created) features. - for (auto & index : mwmFound->second) + for (auto const & index : mwmFound->second) { - FeatureTypeInfo & ftInfo = index.second; - if (ftInfo.m_status == FeatureStatus::Created && - rect.IsPointInside(ftInfo.m_feature.GetCenter())) - f(index.first); + FeatureTypeInfo const & ftInfo = index.second; + if (ftInfo.m_status == FeatureStatus::Created) + { + // 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())) + f(index.first); + lock.lock(); + } } } 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); if (featureInfo == nullptr) return false; @@ -519,6 +538,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); if (featureInfo == nullptr) return false; @@ -530,6 +551,8 @@ bool Editor::GetEditedFeatureStreet(FeatureID const & fid, string & outFeatureSt vector Editor::GetFeaturesByStatus(MwmSet::MwmId const & mwmId, FeatureStatus status) const { + std::lock_guard lock(m_mutex); + vector features; auto const matchedMwm = m_features.find(mwmId); if (matchedMwm == m_features.end()) @@ -545,17 +568,19 @@ vector Editor::GetFeaturesByStatus(MwmSet::MwmId const & mwmId, EditableProperties Editor::GetEditableProperties(FeatureType & feature) const { + std::lock_guard lock(m_mutex); + ASSERT(version::IsSingleMwm(feature.GetID().m_mwmId.GetInfo()->m_version.GetVersion()), ("Edit mode should be available only on new data")); - ASSERT(GetFeatureStatus(feature.GetID()) != FeatureStatus::Obsolete, + ASSERT(GetFeatureStatusImpl(feature.GetID()) != 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 (GetFeatureStatus(feature.GetID()) != FeatureStatus::Created) + if (GetFeatureStatusImpl(feature.GetID()) != FeatureStatus::Created) { auto const originalFeaturePtr = GetOriginalFeature(feature.GetID()); if (!originalFeaturePtr) @@ -588,29 +613,20 @@ EditableProperties Editor::GetEditablePropertiesForTypes(feature::TypesHolder co return {}; } -bool Editor::HaveMapEditsToUpload() const -{ - for (auto const & id : m_features) - { - for (auto const & index : id.second) - { - if (NeedsUpload(index.second.m_uploadStatus)) - return true; - } - } - return false; -} - bool Editor::HaveMapEditsOrNotesToUpload() const { if (m_notes->NotUploadedNotesCount() != 0) return true; + std::lock_guard lock(m_mutex); + return HaveMapEditsToUpload(); } bool Editor::HaveMapEditsToUpload(MwmSet::MwmId const & mwmId) const { + std::lock_guard lock(m_mutex); + auto const found = m_features.find(mwmId); if (found != m_features.end()) { @@ -624,30 +640,35 @@ bool Editor::HaveMapEditsToUpload(MwmSet::MwmId const & mwmId) const } void Editor::UploadChanges(string const & key, string const & secret, ChangesetTags tags, - FinishUploadCallback callBack) + FinishUploadCallback callback) { if (m_notes->NotUploadedNotesCount()) - UploadNotes(key, secret); - - if (!HaveMapEditsToUpload()) { - LOG(LDEBUG, ("There are no local edits to upload.")); - return; + alohalytics::LogEvent("Editor_UploadNotes", strings::to_string(m_notes->NotUploadedNotesCount())); + m_notes->Upload(OsmOAuth::ServerAuth({key, secret})); + } + + { + std::lock_guard lock(m_mutex); + + if (!HaveMapEditsToUpload()) + { + LOG(LDEBUG, ("There are no local edits to upload.")); + return; + } } alohalytics::LogEvent("Editor_DataSync_started"); - // TODO(AlexZ): features access should be synchronized. - auto const upload = [this](string key, string secret, ChangesetTags tags, - FinishUploadCallback callBack) { - // This lambda was designed to start after app goes into background. But for cases when user is - // immediately coming back to the app we work with a copy, because 'for' loops below can take a - // significant amount of time. - auto features = m_features; - + auto const upload = [this](string key, string secret, ChangesetTags tags, FinishUploadCallback callback) + { int uploadedFeaturesCount = 0, errorsCount = 0; ChangesetWrapper changeset({key, secret}, tags); - for (auto & id : features) + + { + std::unique_lock guard(m_mutex); + + for (auto & id : m_features) { for (auto & index : id.second) { @@ -676,36 +697,43 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT ("Linear and area features creation is not supported yet.")); try { - XMLFeature osmFeature = - changeset.GetMatchingNodeFeatureFromOSM(fti.m_feature.GetCenter()); + auto const center = fti.m_feature.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. + // To avoid nodes duplication, merge and apply changes to it instead of creating an new one. 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, ("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 (...) { @@ -718,8 +746,7 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT case FeatureStatus::Modified: { // Do not serialize feature's type to avoid breaking OSM data. - // TODO: Implement correct types matching when we support modifying existing feature - // types. + // TODO: Implement correct types matching when we support modifying existing feature types. XMLFeature feature = editor::ToXML(fti.m_feature, false); if (!fti.m_street.empty()) feature.SetTagValue(kAddrStreetTag, fti.m_street); @@ -730,11 +757,13 @@ 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"); - RemoveFeatureFromStorageIfExists(fti.m_feature.GetID()); + RemoveFeatureIfExists(fti.m_feature.GetID()); continue; } + guard.unlock(); XMLFeature osmFeature = GetMatchingFeatureFromOSM(changeset, *originalFeaturePtr); + guard.lock(); XMLFeature const osmFeatureCopy = osmFeature; osmFeature.ApplyPatch(feature); // Check to avoid uploading duplicates into OSM. @@ -748,7 +777,9 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT else { LOG(LDEBUG, ("Uploading patched feature", osmFeature)); + guard.unlock(); changeset.Modify(osmFeature); + guard.lock(); } } break; @@ -759,10 +790,12 @@ 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"); - RemoveFeatureFromStorageIfExists(fti.m_feature.GetID()); + RemoveFeatureIfExists(fti.m_feature.GetID()); continue; } + guard.unlock(); changeset.Delete(GetMatchingFeatureFromOSM(changeset, *originalFeaturePtr)); + guard.lock(); break; } fti.m_uploadStatus = kUploaded; @@ -808,33 +841,33 @@ void Editor::UploadChanges(string const & key, string const & secret, ChangesetT // Call Save every time we modify each feature's information. SaveUploadedInformation(fti); } - } + } // loop scope + } // unique_lock scope alohalytics::LogEvent("Editor_DataSync_finished", {{"errors", strings::to_string(errorsCount)}, {"uploaded", strings::to_string(uploadedFeaturesCount)}, {"changeset", strings::to_string(changeset.GetChangesetId())}}); - if (callBack) + if (callback) { UploadResult result = UploadResult::NothingToUpload; if (uploadedFeaturesCount) result = UploadResult::Success; else if (errorsCount) result = UploadResult::Error; - callBack(result); + callback(result); } }; // Do not run more than one upload thread at a time. - static auto future = async(launch::async, upload, key, secret, tags, callBack); + static auto future = async(launch::async, upload, key, secret, tags, callback); auto const status = future.wait_for(milliseconds(0)); if (status == future_status::ready) - future = async(launch::async, upload, key, secret, tags, callBack); + future = async(launch::async, upload, key, secret, tags, callback); } void Editor::SaveUploadedInformation(FeatureTypeInfo const & fromUploader) { - // TODO(AlexZ): Correctly synchronize this call and Save() at the end. FeatureID const & fid = fromUploader.m_feature.GetID(); auto id = m_features.find(fid.m_mwmId); if (id == m_features.end()) @@ -913,13 +946,13 @@ Editor::FeatureTypeInfo * Editor::GetFeatureTypeInfo(MwmSet::MwmId const & mwmId #undef GET_FEATURE_TYPE_INFO_BODY -void Editor::RemoveFeatureFromStorageIfExists(MwmSet::MwmId const & mwmId, uint32_t index) +void Editor::RemoveFeatureIfExists(FeatureID const & fid) { - auto matchedMwm = m_features.find(mwmId); + auto matchedMwm = m_features.find(fid.m_mwmId); if (matchedMwm == m_features.end()) return; - auto matchedIndex = matchedMwm->second.find(index); + auto matchedIndex = matchedMwm->second.find(fid.m_index); if (matchedIndex != matchedMwm->second.end()) matchedMwm->second.erase(matchedIndex); @@ -927,11 +960,6 @@ void Editor::RemoveFeatureFromStorageIfExists(MwmSet::MwmId const & mwmId, uint3 m_features.erase(matchedMwm); } -void Editor::RemoveFeatureFromStorageIfExists(FeatureID const & fid) -{ - return RemoveFeatureFromStorageIfExists(fid.m_mwmId, fid.m_index); -} - void Editor::Invalidate() { if (m_invalidateFn) @@ -940,7 +968,7 @@ void Editor::Invalidate() bool Editor::MarkFeatureAsObsolete(FeatureID const & fid) { - auto const featureStatus = GetFeatureStatus(fid); + auto const featureStatus = GetFeatureStatusImpl(fid); if (featureStatus != FeatureStatus::Untouched && featureStatus != FeatureStatus::Modified) { ASSERT(false, ("Only untouched and modified features can be made obsolete")); @@ -955,27 +983,32 @@ bool Editor::MarkFeatureAsObsolete(FeatureID const & fid) bool Editor::RemoveFeature(FeatureID const & fid) { - RemoveFeatureFromStorageIfExists(fid.m_mwmId, fid.m_index); + RemoveFeatureIfExists(fid); Invalidate(); return Save(); } -Editor::Stats Editor::GetStats() +Editor::Stats Editor::GetStats() const { + std::lock_guard lock(m_mutex); + Stats stats; LOG(LDEBUG, ("Edited features status:")); for (auto & id : m_features) { for (auto & index : id.second) { - Editor::FeatureTypeInfo & fti = index.second; + auto const & fti = index.second; + // Temporary solution because of FeatureType does not have constant getters. + // TODO(a): Use constant reference instead of copy. + auto feature = fti.m_feature; stats.m_edits.push_back(make_pair(FeatureID(id.first, index.first), fti.m_uploadStatus + " " + fti.m_uploadError)); LOG(LDEBUG, (fti.m_uploadAttemptTimestamp == my::INVALID_TIME_STAMP ? "NOT_UPLOADED_YET" : my::TimestampToString(fti.m_uploadAttemptTimestamp), - fti.m_uploadStatus, fti.m_uploadError, fti.m_feature.GetFeatureType(), - feature::GetCenter(fti.m_feature))); + fti.m_uploadStatus, fti.m_uploadError, feature.GetFeatureType(), + feature::GetCenter(feature))); if (fti.m_uploadStatus == kUploaded) { ++stats.m_uploadedCount; @@ -1013,7 +1046,7 @@ FeatureID Editor::GenerateNewFeatureId(MwmSet::MwmId const & id) const } bool Editor::CreatePoint(uint32_t type, m2::PointD const & mercator, MwmSet::MwmId const & id, - EditableMapObject & outFeature) + EditableMapObject & outFeature) const { ASSERT(id.IsAlive(), ("Please check that feature is created in valid MWM file before calling this method.")); @@ -1022,6 +1055,9 @@ bool Editor::CreatePoint(uint32_t type, m2::PointD const & mercator, MwmSet::Mwm LOG(LERROR, ("Attempt to create a feature outside of the MWM's bounding box.")); return false; } + + std::lock_guard lock(m_mutex); + outFeature.SetMercator(mercator); outFeature.SetID(GenerateNewFeatureId(id)); outFeature.SetType(type); @@ -1047,9 +1083,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 = GetFeatureStatus(fid) == FeatureStatus::Created; - auto const createdAndUploaded = (isCreated && IsFeatureUploaded(fid.m_mwmId, fid.m_index)); + auto const isCreated = GetFeatureStatusImpl(fid) == FeatureStatus::Created; + auto const createdAndUploaded = + (isCreated && IsFeatureUploadedImpl(fid.m_mwmId, fid.m_index)); CHECK(!isCreated || createdAndUploaded, ()); if (createdAndUploaded) @@ -1080,12 +1119,6 @@ void Editor::CreateNote(ms::LatLon const & latLon, FeatureID const & fid, m_notes->CreateNote(latLon, sstr.str()); } -void Editor::UploadNotes(string const & key, string const & secret) -{ - alohalytics::LogEvent("Editor_UploadNotes", strings::to_string(m_notes->NotUploadedNotesCount())); - m_notes->Upload(OsmOAuth::ServerAuth({key, secret})); -} - void Editor::MarkFeatureWithStatus(FeatureID const & fid, FeatureStatus status) { auto & fti = m_features[fid.m_mwmId][fid.m_index]; @@ -1155,7 +1188,11 @@ FeatureID Editor::GetFeatureIdByXmlFeature(XMLFeature const & xml, MwmSet::MwmId if (needMigrate) { return editor::MigrateFeatureIndex(forEach, xml, status, - [this, &mwmId] { return GenerateNewFeatureId(mwmId); }); + [this, &mwmId] + { + std::lock_guard lock(m_mutex); + return GenerateNewFeatureId(mwmId); + }); } return FeatureID(mwmId, xml.GetMWMFeatureIndex()); @@ -1185,7 +1222,10 @@ void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, boo logHelper.OnStatus(section.m_status); - m_features[fid.m_mwmId].emplace(fid.m_index, move(fti)); + { + std::lock_guard lock(m_mutex); + m_features[fid.m_mwmId].emplace(fid.m_index, move(fti)); + } } catch (editor::XMLFeatureError const & ex) { @@ -1201,6 +1241,43 @@ void Editor::LoadMwmEdits(xml_node const & mwm, MwmSet::MwmId const & mwmId, boo } } +FeatureStatus Editor::GetFeatureStatusImpl(MwmSet::MwmId const & mwmId, uint32_t index) 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 & index : id.second) + { + if (NeedsUpload(index.second.m_uploadStatus)) + return true; + } + } + return false; +} + 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 8567abfe5b..79dec05d13 100644 --- a/editor/osm_editor.hpp +++ b/editor/osm_editor.hpp @@ -65,8 +65,35 @@ public: Error, NothingToUpload }; + using FinishUploadCallback = function; + enum SaveResult + { + NothingWasChanged, + SavedSuccessfully, + NoFreeSpaceError, + NoUnderlyingMapError, + SavingError + }; + + enum class NoteProblemType + { + General, + PlaceDoesNotExist + }; + + struct Stats + { + /// + vector> m_edits; + size_t m_uploadedCount = 0; + time_t m_lastUploadTimestamp = my::INVALID_TIME_STAMP; + }; + + // Predefined messages. + static const char * const kPlaceDoesNotExistMessage; + static Editor & Instance(); inline void SetDelegate(unique_ptr delegate) { m_delegate = move(delegate); } @@ -94,7 +121,7 @@ public: using FeatureIndexFunctor = function; void ForEachFeatureInMwmRectAndScale(MwmSet::MwmId const & id, FeatureIndexFunctor const & f, - m2::RectD const & rect, int scale); + m2::RectD const & rect, int scale) const; // TODO(mgsergio): Unify feature functions signatures. @@ -120,14 +147,6 @@ public: /// @returns sorted features indices with specified status. vector GetFeaturesByStatus(MwmSet::MwmId const & mwmId, FeatureStatus status) const; - enum SaveResult - { - NothingWasChanged, - SavedSuccessfully, - NoFreeSpaceError, - NoUnderlyingMapError, - SavingError - }; /// Editor checks internally if any feature params were actually edited. SaveResult SaveEditedFeature(EditableMapObject const & emo); @@ -139,7 +158,7 @@ public: bool HaveMapEditsOrNotesToUpload() const; bool HaveMapEditsToUpload(MwmSet::MwmId const & mwmId) const; - bool HaveMapEditsToUpload() const; + using ChangesetTags = map; /// Tries to upload all local changes to OSM server in a separate thread. /// @param[in] tags should provide additional information about client to use in changeset. @@ -149,31 +168,14 @@ public: // Editor should silently ignore all types in config which are unknown to him. NewFeatureCategories GetNewFeatureCategories() const; - bool CreatePoint(uint32_t type, m2::PointD const & mercator, - MwmSet::MwmId const & id, EditableMapObject & outFeature); - - // Predefined messages. - static const char * const kPlaceDoesNotExistMessage; - - enum class NoteProblemType - { - General, - PlaceDoesNotExist - }; + bool CreatePoint(uint32_t type, m2::PointD const & mercator, MwmSet::MwmId const & id, + EditableMapObject & outFeature) const; void CreateNote(ms::LatLon const & latLon, FeatureID const & fid, feature::TypesHolder const & holder, string const & defaultName, NoteProblemType const type, string const & note); - void UploadNotes(string const & key, string const & secret); - struct Stats - { - /// - vector> m_edits; - size_t m_uploadedCount = 0; - time_t m_lastUploadTimestamp = my::INVALID_TIME_STAMP; - }; - Stats GetStats(); + Stats GetStats() const; // Don't use this function to determine if a feature in editor was created. // Use GetFeatureStatus(fid) instead. This function is used when a feature is @@ -181,21 +183,6 @@ public: static bool IsCreatedFeature(FeatureID const & fid); private: - // TODO(AlexZ): Synchronize Save call/make it on a separate thread. - /// @returns false if fails. - bool Save(); - void RemoveFeatureFromStorageIfExists(MwmSet::MwmId const & mwmId, uint32_t index); - void RemoveFeatureFromStorageIfExists(FeatureID const & fid); - /// Notify framework that something has changed and should be redisplayed. - void Invalidate(); - - // Saves a feature in internal storage with FeatureStatus::Obsolete status. - bool MarkFeatureAsObsolete(FeatureID const & fid); - bool RemoveFeature(FeatureID const & fid); - - FeatureID GenerateNewFeatureId(MwmSet::MwmId const & id) const; - EditableProperties GetEditablePropertiesForTypes(feature::TypesHolder const & types) const; - struct FeatureTypeInfo { FeatureStatus m_status; @@ -210,6 +197,19 @@ private: string m_uploadError; }; + /// @returns false if fails. + bool Save() const; + void RemoveFeatureIfExists(FeatureID const & fid); + /// Notify framework that something has changed and should be redisplayed. + void Invalidate(); + + // Saves a feature in internal storage with FeatureStatus::Obsolete status. + bool MarkFeatureAsObsolete(FeatureID const & fid); + bool RemoveFeature(FeatureID const & fid); + + FeatureID GenerateNewFeatureId(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. @@ -228,7 +228,15 @@ private: FeatureStatus status, bool needMigrate) const; void LoadMwmEdits(pugi::xml_node const & mwm, MwmSet::MwmId const & mwmId, bool needMigrate); - // TODO(AlexZ): Synchronize multithread access. + FeatureStatus GetFeatureStatusImpl(MwmSet::MwmId const & mwmId, uint32_t index) const; + FeatureStatus GetFeatureStatusImpl(FeatureID const & fid) const; + + bool IsFeatureUploadedImpl(MwmSet::MwmId const & mwmId, uint32_t index) const; + + bool HaveMapEditsToUpload() const; + + mutable std::mutex m_mutex; + /// Deleted, edited and created features. map> m_features; @@ -243,8 +251,6 @@ private: /// Notes to be sent to osm. shared_ptr m_notes; - // Mutex which locks OnMapDeregistered method - mutex m_mapDeregisteredMutex; unique_ptr m_storage; }; // class Editor diff --git a/search/editor_delegate.cpp b/search/editor_delegate.cpp index cc4f323247..b6fd7f433b 100644 --- a/search/editor_delegate.cpp +++ b/search/editor_delegate.cpp @@ -29,7 +29,7 @@ unique_ptr EditorDelegate::GetOriginalFeature(FeatureID const & fid string EditorDelegate::GetOriginalFeatureStreet(FeatureType & ft) const { search::ReverseGeocoder const coder(m_dataSource); - auto const streets = coder.GetNearbyFeatureStreets(ft); + auto const streets = coder.GetOriginalNearbyFeatureStreets(ft); if (streets.second < streets.first.size()) return streets.first[streets.second].m_name; return {}; diff --git a/search/mwm_context.cpp b/search/mwm_context.cpp index 81f49d95e1..76544b7560 100644 --- a/search/mwm_context.cpp +++ b/search/mwm_context.cpp @@ -1,6 +1,7 @@ #include "search/mwm_context.hpp" #include "indexer/cell_id.hpp" +#include "indexer/fake_feature_ids.hpp" #include "indexer/feature_source.hpp" namespace search @@ -40,6 +41,16 @@ bool MwmContext::GetFeature(uint32_t index, FeatureType & ft) const CHECK_SWITCH(); } +bool MwmContext::GetOriginalFeature(uint32_t index, FeatureType & ft) const +{ + if (feature::FakeFeatureIds::IsEditorCreatedFeature(index)) + return false; + + m_vector.GetByIndex(index, ft); + ft.SetID(FeatureID(GetId(), index)); + return true; +} + bool MwmContext::GetStreetIndex(uint32_t houseId, uint32_t & streetId) { if (!m_houseToStreetTable) diff --git a/search/mwm_context.hpp b/search/mwm_context.hpp index 9f42e50f7c..69daedee52 100644 --- a/search/mwm_context.hpp +++ b/search/mwm_context.hpp @@ -59,20 +59,29 @@ public: template void ForEachFeature(m2::RectD const & rect, TFn && fn) const { - uint32_t const scale = m_value.GetHeader().GetLastScale(); - covering::Intervals intervals; - CoverRect(rect, scale, intervals); - - ForEachIndexImpl(intervals, scale, [&](uint32_t index) - { - FeatureType ft; - if (GetFeature(index, ft)) - fn(ft); - }); + ForEachFeatureImpl(rect, [&](uint32_t index) + { + FeatureType ft; + if (GetFeature(index, ft)) + fn(ft); + }); } - // @returns false if feature was deleted by user. + template + void ForEachOriginalFeature(m2::RectD const & rect, TFn && fn) const + { + ForEachFeatureImpl(rect, [&](uint32_t index) + { + FeatureType ft; + if (GetOriginalFeature(index, ft)) + fn(ft); + }); + } + + // Returns false if feature was deleted by user. WARN_UNUSED_RESULT bool GetFeature(uint32_t index, FeatureType & ft) const; + // Returns false if feature was created by user. + WARN_UNUSED_RESULT bool GetOriginalFeature(uint32_t index, FeatureType & ft) const; WARN_UNUSED_RESULT bool GetStreetIndex(uint32_t houseId, uint32_t & streetId); @@ -90,6 +99,16 @@ private: return osm::Editor::Instance().GetFeatureStatus(GetId(), index); } + template + void ForEachFeatureImpl(m2::RectD const & rect, TFn && fn) const + { + uint32_t const scale = m_value.GetHeader().GetLastScale(); + covering::Intervals intervals; + CoverRect(rect, scale, intervals); + + ForEachIndexImpl(intervals, scale, fn); + } + template void ForEachIndexImpl(covering::Intervals const & intervals, uint32_t scale, TFn && fn) const { diff --git a/search/reverse_geocoder.cpp b/search/reverse_geocoder.cpp index 602e38f5d1..c5fa762555 100644 --- a/search/reverse_geocoder.cpp +++ b/search/reverse_geocoder.cpp @@ -12,6 +12,7 @@ #include "base/stl_helpers.hpp" +#include "std/function.hpp" #include "std/limits.hpp" namespace search @@ -22,7 +23,50 @@ size_t constexpr kSimilarityThresholdPercent = 10; int constexpr kQueryScale = scales::GetUpperScale(); /// Max number of tries (nearest houses with housenumber) to check when getting point address. size_t constexpr kMaxNumTriesToApproxAddress = 10; -} // namespace + +using AppendStreet = function; +using FillStreets = + function; + +m2::RectD GetLookupRect(m2::PointD const & center, double radiusM) +{ + return MercatorBounds::RectByCenterXYAndSizeInMeters(center, radiusM); +} + +void AddStreet(FeatureType & ft, m2::PointD const & center, + vector & streets) +{ + if (ft.GetFeatureType() != feature::GEOM_LINE || !ftypes::IsStreetChecker::Instance()(ft)) + { + return; + } + + string name; + if (!ft.GetName(StringUtf8Multilang::kDefaultCode, name)) + return; + + ASSERT(!name.empty(), ()); + + streets.emplace_back(ft.GetID(), feature::GetMinDistanceMeters(ft, center), name); +} + +void GetNearbyStreetsImpl(DataSource const & source, MwmSet::MwmId const & id, + m2::PointD const & center, vector & streets, + FillStreets && fillStreets) +{ + m2::RectD const rect = GetLookupRect(center, ReverseGeocoder::kLookupRadiusM); + MwmSet::MwmHandle mwmHandle = source.GetMwmHandleById(id); + + if (!mwmHandle.IsAlive()) + return; + + auto const addStreet = [¢er, &streets](FeatureType & ft) { AddStreet(ft, center, streets); }; + + fillStreets(move(mwmHandle), rect, addStreet); + + sort(streets.begin(), streets.end(), my::LessBy(&ReverseGeocoder::Street::m_distanceMeters)); +} +} // namespace ReverseGeocoder::ReverseGeocoder(DataSource const & dataSource) : m_dataSource(dataSource) {} @@ -32,22 +76,7 @@ void ReverseGeocoder::GetNearbyStreets(search::MwmContext & context, m2::PointD { m2::RectD const rect = GetLookupRect(center, kLookupRadiusM); - auto const addStreet = [&](FeatureType & ft) - { - if (ft.GetFeatureType() != feature::GEOM_LINE || - !ftypes::IsStreetChecker::Instance()(ft)) - { - return; - } - - string name; - if (!ft.GetName(StringUtf8Multilang::kDefaultCode, name)) - return; - - ASSERT(!name.empty(), ()); - - streets.emplace_back(ft.GetID(), feature::GetMinDistanceMeters(ft, center), name); - }; + auto const addStreet = [¢er, &streets](FeatureType & ft) { AddStreet(ft, center, streets); }; context.ForEachFeature(rect, addStreet); sort(streets.begin(), streets.end(), my::LessBy(&Street::m_distanceMeters)); @@ -56,12 +85,13 @@ void ReverseGeocoder::GetNearbyStreets(search::MwmContext & context, m2::PointD void ReverseGeocoder::GetNearbyStreets(MwmSet::MwmId const & id, m2::PointD const & center, vector & streets) const { - MwmSet::MwmHandle mwmHandle = m_dataSource.GetMwmHandleById(id); - if (mwmHandle.IsAlive()) + auto const fillStreets = [](MwmSet::MwmHandle && handle, m2::RectD const & rect, + AppendStreet && addStreet) { - search::MwmContext context(move(mwmHandle)); - GetNearbyStreets(context, center, streets); - } + search::MwmContext(move(handle)).ForEachFeature(rect, addStreet); + }; + + GetNearbyStreetsImpl(m_dataSource, id, center, streets, move(fillStreets)); } void ReverseGeocoder::GetNearbyStreets(FeatureType & ft, vector & streets) const @@ -70,6 +100,18 @@ void ReverseGeocoder::GetNearbyStreets(FeatureType & ft, vector & street GetNearbyStreets(ft.GetID().m_mwmId, feature::GetCenter(ft), streets); } +void ReverseGeocoder::GetOriginalNearbyStreets(MwmSet::MwmId const & id, m2::PointD const & center, + vector & streets) const +{ + auto const fillStreets = [](MwmSet::MwmHandle && handle, m2::RectD const & rect, + AppendStreet && addStreet) + { + search::MwmContext(move(handle)).ForEachOriginalFeature(rect, addStreet); + }; + + GetNearbyStreetsImpl(m_dataSource, id, center, streets, move(fillStreets)); +} + // static size_t ReverseGeocoder::GetMatchedStreetIndex(strings::UniString const & keyName, vector const & streets) @@ -117,6 +159,21 @@ ReverseGeocoder::GetNearbyFeatureStreets(FeatureType & ft) const return result; } +pair, uint32_t> +ReverseGeocoder::GetOriginalNearbyFeatureStreets(FeatureType & ft) const +{ + pair, uint32_t> result; + + ASSERT(ft.GetID().IsValid(), ()); + GetOriginalNearbyStreets(ft.GetID().m_mwmId, feature::GetCenter(ft), result.first); + + HouseTable table(m_dataSource); + if (!table.Get(ft.GetID(), result.second)) + result.second = numeric_limits::max(); + + return result; +} + void ReverseGeocoder::GetNearbyAddress(m2::PointD const & center, Address & addr) const { vector buildings; @@ -192,12 +249,6 @@ ReverseGeocoder::Building ReverseGeocoder::FromFeature(FeatureType & ft, double return { ft.GetID(), distMeters, ft.GetHouseNumber(), feature::GetCenter(ft) }; } -// static -m2::RectD ReverseGeocoder::GetLookupRect(m2::PointD const & center, double radiusM) -{ - return MercatorBounds::RectByCenterXYAndSizeInMeters(center, radiusM); -} - bool ReverseGeocoder::HouseTable::Get(FeatureID const & fid, uint32_t & streetIndex) { if (!m_table || m_handle.GetId() != fid.m_mwmId) diff --git a/search/reverse_geocoder.hpp b/search/reverse_geocoder.hpp index af4d222199..5ae328b324 100644 --- a/search/reverse_geocoder.hpp +++ b/search/reverse_geocoder.hpp @@ -77,18 +77,20 @@ public: friend string DebugPrint(Address const & addr); /// @return Sorted by distance streets vector for the specified MwmId. - //@{ static void GetNearbyStreets(search::MwmContext & context, m2::PointD const & center, vector & streets); void GetNearbyStreets(MwmSet::MwmId const & id, m2::PointD const & center, vector & streets) const; void GetNearbyStreets(FeatureType & ft, vector & streets) const; - //@} + void GetOriginalNearbyStreets(MwmSet::MwmId const & id, m2::PointD const & center, + vector & streets) const; /// @returns [a lot of] nearby feature's streets and an index of a feature's street. /// Returns a value greater than vector size when there are no Street the feature belongs to. /// @note returned vector can contain duplicated street segments. pair, uint32_t> GetNearbyFeatureStreets(FeatureType & ft) const; + /// Same as GetNearbyFeatureStreets but returns streets from MWM only. + pair, uint32_t> GetOriginalNearbyFeatureStreets(FeatureType & ft) const; /// @return The nearest exact address where building has house number and valid street match. void GetNearbyAddress(m2::PointD const & center, Address & addr) const; @@ -115,7 +117,6 @@ private: void GetNearbyBuildings(m2::PointD const & center, vector & buildings) const; static Building FromFeature(FeatureType & ft, double distMeters); - static m2::RectD GetLookupRect(m2::PointD const & center, double radiusM); }; } // namespace search