AndrewShkrob/organicmaps
1
Fork 0
forked from organicmaps/organicmaps
Code Pull requests Activity

Code review. Fix data loss.

Browse source
This commit is contained in:
Sergey Magidovich 2016-03-11 17:34:10 +03:00 • committed by Sergey Yershov
parent f9e8c97c6f
commit 9cec925752
10 changed files with 207 additions and 198 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

21
coding/internal/file_data.cpp
View file

@ -6,12 +6,14 @@
#include "base/exception.hpp"
#include "base/logging.hpp"
#include "base/string_utils.hpp"
#include "std/cerrno.hpp"
#include "std/cstring.hpp"
#include "std/exception.hpp"
#include "std/fstream.hpp"
#include "std/target_os.hpp"
#include "std/thread.hpp"
#ifdef OMIM_OS_WINDOWS
#include <io.h>
@ -267,6 +269,25 @@ bool RenameFileX(string const & fOld, string const & fNew)
return CheckFileOperationResult(res, fOld);
}
bool WriteToTempAndRenameToFile(string const & dest, function<bool(string const &)> const & write,
string const & tmp)
{
string const tmpFileName =
tmp.empty() ? dest + ".tmp" + strings::to_string(this_thread::get_id()) : tmp;
if (!write(tmpFileName))
{
LOG(LERROR, ("Can't write to", tmpFileName));
return false;
}
if (!RenameFileX(tmpFileName, dest))
{
LOG(LERROR, ("Can't rename file", tmpFileName, "to", dest));
DeleteFileX(tmpFileName);
return false;
}
return true;
}
bool CopyFileX(string const & fOld, string const & fNew)
{
try

10
coding/internal/file_data.hpp
View file

@ -3,9 +3,10 @@
#include "base/base.hpp"
#include "std/functional.hpp"
#include "std/noncopyable.hpp"
#include "std/string.hpp"
#include "std/target_os.hpp"
#include "std/noncopyable.hpp"
#ifdef OMIM_OS_TIZEN
namespace Tizen
@ -58,8 +59,13 @@ private:
bool GetFileSize(string const & fName, uint64_t & sz);
bool DeleteFileX(string const & fName);
bool RenameFileX(string const & fOld, string const & fNew);
/// Write to temp file and rename it to dest. Delete temp on failure.
/// @param write function that writes to file with a given name, returns true on success.
bool WriteToTempAndRenameToFile(string const & dest, function<bool(string const &)> const & write,
string const & tmp = "");
/// @return false if copy fails. DO NOT THROWS exceptions
bool CopyFileX(string const & fOld, string const & fNew);
bool IsEqualFiles(string const & firstFile, string const & secondFile);
}

241
editor/editor_notes.cpp
View file

