Merge pull request #2451 from Zverik/comments2

[editor] Changeset comments

7 files changed, 157 insertions, 17 deletions

diff --git a/editor/changeset_wrapper.cpp b/editor/changeset_wrapper.cpp
index 2b00a1e4d8..d151fca0b9 100644
--- a/editor/changeset_wrapper.cpp
+++ b/editor/changeset_wrapper.cpp
@@ -34,6 +34,30 @@ bool OsmFeatureHasTags(pugi::xml_node const & osmFt)
   return osmFt.child("tag");
 }
 
+vector<string> const static kMainTags = {"amenity", "shop",      "tourism", "historic",
+                                         "craft",   "emergency", "barrier", "highway",
+                                         "office",  "entrance",  "building"};
+
+string GetTypeForFeature(XMLFeature const & node)
+{
+  for (string const & key : kMainTags)
+  {
+    if (node.HasTag(key))
+    {
+      string const 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.
+  return node.HasAnyTags() ? "unknown object" : "empty object";
+}
+
 vector<m2::PointD> NaiveSample(vector<m2::PointD> const & source, size_t count)
 {
   count = min(count, source.size());
@@ -74,7 +98,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 +109,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 +127,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(ms::LatLon const & min, ms::LatLon const & max, pugi::xml_document & doc)
@@ -110,7 +139,8 @@ void ChangesetWrapper::LoadXmlFromOSM(ms::LatLon const & min, ms::LatLon const &
     MYTHROW(HttpErrorException, ("HTTP error", response, "with GetXmlFeaturesInRect", min, max));
 
   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 +201,7 @@ XMLFeature ChangesetWrapper::GetMatchingAreaFeatureFromOSM(vector<m2::PointD> 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 +233,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 +244,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)
@@ -224,6 +255,80 @@ 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)
+{
+  if (typeCount.empty())
+    return string();
+
+  // Convert map to vector and sort pairs by count, descending.
+  vector<pair<string, size_t>> items;
+  for (auto const & tc : typeCount)
+    items.push_back(tc);
+
+  sort(items.begin(), items.end(),
+       [](pair<string, size_t> const & a, pair<string, size_t> const & b)
+       {
+         return a.second > b.second;
+       });
+
+  ostringstream ss;
+  auto const limit = min(3UL, 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 (limit > 2)
+        ss << ", ";
+      else
+        ss << " ";
+      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 (currentPair.second == 1)
+      ss << "a ";
+    else
+      ss << currentPair.second << ' ';
+    ss << currentPair.first;
+    if (currentPair.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);
+  }
+  if (!m_deleted_types.empty())
+  {
+    if (!result.empty())
+      result += "; ";
+    result += "Deleted " + TypeCountToString(m_deleted_types);
+  }
+  return result;
 }
 
-} // namespace osm
+}  // namespace osm
diff --git a/editor/changeset_wrapper.hpp b/editor/changeset_wrapper.hpp
index 375cd2b279..4081044e94 100644
--- a/editor/changeset_wrapper.hpp
+++ b/editor/changeset_wrapper.hpp
@@ -19,6 +19,8 @@ struct ClientToken;
 
 class ChangesetWrapper
 {
+  using TTypeCount = map<string, size_t>;
+
 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;
+
+  TTypeCount m_modified_types;
+  TTypeCount m_created_types;
+  TTypeCount m_deleted_types;
+  static string TypeCountToString(TTypeCount const & typeCount);
+  string GetDescription() const;
 };
 
-} // namespace osm
+}  // namespace osm
diff --git a/editor/editor_tests/server_api_test.cpp b/editor/editor_tests/server_api_test.cpp
index 20c4fa0110..e4b69e8122 100644
--- a/editor/editor_tests/server_api_test.cpp
+++ b/editor/editor_tests/server_api_test.cpp
@@ -96,6 +96,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 4d511e6df2..f20ce26394 100644
--- a/editor/server_api.cpp
+++ b/editor/server_api.cpp
@@ -13,6 +13,21 @@
 
 #include "3party/pugixml/src/pugixml.hpp"
 
+namespace
+{
+string KeyValueTagsToXML(osm::ServerApi06::TKeyValueTags const & kvTags)
+{
+  ostringstream stream;
+  stream << "<osm>\n"
+  "<changeset>\n";
+  for (auto const & tag : kvTags)
+    stream << "  <tag k=\"" << tag.first << "\" v=\"" << tag.second << "\"/>\n";
+  stream << "</changeset>\n"
+  "</osm>\n";
+  return stream.str();
+}
+}  // namespace
+
 namespace osm
 {
 
@@ -26,15 +41,7 @@ uint64_t ServerApi06::CreateChangeSet(TKeyValueTags const & kvTags) const
   if (!m_auth.IsAuthorized())
     MYTHROW(NotAuthorized, ("Not authorized."));
 
-  ostringstream stream;
-  stream << "<osm>\n"
-  "<changeset>\n";
-  for (auto const & tag : kvTags)
-    stream << "  <tag k=\"" << tag.first << "\" v=\"" << tag.second << "\"/>\n";
-  stream << "</changeset>\n"
-  "</osm>\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));
 
@@ -97,6 +104,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(UpdateChangeSetHasFailed, ("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 1ff1b697a8..d5cfaa5380 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);
@@ -70,6 +71,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;
diff --git a/editor/xml_feature.cpp b/editor/xml_feature.cpp
index 2367f036eb..c125725efb 100644
--- a/editor/xml_feature.cpp
+++ b/editor/xml_feature.cpp
@@ -323,6 +323,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 f6f09240fd..720ed81e68 100644
--- a/editor/xml_feature.hpp
+++ b/editor/xml_feature.hpp
@@ -140,6 +140,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;