From 14274b554310cfaec9daddebb73553a8c7bf6349 Mon Sep 17 00:00:00 2001 From: Ilya Zverev Date: Wed, 23 Mar 2016 18:45:03 +0300 Subject: [PATCH 1/4] [editor] Changeset comments --- editor/changeset_wrapper.cpp | 106 ++++++++++++++++++++++++++++++++--- editor/changeset_wrapper.hpp | 13 ++++- editor/server_api.cpp | 32 ++++++++--- editor/server_api.hpp | 1 + 4 files changed, 133 insertions(+), 19 deletions(-) diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp index 73e73e91ed..a426d38cc9 100644 --- a/editor/changeset_wrapper.cpp +++ b/editor/changeset_wrapper.cpp @@ -29,9 +29,34 @@ m2::RectD GetBoundingRect(vector const & geometry) return rect; } -bool OsmFeatureHasTags(pugi::xml_node const & osmFt) +bool OsmFeatureHasTags(pugi::xml_node const & osmFt) { return osmFt.child("tag"); } +vector const static kMainTags = {"amenity", "shop", "tourism", "historic", + "craft", "emergency", "barrier", "highway", + "office", "entrance", "building"}; + +string GetTypeForFeature(XMLFeature const & node) { - return osmFt.child("tag"); + for (string const & key : kMainTags) + { + if (node.HasTag(key)) + { + string value = node.GetTagValue(key); + if (value == "yes") + return key; + else if (key == "shop" || key == "office" || key == "building" || key == "entrance") + return value + " " + key; // "convenience shop" + else + return value; + } + } + + // Did not find any known tags. + bool foundTags = false; + node.ForEachTag([&foundTags](string const & k, string const & v) + { + foundTags = true; + }); + return foundTags ? "unknown object" : "empty object"; } vector NaiveSample(vector const & source, size_t count) @@ -74,7 +99,8 @@ namespace osm { ChangesetWrapper::ChangesetWrapper(TKeySecret const & keySecret, ServerApi06::TKeyValueTags const & comments) noexcept - : m_changesetComments(comments), m_api(OsmOAuth::ServerAuth(keySecret)) + : m_changesetComments(comments), + m_api(OsmOAuth::ServerAuth(keySecret)) { } @@ -84,6 +110,8 @@ ChangesetWrapper::~ChangesetWrapper() { try { + m_changesetComments["comment"] = GetDescription(); + m_api.UpdateChangeSet(m_changesetId, m_changesetComments); m_api.CloseChangeSet(m_changesetId); } catch (std::exception const & ex) @@ -100,7 +128,9 @@ void ChangesetWrapper::LoadXmlFromOSM(ms::LatLon const & ll, pugi::xml_document MYTHROW(HttpErrorException, ("HTTP error", response, "with GetXmlFeaturesAtLatLon", ll)); if (pugi::status_ok != doc.load(response.second.c_str()).status) - MYTHROW(OsmXmlParseException, ("Can't parse OSM server response for GetXmlFeaturesAtLatLon request", response.second)); + MYTHROW( + OsmXmlParseException, + ("Can't parse OSM server response for GetXmlFeaturesAtLatLon request", response.second)); } void ChangesetWrapper::LoadXmlFromOSM(m2::RectD const & rect, pugi::xml_document & doc) @@ -110,7 +140,8 @@ void ChangesetWrapper::LoadXmlFromOSM(m2::RectD const & rect, pugi::xml_document MYTHROW(HttpErrorException, ("HTTP error", response, "with GetXmlFeaturesInRect", rect)); if (pugi::status_ok != doc.load(response.second.c_str()).status) - MYTHROW(OsmXmlParseException, ("Can't parse OSM server response for GetXmlFeaturesInRect request", response.second)); + MYTHROW(OsmXmlParseException, + ("Can't parse OSM server response for GetXmlFeaturesInRect request", response.second)); } XMLFeature ChangesetWrapper::GetMatchingNodeFeatureFromOSM(m2::PointD const & center) @@ -171,8 +202,7 @@ XMLFeature ChangesetWrapper::GetMatchingAreaFeatureFromOSM(vector co stringstream sstr; bestWayOrRelation.print(sstr); LOG(LDEBUG, ("Relation is the best match", sstr.str())); - MYTHROW(RelationFeatureAreNotSupportedException, - ("Got relation as the best matching")); + MYTHROW(RelationFeatureAreNotSupportedException, ("Got relation as the best matching")); } if (!OsmFeatureHasTags(bestWayOrRelation)) @@ -204,6 +234,7 @@ void ChangesetWrapper::Create(XMLFeature node) node.SetAttribute("changeset", strings::to_string(m_changesetId)); // TODO(AlexZ): Think about storing/logging returned OSM ids. UNUSED_VALUE(m_api.CreateElement(node)); + m_created_types[GetTypeForFeature(node)]++; } void ChangesetWrapper::Modify(XMLFeature node) @@ -214,6 +245,7 @@ void ChangesetWrapper::Modify(XMLFeature node) // Changeset id should be updated for every OSM server commit. node.SetAttribute("changeset", strings::to_string(m_changesetId)); m_api.ModifyElement(node); + m_modified_types[GetTypeForFeature(node)]++; } void ChangesetWrapper::Delete(XMLFeature node) @@ -226,4 +258,62 @@ void ChangesetWrapper::Delete(XMLFeature node) m_api.DeleteElement(node); } -} // namespace osm +string ChangesetWrapper::TypeCountToString(TTypeCount const & typeCount) const +{ + if (typeCount.empty()) + return string(); + if (typeCount.size() > 3) + { + // TODO(zverik): Instead return top 2 + "and XX other features". + int count = 0; + for (auto const & tc : typeCount) + count += tc.second; + return strings::to_string(count) + " objects"; + } + + // TODO(zverik): Reverse sort by count + vector> items; + for (auto const & tc : typeCount) + items.push_back(tc); + + ostringstream ss; + for (auto i = 0; i < items.size(); ++i) + { + if (i > 0) + { + // Separator: "A and B" for two, "A, B, and C" for three or more. + if (items.size() > 2) + ss << ", "; + else + ss << " "; + if (i == items.size() - 1) + ss << "and "; + } + + // Format a count: "a shop" for single shop, "4 shops" for multiple. + if (items[i].second == 1) + ss << "a "; + else + ss << items[i].second << ' '; + ss << items[i].first; + if (items[i].second > 1) + ss << 's'; + } + return ss.str(); +} + +string ChangesetWrapper::GetDescription() const +{ + string result; + if (!m_created_types.empty()) + result = "Created " + TypeCountToString(m_created_types); + if (!m_modified_types.empty()) + { + if (!result.empty()) + result += "; "; + result += "Updated " + TypeCountToString(m_modified_types); + } + return result; +} + +} // namespace osm diff --git a/editor/changeset_wrapper.hpp b/editor/changeset_wrapper.hpp index 3cc09f251d..c1d0d5583c 100644 --- a/editor/changeset_wrapper.hpp +++ b/editor/changeset_wrapper.hpp @@ -19,6 +19,8 @@ struct ClientToken; class ChangesetWrapper { + using TTypeCount = map; + public: DECLARE_EXCEPTION(ChangesetWrapperException, RootException); DECLARE_EXCEPTION(NetworkErrorException, ChangesetWrapperException); @@ -32,7 +34,8 @@ public: DECLARE_EXCEPTION(RelationFeatureAreNotSupportedException, ChangesetWrapperException); DECLARE_EXCEPTION(EmptyFeatureException, ChangesetWrapperException); - ChangesetWrapper(TKeySecret const & keySecret, ServerApi06::TKeyValueTags const & comments) noexcept; + ChangesetWrapper(TKeySecret const & keySecret, + ServerApi06::TKeyValueTags const & comments) noexcept; ~ChangesetWrapper(); /// Throws many exceptions from above list, plus including XMLNode's parsing ones. @@ -60,6 +63,12 @@ private: ServerApi06 m_api; static constexpr uint64_t kInvalidChangesetId = 0; uint64_t m_changesetId = kInvalidChangesetId; + + // No m_deleted_types here, since we do not delete features. + TTypeCount m_modified_types; + TTypeCount m_created_types; + string TypeCountToString(TTypeCount const & typeCount) const; + string GetDescription() const; }; -} // namespace osm +} // namespace osm diff --git a/editor/server_api.cpp b/editor/server_api.cpp index fad349603c..513fb04a02 100644 --- a/editor/server_api.cpp +++ b/editor/server_api.cpp @@ -12,6 +12,21 @@ #include "3party/pugixml/src/pugixml.hpp" +namespace +{ +string KeyValueTagsToXML(osm::ServerApi06::TKeyValueTags const & kvTags) +{ + ostringstream stream; + stream << "\n" + "\n"; + for (auto const & tag : kvTags) + stream << " \n"; + stream << "\n" + "\n"; + return stream.str(); +} +} // namespace + namespace osm { @@ -25,15 +40,7 @@ uint64_t ServerApi06::CreateChangeSet(TKeyValueTags const & kvTags) const if (!m_auth.IsAuthorized()) MYTHROW(NotAuthorized, ("Not authorized.")); - ostringstream stream; - stream << "\n" - "\n"; - for (auto const & tag : kvTags) - stream << " \n"; - stream << "\n" - "\n"; - - OsmOAuth::Response const response = m_auth.Request("/changeset/create", "PUT", stream.str()); + OsmOAuth::Response const response = m_auth.Request("/changeset/create", "PUT", KeyValueTagsToXML(kvTags)); if (response.first != OsmOAuth::HTTP::OK) MYTHROW(CreateChangeSetHasFailed, ("CreateChangeSet request has failed:", response)); @@ -96,6 +103,13 @@ void ServerApi06::DeleteElement(editor::XMLFeature const & element) const MYTHROW(ErrorDeletingElement, ("Could not delete an element:", response)); } +void ServerApi06::UpdateChangeSet(uint64_t changesetId, TKeyValueTags const & kvTags) const +{ + OsmOAuth::Response const response = m_auth.Request("/changeset/" + strings::to_string(changesetId), "PUT", KeyValueTagsToXML(kvTags)); + if (response.first != OsmOAuth::HTTP::OK) + MYTHROW(CreateChangeSetHasFailed, ("UpdateChangeSet request has failed:", response)); +} + void ServerApi06::CloseChangeSet(uint64_t changesetId) const { OsmOAuth::Response const response = m_auth.Request("/changeset/" + strings::to_string(changesetId) + "/close", "PUT"); diff --git a/editor/server_api.hpp b/editor/server_api.hpp index e69983b5a5..57027d0462 100644 --- a/editor/server_api.hpp +++ b/editor/server_api.hpp @@ -70,6 +70,7 @@ public: /// @param element should already have all attributes set, including "id", "version", "changeset". /// @returns true if element was successfully deleted (or was already deleted). void DeleteElement(editor::XMLFeature const & element) const; + void UpdateChangeSet(uint64_t changesetId, TKeyValueTags const & kvTags) const; void CloseChangeSet(uint64_t changesetId) const; /// @returns id of a created note. uint64_t CreateNote(ms::LatLon const & ll, string const & message) const; From 12c03180d0dff8fa9f14076f390d70dd3531acb3 Mon Sep 17 00:00:00 2001 From: Ilya Zverev Date: Thu, 24 Mar 2016 11:12:37 +0300 Subject: [PATCH 2/4] [editor] Sort types in changeset comments --- editor/changeset_wrapper.cpp | 41 ++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp index a426d38cc9..ee167efb6c 100644 --- a/editor/changeset_wrapper.cpp +++ b/editor/changeset_wrapper.cpp @@ -262,41 +262,50 @@ string ChangesetWrapper::TypeCountToString(TTypeCount const & typeCount) const { if (typeCount.empty()) return string(); - if (typeCount.size() > 3) - { - // TODO(zverik): Instead return top 2 + "and XX other features". - int count = 0; - for (auto const & tc : typeCount) - count += tc.second; - return strings::to_string(count) + " objects"; - } - // TODO(zverik): Reverse sort by count + // Convert map to vector and sort pairs by count, descending. vector> items; for (auto const & tc : typeCount) items.push_back(tc); + sort(items.begin(), items.end(), + [](pair const & a, pair const & b) + { + return a.second > b.second; + }); + ostringstream ss; - for (auto i = 0; i < items.size(); ++i) + auto limit = items.size() > 3 ? 3 : items.size(); + for (auto i = 0; i < limit; ++i) { if (i > 0) { // Separator: "A and B" for two, "A, B, and C" for three or more. - if (items.size() > 2) + if (limit > 2) ss << ", "; else ss << " "; - if (i == items.size() - 1) + if (i == limit - 1) ss << "and "; } + auto & currentPair = items[i]; + // If we have more objects left, make the last one a list of these. + if (i == limit - 1 && limit < items.size()) + { + int count = 0; + for (auto j = i; j < items.size(); ++j) + count += items[j].second; + currentPair = {"other object", count}; + } + // Format a count: "a shop" for single shop, "4 shops" for multiple. - if (items[i].second == 1) + if (currentPair.second == 1) ss << "a "; else - ss << items[i].second << ' '; - ss << items[i].first; - if (items[i].second > 1) + ss << currentPair.second << ' '; + ss << currentPair.first; + if (currentPair.second > 1) ss << 's'; } return ss.str(); From 99fec047b33e0ae7c2ec379e531c1492faef8b09 Mon Sep 17 00:00:00 2001 From: Ilya Zverev Date: Thu, 24 Mar 2016 12:07:59 +0300 Subject: [PATCH 3/4] [editor] Review fixes, and a new method HasAnyTags() in XMLFeature --- editor/changeset_wrapper.cpp | 17 ++++++++--------- editor/xml_feature.cpp | 5 +++++ editor/xml_feature.hpp | 1 + 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp index ee167efb6c..c8b7d26a5e 100644 --- a/editor/changeset_wrapper.cpp +++ b/editor/changeset_wrapper.cpp @@ -29,7 +29,11 @@ m2::RectD GetBoundingRect(vector const & geometry) return rect; } -bool OsmFeatureHasTags(pugi::xml_node const & osmFt) { return osmFt.child("tag"); } +bool OsmFeatureHasTags(pugi::xml_node const & osmFt) +{ + return osmFt.child("tag"); +} + vector const static kMainTags = {"amenity", "shop", "tourism", "historic", "craft", "emergency", "barrier", "highway", "office", "entrance", "building"}; @@ -40,7 +44,7 @@ string GetTypeForFeature(XMLFeature const & node) { if (node.HasTag(key)) { - string value = node.GetTagValue(key); + string const value = node.GetTagValue(key); if (value == "yes") return key; else if (key == "shop" || key == "office" || key == "building" || key == "entrance") @@ -51,12 +55,7 @@ string GetTypeForFeature(XMLFeature const & node) } // Did not find any known tags. - bool foundTags = false; - node.ForEachTag([&foundTags](string const & k, string const & v) - { - foundTags = true; - }); - return foundTags ? "unknown object" : "empty object"; + return node.HasAnyTags() ? "unknown object" : "empty object"; } vector NaiveSample(vector const & source, size_t count) @@ -275,7 +274,7 @@ string ChangesetWrapper::TypeCountToString(TTypeCount const & typeCount) const }); ostringstream ss; - auto limit = items.size() > 3 ? 3 : items.size(); + auto const limit = items.size() > 3 ? 3 : items.size(); for (auto i = 0; i < limit; ++i) { if (i > 0) diff --git a/editor/xml_feature.cpp b/editor/xml_feature.cpp index 3b7abad3b6..7117f97de2 100644 --- a/editor/xml_feature.cpp +++ b/editor/xml_feature.cpp @@ -314,6 +314,11 @@ void XMLFeature::SetUploadError(string const & error) SetAttribute(kUploadError, error); } +bool XMLFeature::HasAnyTags() const +{ + return m_document.child("tag"); +} + bool XMLFeature::HasTag(string const & key) const { return FindTag(m_document, key); diff --git a/editor/xml_feature.hpp b/editor/xml_feature.hpp index 09043e9f75..47c82f341b 100644 --- a/editor/xml_feature.hpp +++ b/editor/xml_feature.hpp @@ -138,6 +138,7 @@ public: void SetUploadError(string const & error); //@} + bool HasAnyTags() const; bool HasTag(string const & key) const; bool HasAttribute(string const & key) const; bool HasKey(string const & key) const; From 24b65aac4290d6a1377679ea6e7be65aad23c648 Mon Sep 17 00:00:00 2001 From: Ilya Zverev Date: Thu, 24 Mar 2016 13:23:10 +0300 Subject: [PATCH 4/4] [editor] Review fixes --- editor/changeset_wrapper.cpp | 15 +++++++++++---- editor/changeset_wrapper.hpp | 6 +++--- editor/editor_tests/server_api_test.cpp | 4 ++++ editor/server_api.cpp | 2 +- editor/server_api.hpp | 1 + 5 files changed, 20 insertions(+), 8 deletions(-) diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp index c8b7d26a5e..94c06f813e 100644 --- a/editor/changeset_wrapper.cpp +++ b/editor/changeset_wrapper.cpp @@ -255,26 +255,27 @@ void ChangesetWrapper::Delete(XMLFeature node) // Changeset id should be updated for every OSM server commit. node.SetAttribute("changeset", strings::to_string(m_changesetId)); m_api.DeleteElement(node); + m_deleted_types[GetTypeForFeature(node)]++; } -string ChangesetWrapper::TypeCountToString(TTypeCount const & typeCount) const +string ChangesetWrapper::TypeCountToString(TTypeCount const & typeCount) { if (typeCount.empty()) return string(); // Convert map to vector and sort pairs by count, descending. - vector> items; + vector> items; for (auto const & tc : typeCount) items.push_back(tc); sort(items.begin(), items.end(), - [](pair const & a, pair const & b) + [](pair const & a, pair const & b) { return a.second > b.second; }); ostringstream ss; - auto const limit = items.size() > 3 ? 3 : items.size(); + auto const limit = min(3UL, items.size()); for (auto i = 0; i < limit; ++i) { if (i > 0) @@ -321,6 +322,12 @@ string ChangesetWrapper::GetDescription() const result += "; "; result += "Updated " + TypeCountToString(m_modified_types); } + if (!m_deleted_types.empty()) + { + if (!result.empty()) + result += "; "; + result += "Deleted " + TypeCountToString(m_deleted_types); + } return result; } diff --git a/editor/changeset_wrapper.hpp b/editor/changeset_wrapper.hpp index c1d0d5583c..b199d8f095 100644 --- a/editor/changeset_wrapper.hpp +++ b/editor/changeset_wrapper.hpp @@ -19,7 +19,7 @@ struct ClientToken; class ChangesetWrapper { - using TTypeCount = map; + using TTypeCount = map; public: DECLARE_EXCEPTION(ChangesetWrapperException, RootException); @@ -64,10 +64,10 @@ private: static constexpr uint64_t kInvalidChangesetId = 0; uint64_t m_changesetId = kInvalidChangesetId; - // No m_deleted_types here, since we do not delete features. TTypeCount m_modified_types; TTypeCount m_created_types; - string TypeCountToString(TTypeCount const & typeCount) const; + TTypeCount m_deleted_types; + static string TypeCountToString(TTypeCount const & typeCount); string GetDescription() const; }; diff --git a/editor/editor_tests/server_api_test.cpp b/editor/editor_tests/server_api_test.cpp index 83d6e652f9..4962155e2c 100644 --- a/editor/editor_tests/server_api_test.cpp +++ b/editor/editor_tests/server_api_test.cpp @@ -95,6 +95,10 @@ UNIT_TEST(OSM_ServerAPI_ChangesetAndNode) // After modification, node version increases in ModifyElement. TEST_EQUAL(node.GetAttribute("version"), "2", ()); + // All tags must be specified, because there is no merging of old and new tags. + api.UpdateChangeSet(changeSetId, {{"created_by", "MAPS.ME Unit Test"}, + {"comment", "For test purposes only (updated)."}}); + // To retrieve created node, changeset should be closed first. // It is done here via Scope Guard. } diff --git a/editor/server_api.cpp b/editor/server_api.cpp index 513fb04a02..1f5fb6b8d4 100644 --- a/editor/server_api.cpp +++ b/editor/server_api.cpp @@ -107,7 +107,7 @@ void ServerApi06::UpdateChangeSet(uint64_t changesetId, TKeyValueTags const & kv { OsmOAuth::Response const response = m_auth.Request("/changeset/" + strings::to_string(changesetId), "PUT", KeyValueTagsToXML(kvTags)); if (response.first != OsmOAuth::HTTP::OK) - MYTHROW(CreateChangeSetHasFailed, ("UpdateChangeSet request has failed:", response)); + MYTHROW(UpdateChangeSetHasFailed, ("UpdateChangeSet request has failed:", response)); } void ServerApi06::CloseChangeSet(uint64_t changesetId) const diff --git a/editor/server_api.hpp b/editor/server_api.hpp index 57027d0462..59bc82c4c7 100644 --- a/editor/server_api.hpp +++ b/editor/server_api.hpp @@ -34,6 +34,7 @@ public: DECLARE_EXCEPTION(NotAuthorized, ServerApi06Exception); DECLARE_EXCEPTION(CantParseServerResponse, ServerApi06Exception); DECLARE_EXCEPTION(CreateChangeSetHasFailed, ServerApi06Exception); + DECLARE_EXCEPTION(UpdateChangeSetHasFailed, ServerApi06Exception); DECLARE_EXCEPTION(CreateElementHasFailed, ServerApi06Exception); DECLARE_EXCEPTION(ModifiedElementHasNoIdAttribute, ServerApi06Exception); DECLARE_EXCEPTION(ModifyElementHasFailed, ServerApi06Exception);