@ -6,9 +6,9 @@
#include "geometry/mercator.hpp"
#include "base/string_utils.hpp"
#include "base/assert.hpp"
#include "base/logging.hpp"
#include "base/string_utils.hpp"
#include "base/timer.hpp"
#include "std/future.hpp"
@ -17,174 +17,187 @@
namespace
{
bool LoadFromXml(pugi::xml_document const & xml,
vector<editor::Note> & notes,
bool LoadFromXml(pugi::xml_document const & xml, list<editor::Note> & notes,
uint32_t & uploadedNotesCount)
{
uint64_t notesCount;
auto const root = xml.child("notes");
if (!strings::to_uint64(root.attribute("count").value(), notesCount))
return false;
uploadedNotesCount = static_cast<uint32_t>(notesCount);
if (!strings::to_uint64(root.attribute("uploadedNotesCount").value(), notesCount))
{
LOG(LERROR, ("Can't read uploadedNotesCount from file."));
uploadedNotesCount = 0;
}
else
{
uploadedNotesCount = static_cast<uint32_t>(notesCount);
}
for (auto const xNode : root.select_nodes("note"))
{
m2::PointD point;
ms::LatLon latLon;
auto const node = xNode.node();
auto const point_x = node.attribute("x");
if (!point_x || !strings::to_double(point_x.value(), point.x))
return false;
auto const lat = node.attribute("lat");
if (!lat || !strings::to_double(lat.value(), latLon.lat))
continue;
auto const point_y = node.attribute("y");
if (!point_y || !strings::to_double(point_y.value(), point.y))
return false;
auto const lon = node.attribute("lon");
if (!lon || !strings::to_double(lon.value(), latLon.lon))
continue;
auto const text = node.attribute("text");
if (!text)
return false;
continue;
notes.emplace_back(point, text.value());
notes.emplace_back(latLon, text.value());
}
return true;
}
void SaveToXml(vector<editor::Note> const & notes,
pugi::xml_document & xml,
uint32_t const UploadedNotesCount)
void SaveToXml(list<editor::Note> const & notes, pugi::xml_document & xml,
uint32_t const uploadedNotesCount)
{
auto constexpr kDigitsAfterComma = 7;
auto root = xml.append_child("notes");
root.append_attribute("count") = UploadedNotesCount;
root.append_attribute("uploadedNotesCount") = uploadedNotesCount;
for (auto const & note : notes)
{
auto node = root.append_child("note");
node.append_attribute("x") = DebugPrint(note.m_point.x).data();
node.append_attribute("y") = DebugPrint(note.m_point.y).data();
node.append_attribute("lat") =
strings::to_string_dac(note.m_point.lat, kDigitsAfterComma).data();
node.append_attribute("lon") =
strings::to_string_dac(note.m_point.lon, kDigitsAfterComma).data();
node.append_attribute("text") = note.m_note.data();
}
}
vector<editor::Note> MoveNoteVectorAtomicly(vector<editor::Note> && notes, mutex & mu)
{
lock_guard<mutex> g(mu);
return move(notes);
}
} // namespace
namespace editor
{
shared_ptr<Notes> Notes::MakeNotes(string const & fileName)
{
return shared_ptr<Notes>(new Notes(fileName));
}
Notes::Notes(string const & fileName)
: m_fileName(fileName)
{
Load();
}
void Notes::CreateNote(m2::PointD const & point, string const & text)
{
lock_guard<mutex> g(m_mu);
m_notes.emplace_back(point, text);
Save();
}
void Notes::Upload(osm::OsmOAuth const & auth)
{
auto const launch = [this, &auth]()
{
// Capture self to keep it from destruction until this thread is done.
auto const self = shared_from_this();
return async(launch::async,
[self, auth]()
{
auto const notes = MoveNoteVectorAtomicly(move(self->m_notes),
self->m_mu);
vector<Note> unuploaded;
osm::ServerApi06 api(auth);
for (auto const & note : notes)
{
try
{
api.CreateNote(MercatorBounds::ToLatLon(note.m_point), note.m_note);
++self->m_uploadedNotes;
}
catch (osm::ServerApi06::ServerApi06Exception const & e)
{
LOG(LERROR, ("Can't upload note.", e.Msg()));
unuploaded.push_back(note);
}
}
lock_guard<mutex> g(self->m_mu);
self->m_notes.insert(end(self->m_notes), begin(unuploaded), end(unuploaded));
self->Save();
});
};
// Do not run more than one upload thread at a time.
static auto future = launch();
auto const status = future.wait_for(milliseconds(0));
if (status == future_status::ready)
future = launch();
}
bool Notes::Load()
/// Not thread-safe, use only for initialization.
bool Load(string const & fileName, list<editor::Note> & notes, uint32_t & uploadedNotesCount)
{
string content;
try
{
auto const reader = GetPlatform().GetReader(m_fileName);
auto const reader = GetPlatform().GetReader(fileName);
reader->ReadAsString(content);
}
catch (FileAbsentException const &)
{
LOG(LINFO, ("No edits file."));
// It's normal if no notes file is present.
return true;
}
catch (Reader::Exception const &)
catch (Reader::Exception const & e)
{
LOG(LERROR, ("Can't process file.", m_fileName));
LOG(LERROR, ("Can't process file.", fileName, e.Msg()));
return false;
}
pugi::xml_document xml;
if (!xml.load_buffer(content.data(), content.size()))
{
LOG(LERROR, ("Can't load notes, xml is illformed."));
LOG(LERROR, ("Can't load notes, XML is ill-formed.", content));
return false;
}
lock_guard<mutex> g(m_mu);
m_notes.clear();
if (!LoadFromXml(xml, m_notes, m_uploadedNotes))
notes.clear();
if (!LoadFromXml(xml, notes, uploadedNotesCount))
{
LOG(LERROR, ("Can't load notes, file is illformed."));
LOG(LERROR, ("Can't load notes, file is ill-formed.", content));
return false;
}
return true;
}
/// Not thread-safe, use syncronization.
bool Notes::Save()
/// Not thread-safe, use synchronization.
bool Save(string const & fileName, list<editor::Note> const & notes,
uint32_t const uploadedNotesCount)
{
pugi::xml_document xml;
SaveToXml(m_notes, xml, m_uploadedNotes);
SaveToXml(notes, xml, uploadedNotesCount);
return my::WriteToTempAndRenameToFile(
fileName, [&xml](string const & fileName) { return xml.save_file(fileName.data(), " "); });
}
} // namespace
string const tmpFileName = m_fileName + ".tmp";
if (!xml.save_file(tmpFileName.data(), " "))
{
LOG(LERROR, ("Can't save map edits into", tmpFileName));
return false;
}
else if (!my::RenameFileX(tmpFileName, m_fileName))
{
LOG(LERROR, ("Can't rename file", tmpFileName, "to", m_fileName));
return false;
}
return true;
namespace editor
{
shared_ptr<Notes> Notes::MakeNotes(string const & fileName, bool const fullPath)
{
return shared_ptr<Notes>(
new Notes(fullPath ? fileName : GetPlatform().WritablePathForFile(fileName)));
}
Notes::Notes(string const & fileName) : m_fileName(fileName)
{
Load(m_fileName, m_notes, m_uploadedNotesCount);
}
void Notes::CreateNote(m2::PointD const & point, string const & text)
{
lock_guard<mutex> g(m_mu);
m_notes.emplace_back(MercatorBounds::ToLatLon(point), text);
Save(m_fileName, m_notes, m_uploadedNotesCount);
}
void Notes::Upload(osm::OsmOAuth const & auth)
{
// Capture self to keep it from destruction until this thread is done.
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();
}
osm::ServerApi06 api(auth);
auto it = begin(self->m_notes);
for (size_t i = 0; i != size; ++i, ++it)
{
try
{
auto const id = api.CreateNote(it->m_point, it->m_note);
LOG(LINFO, ("A note uploaded with id", id));
}
catch (osm::ServerApi06::ServerApi06Exception const & e)
{
LOG(LERROR, ("Can't upload note.", e.Msg()));
// We believe that next iterations will suffer from the same error.
return;
}
lock_guard<mutex> g(self->m_mu);
it = self->m_notes.erase(it);
++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);
}
vector<Note> const Notes::GetNotes() const
{
lock_guard<mutex> g(m_mu);
return {begin(m_notes), end(m_notes)};
}
uint32_t Notes::NotUploadedNotesCount() const
{
lock_guard<mutex> g(m_mu);
return m_notes.size();
}
uint32_t Notes::UploadedNotesCount() const
{
lock_guard<mutex> g(m_mu);
return m_uploadedNotesCount;
}
} // namespace editor

