notes fix

Author: Arsentiy Milchakov

editor/editor_notes.cpp

@@ -11,14 +11,14 @@
 #include "base/string_utils.hpp"
 #include "base/timer.hpp"
 
-#include "std/chrono.hpp"
-#include "std/future.hpp"
+#include <chrono>
+#include <future>
 
 #include "3party/pugixml/src/pugixml.hpp"
 
 namespace
 {
-bool LoadFromXml(pugi::xml_document const & xml, list<editor::Note> & notes,
+bool LoadFromXml(pugi::xml_document const & xml, std::list<editor::Note> & notes,
                  uint32_t & uploadedNotesCount)
 {
   uint64_t notesCount;
@@ -55,7 +55,7 @@ bool LoadFromXml(pugi::xml_document const & xml, list<editor::Note> & notes,
   return true;
 }
 
-void SaveToXml(list<editor::Note> const & notes, pugi::xml_document & xml,
+void SaveToXml(std::list<editor::Note> const & notes, pugi::xml_document & xml,
                uint32_t const uploadedNotesCount)
 {
   auto constexpr kDigitsAfterComma = 7;
@@ -74,9 +74,10 @@ void SaveToXml(list<editor::Note> const & notes, pugi::xml_document & xml,
 }
 
 /// Not thread-safe, use only for initialization.
-bool Load(string const & fileName, list<editor::Note> & notes, uint32_t & uploadedNotesCount)
+bool Load(std::string const & fileName, std::list<editor::Note> & notes,
+          uint32_t & uploadedNotesCount)
 {
-  string content;
+  std::string content;
   try
   {
     auto const reader = GetPlatform().GetReader(fileName);
@@ -111,30 +112,31 @@ bool Load(string const & fileName, list<editor::Note> & notes, uint32_t & upload
 }
 
 /// Not thread-safe, use synchronization.
-bool Save(string const & fileName, list<editor::Note> const & notes,
+bool Save(std::string const & fileName, std::list<editor::Note> const & notes,
           uint32_t const uploadedNotesCount)
 {
   pugi::xml_document xml;
   SaveToXml(notes, xml, uploadedNotesCount);
-  return my::WriteToTempAndRenameToFile(
-      fileName, [&xml](string const & fileName) { return xml.save_file(fileName.data(), "  "); });
+  return my::WriteToTempAndRenameToFile(fileName, [&xml](std::string const & fileName) {
+    return xml.save_file(fileName.data(), "  ");
+  });
 }
 }  // namespace
 
 namespace editor
 {
-shared_ptr<Notes> Notes::MakeNotes(string const & fileName, bool const fullPath)
+std::shared_ptr<Notes> Notes::MakeNotes(std::string const & fileName, bool const fullPath)
 {
-  return shared_ptr<Notes>(
+  return std::shared_ptr<Notes>(
       new Notes(fullPath ? fileName : GetPlatform().WritablePathForFile(fileName)));
 }
 
-Notes::Notes(string const & fileName) : m_fileName(fileName)
+Notes::Notes(std::string const & fileName) : m_fileName(fileName)
 {
   Load(m_fileName, m_notes, m_uploadedNotesCount);
 }
 
-void Notes::CreateNote(ms::LatLon const & latLon, string const & text)
+void Notes::CreateNote(ms::LatLon const & latLon, std::string const & text)
 {
   if (text.empty())
   {
@@ -148,7 +150,14 @@ void Notes::CreateNote(ms::LatLon const & latLon, string const & text)
     return;
   }
 
-  lock_guard<mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_mu);
+  auto const it = std::find_if(m_notes.begin(), m_notes.end(), [&latLon, &text](Note const & note) {
+    return latLon.EqualDxDy(note.m_point, kTolerance) && text == note.m_note;
+  });
+  // No need to add the same note. It works in case when saved note are not uploaded yet.
+  if (it != m_notes.end())
+    return;
+
   m_notes.emplace_back(latLon, text);
   Save(m_fileName, m_notes, m_uploadedNotesCount);
 }
@@ -159,20 +168,19 @@ void Notes::Upload(osm::OsmOAuth const & auth)
   auto const self = shared_from_this();
 
   auto const doUpload = [self, auth]() {
-    size_t size;
-
-    {
-      lock_guard<mutex> g(self->m_mu);
-      size = self->m_notes.size();
-    }
-
+    std::unique_lock<std::mutex> ulock(self->m_mu);
+    // Size of m_notes is decreased only in this method.
+    size_t size = notes.size();
+    auto & notes = self->m_notes;
     osm::ServerApi06 api(auth);
-    auto it = begin(self->m_notes);
-    for (size_t i = 0; i != size; ++i, ++it)
+
+    while (size > 0)
     {
       try
       {
-        auto const id = api.CreateNote(it->m_point, it->m_note);
+        ulock.unlock();
+        auto const id = api.CreateNote(notes.front().m_point, notes.front().m_note);
+        ulock.lock();
         LOG(LINFO, ("A note uploaded with id", id));
       }
       catch (osm::ServerApi06::ServerApi06Exception const & e)
@@ -182,35 +190,34 @@ void Notes::Upload(osm::OsmOAuth const & auth)
         return;
       }
 
-      lock_guard<mutex> g(self->m_mu);
-      it = self->m_notes.erase(it);
+      notes.pop_front();
+      --size;
       ++self->m_uploadedNotesCount;
       Save(self->m_fileName, self->m_notes, self->m_uploadedNotesCount);
     }
   };
 
-  // Do not run more than one upload thread at a time.
-  static auto future = async(launch::async, doUpload);
-  auto const status = future.wait_for(milliseconds(0));
-  if (status == future_status::ready)
-    future = async(launch::async, doUpload);
+  static auto future = std::async(std::launch::async, doUpload);
+  auto const status = future.wait_for(std::chrono::milliseconds(0));
+  if (status == std::future_status::ready)
+    future = std::async(std::launch::async, doUpload);
 }
 
-vector<Note> const Notes::GetNotes() const
+std::list<Note> Notes::GetNotes() const
 {
-  lock_guard<mutex> g(m_mu);
-  return {begin(m_notes), end(m_notes)};
+  std::lock_guard<std::mutex> g(m_mu);
+  return m_notes;
 }
 
 size_t Notes::NotUploadedNotesCount() const
 {
-  lock_guard<mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_mu);
   return m_notes.size();
 }
 
 size_t Notes::UploadedNotesCount() const
 {
-  lock_guard<mutex> g(m_mu);
+  std::lock_guard<std::mutex> g(m_mu);
   return m_uploadedNotesCount;
 }
 }  // namespace editor

editor/editor_notes.hpp

@@ -6,50 +6,52 @@
 
 #include "base/macros.hpp"
 
-#include "std/list.hpp"
-#include "std/mutex.hpp"
-#include "std/shared_ptr.hpp"
-#include "std/string.hpp"
+#include <list>
+#include <memory>
+#include <mutex>
+#include <string>
 
 namespace editor
 {
 struct Note
 {
-  Note(ms::LatLon const & point, string const & text) : m_point(point), m_note(text) {}
+  Note(ms::LatLon const & point, std::string const & text) : m_point(point), m_note(text) {}
   ms::LatLon m_point;
-  string m_note;
+  std::string m_note;
 };
 
 inline bool operator==(Note const & a, Note const & b)
 {
   return a.m_point == b.m_point && b.m_note == b.m_note;
 }
 
-class Notes : public enable_shared_from_this<Notes>
+class Notes : public std::enable_shared_from_this<Notes>
 {
 public:
-  static shared_ptr<Notes> MakeNotes(string const & fileName = "notes.xml",
-                                     bool const fullPath = false);
+  static float constexpr kTolerance = 1e-7;
+  static std::shared_ptr<Notes> MakeNotes(std::string const & fileName = "notes.xml",
+                                          bool const fullPath = false);
 
-  void CreateNote(ms::LatLon const & latLon, string const & text);
+  void CreateNote(ms::LatLon const & latLon, std::string const & text);
 
   /// Uploads notes to the server in a separate thread.
+  /// Called on main thread from system event.
   void Upload(osm::OsmOAuth const & auth);
 
-  vector<Note> const GetNotes() const;
+  std::list<Note> GetNotes() const;
 
   size_t NotUploadedNotesCount() const;
   size_t UploadedNotesCount() const;
 
 private:
-  Notes(string const & fileName);
+  explicit Notes(std::string const & fileName);
 
-  string const m_fileName;
-  mutable mutex m_mu;
+  std::string const m_fileName;
+  mutable std::mutex m_mu;
 
   // m_notes keeps the notes that have not been uploaded yet.
   // Once a note has been uploaded, it is removed from m_notes.
-  list<Note> m_notes;
+  std::list<Note> m_notes;
 
   uint32_t m_uploadedNotesCount = 0;
 

editor/editor_tests/editor_notes_test.cpp

@@ -27,16 +27,14 @@ UNIT_TEST(Notes_Smoke)
     auto const notes = Notes::MakeNotes(fullFileName, true);
     auto const result = notes->GetNotes();
     TEST_EQUAL(result.size(), 3, ());
-    vector<Note> const expected {
-      {MercatorBounds::ToLatLon({1, 2}), "Some note1"},
-      {MercatorBounds::ToLatLon({2, 2}), "Some note2"},
-      {MercatorBounds::ToLatLon({1, 1}), "Some note3"}
-    };
-    for (auto i = 0; i < result.size(); ++i)
-    {
-      TEST(my::AlmostEqualAbs(result[i].m_point.lat, expected[i].m_point.lat, 1e-7), ());
-      TEST(my::AlmostEqualAbs(result[i].m_point.lon, expected[i].m_point.lon, 1e-7), ());
-      TEST_EQUAL(result[i].m_note, expected[i].m_note, ());
-    }
+    std::vector<Note> const expected{{MercatorBounds::ToLatLon({1, 2}), "Some note1"},
+                                     {MercatorBounds::ToLatLon({2, 2}), "Some note2"},
+                                     {MercatorBounds::ToLatLon({1, 1}), "Some note3"}};
+
+    auto const isEqual = std::equal(
+        result.begin(), result.end(), expected.begin(), [](Note const & lhs, Note const & rhs) {
+          return lhs.m_point.EqualDxDy(rhs.m_point, Notes::kTolerance);
+        });
+    TEST(isEqual, ());
   }
 }