diff --git a/editor/editor_notes.cpp b/editor/editor_notes.cpp index 9c25db4e85..88a9c661fc 100644 --- a/editor/editor_notes.cpp +++ b/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 +#include #include "3party/pugixml/src/pugixml.hpp" namespace { -bool LoadFromXml(pugi::xml_document const & xml, list & notes, +bool LoadFromXml(pugi::xml_document const & xml, std::list & notes, uint32_t & uploadedNotesCount) { uint64_t notesCount; @@ -55,7 +55,7 @@ bool LoadFromXml(pugi::xml_document const & xml, list & notes, return true; } -void SaveToXml(list const & notes, pugi::xml_document & xml, +void SaveToXml(std::list const & notes, pugi::xml_document & xml, uint32_t const uploadedNotesCount) { auto constexpr kDigitsAfterComma = 7; @@ -74,9 +74,10 @@ void SaveToXml(list const & notes, pugi::xml_document & xml, } /// Not thread-safe, use only for initialization. -bool Load(string const & fileName, list & notes, uint32_t & uploadedNotesCount) +bool Load(std::string const & fileName, std::list & 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 & notes, uint32_t & upload } /// Not thread-safe, use synchronization. -bool Save(string const & fileName, list const & notes, +bool Save(std::string const & fileName, std::list 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::MakeNotes(string const & fileName, bool const fullPath) +std::shared_ptr Notes::MakeNotes(std::string const & fileName, bool const fullPath) { - return shared_ptr( + return std::shared_ptr( 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 g(m_mu); + std::lock_guard 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 g(self->m_mu); - size = self->m_notes.size(); - } - + std::unique_lock 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 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 const Notes::GetNotes() const +std::list Notes::GetNotes() const { - lock_guard g(m_mu); - return {begin(m_notes), end(m_notes)}; + std::lock_guard g(m_mu); + return m_notes; } size_t Notes::NotUploadedNotesCount() const { - lock_guard g(m_mu); + std::lock_guard g(m_mu); return m_notes.size(); } size_t Notes::UploadedNotesCount() const { - lock_guard g(m_mu); + std::lock_guard g(m_mu); return m_uploadedNotesCount; } } // namespace editor diff --git a/editor/editor_notes.hpp b/editor/editor_notes.hpp index 34ea5b24b4..40ca8a30c0 100644 --- a/editor/editor_notes.hpp +++ b/editor/editor_notes.hpp @@ -6,18 +6,18 @@ #include "base/macros.hpp" -#include "std/list.hpp" -#include "std/mutex.hpp" -#include "std/shared_ptr.hpp" -#include "std/string.hpp" +#include +#include +#include +#include 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) @@ -25,31 +25,33 @@ 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 +class Notes : public std::enable_shared_from_this { public: - static shared_ptr MakeNotes(string const & fileName = "notes.xml", - bool const fullPath = false); + static float constexpr kTolerance = 1e-7; + static std::shared_ptr 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 const GetNotes() const; + std::list 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 m_notes; + std::list m_notes; uint32_t m_uploadedNotesCount = 0; diff --git a/editor/editor_tests/editor_notes_test.cpp b/editor/editor_tests/editor_notes_test.cpp index a6b6496063..a958b2360f 100644 --- a/editor/editor_tests/editor_notes_test.cpp +++ b/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 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 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, ()); } }