37
editor/editor_notes.hpp
View file

@ -1,25 +1,22 @@
#pragma once
#include "geometry/point2d.hpp"
#include "editor/server_api.hpp"
#include "geometry/latlon.hpp"
#include "base/macros.hpp"
#include "std/list.hpp"
#include "std/mutex.hpp"
#include "std/shared_ptr.hpp"
#include "std/string.hpp"
#include "std/vector.hpp"
namespace editor
{
struct Note
{
Note(m2::PointD const & point, string const & text)
: m_point(point),
m_note(text)
{
}
m2::PointD m_point;
Note(ms::LatLon const & point, string const & text) : m_point(point), m_note(text) {}
ms::LatLon m_point;
string m_note;
};
@ -31,29 +28,31 @@ inline bool operator==(Note const & a, Note const & b)
class Notes : public enable_shared_from_this<Notes>
{
public:
static shared_ptr<Notes> MakeNotes(string const & fileName);
static shared_ptr<Notes> MakeNotes(string const & fileName = "notes.xml",
bool const fullPath = false);
void CreateNote(m2::PointD const & point, string const & text);
/// Uploads notes to the server in a separate thread.
void Upload(osm::OsmOAuth const & auth);
vector<Note> const & GetNotes() const { return m_notes; }
vector<Note> const GetNotes() const;
uint32_t UnuploadedNotesCount() const { return m_notes.size(); }
uint32_t UploadedNotesCount() const { return m_uploadedNotes; }
uint32_t NotUploadedNotesCount() const;
uint32_t UploadedNotesCount() const;
private:
Notes(string const & fileName);
bool Load();
bool Save();
string const m_fileName;
mutable mutex m_mu;
vector<Note> m_notes;
// 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;
uint32_t m_uploadedNotes;
uint32_t m_uploadedNotesCount = 0;
DISALLOW_COPY_AND_MOVE(Notes);
};
} // namespace

40
editor/editor_tests/editor_notes_test.cpp
View file

@ -2,39 +2,41 @@
#include "editor/editor_notes.hpp"
#include "geometry/mercator.hpp"
#include "platform/platform_tests_support/scoped_file.hpp"
#include "coding/file_name_utils.hpp"
#include "base/math.hpp"
using namespace editor;
namespace editor
{
string DebugPrint(Note const & note)
{
return "Note(" + DebugPrint(note.m_point) + ", \"" + note.m_note + "\")";
}
} // namespace editor
UNIT_TEST(Notes)
UNIT_TEST(Notes_Smoke)
{
auto const fileName = "notes.xml";
auto const fullFileName = Platform::PathJoin({GetPlatform().WritableDir(),
fileName});
auto const fullFileName = my::JoinFoldersToPath({GetPlatform().WritableDir()}, fileName);
platform::tests_support::ScopedFile sf(fileName);
{
auto const notes = Notes::MakeNotes(fullFileName);
auto const notes = Notes::MakeNotes(fullFileName, true);
notes->CreateNote({1, 2}, "Some note1");
notes->CreateNote({2, 2}, "Some note2");
notes->CreateNote({1, 1}, "Some note3");
}
{
auto const notes = Notes::MakeNotes(fullFileName);
auto const notes = Notes::MakeNotes(fullFileName, true);
auto const result = notes->GetNotes();
TEST_EQUAL(result.size(), 3, ());
TEST_EQUAL(result,
(vector<Note>{
{{1, 2}, "Some note1"},
{{2, 2}, "Some note2"},
{{1, 1}, "Some note3"}
}), ());
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, ());
}
}
}

31
indexer/osm_editor.cpp
View file

@ -48,8 +48,6 @@ using editor::XMLFeature;
namespace
{
constexpr char const * kEditorXMLFileName = "edits.xml";
// TODO(mgsergio): Hide in notes.
constexpr char const * kNotesXMLFileName = "notes.xml";
constexpr char const * kXmlRootNode = "mapsme";
constexpr char const * kXmlMwmNode = "mwm";
constexpr char const * kDeleteSection = "delete";
@ -68,7 +66,6 @@ bool NeedsUpload(string const & uploadStatus)
}
string GetEditorFilePath() { return GetPlatform().WritablePathForFile(kEditorXMLFileName); }
string GetNotesFilePath() { return GetPlatform().WritablePathForFile(kNotesXMLFileName); }
/// Compares editable fields connected with feature ignoring street.
bool AreFeaturesEqualButStreet(FeatureType const & a, FeatureType const & b)
@ -131,11 +128,7 @@ namespace osm
// TODO(AlexZ): Normalize osm multivalue strings for correct merging
// (e.g. insert/remove spaces after ';' delimeter);
Editor::Editor()
: m_notes(editor::Notes::MakeNotes(GetNotesFilePath()))
{
}
Editor::Editor() : m_notes(editor::Notes::MakeNotes()) {}
Editor & Editor::Instance()
{
static Editor instance;
@ -300,18 +293,12 @@ bool Editor::Save(string const & fullFilePath) const
}
}
string const tmpFileName = fullFilePath + ".tmp";
if (!doc.save_file(tmpFileName.data(), " "))
{
LOG(LERROR, ("Can't save map edits into", tmpFileName));
return false;
}
else if (!my::RenameFileX(tmpFileName, fullFilePath))
{
LOG(LERROR, ("Can't rename file", tmpFileName, "to", fullFilePath));
return false;
}
return true;
return my::WriteToTempAndRenameToFile(
fullFilePath,
[&doc](string const & fileName)
{
return doc.save_file(fileName.data(), " ");
});
}
void Editor::ClearAllLocalEdits()
@ -510,6 +497,8 @@ EditableProperties Editor::GetEditablePropertiesForTypes(feature::TypesHolder co
bool Editor::HaveSomethingToUpload() const
{
if (m_notes->NotUploadedNotesCount() != 0)
return true;
for (auto const & id : m_features)
{
for (auto const & index : id.second)
@ -538,7 +527,7 @@ bool Editor::HaveSomethingToUpload(MwmSet::MwmId const & mwmId) const
void Editor::UploadChanges(string const & key, string const & secret, TChangesetTags tags,
TFinishUploadCallback callBack)
{
if (m_notes->UnuploadedNotesCount())
if (m_notes->NotUploadedNotesCount())
UploadNotes(key, secret);
if (!HaveSomethingToUpload())

10
platform/platform.cpp
View file

@ -94,16 +94,6 @@ bool Platform::RmDirRecursively(string const & dirName)
return res;
}
string Platform::PathJoin(vector<string> const & parts)
{
#ifdef OMIM_OS_WINDOWS
auto const delimiter = "\\";
#else
auto const delimiter = "/";
#endif
return strings::JoinStrings(parts, delimiter);
}
string Platform::ReadPathForFile(string const & file, string searchScope) const
{
if (searchScope.empty())

2
platform/platform.hpp
View file

@ -115,8 +115,6 @@ public:
/// @note If function fails, directory can be partially removed.
static bool RmDirRecursively(string const & dirName);
static string PathJoin(vector<string> const & parts);
/// @return path for directory with temporary files with slash at the end
string TmpDir() const { return m_tmpDir; }
/// @return full path to file in the temporary directory

11
platform/platform_tests/platform_test.cpp
View file

@ -258,14 +258,3 @@ UNIT_TEST(IsSingleMwm)
TEST(!version::IsSingleMwm(version::FOR_TESTING_TWO_COMPONENT_MWM1), ());
TEST(!version::IsSingleMwm(version::FOR_TESTING_TWO_COMPONENT_MWM2), ());
}
UNIT_TEST(PathJoin)
{
#ifdef OMIM_OS_WINDOWS
TEST_EQUAL("foo\\bar", Platform::PathJoin({"foo", "bar"}), ());
TEST_EQUAL("foo\\bar\\baz", Platform::PathJoin({"foo", "bar", "baz"}), ());
#else
TEST_EQUAL("foo/bar", Platform::PathJoin({"foo", "bar"}), ());
TEST_EQUAL("foo/bar/baz", Platform::PathJoin({"foo", "bar", "baz"}), ());
#endif
}

2
std/functional.hpp
View file

@ -10,6 +10,8 @@ using std::less_equal;
using std::greater;
using std::equal_to;
using std::hash;
using std::function;
using std::ref;
#ifdef DEBUG_NEW
#define new DEBUG_NEW