From 7ce5a1282179d15f50396dd639645736a71d7f68 Mon Sep 17 00:00:00 2001 From: vng Date: Thu, 11 Oct 2012 23:08:04 +0300 Subject: [PATCH] Fix bookmarks saving routine. --- map/bookmark.cpp | 90 +++++++++++++++++++++++--------- map/bookmark.hpp | 11 ++++ map/map_tests/bookmarks_test.cpp | 52 ++++++++++-------- 3 files changed, 105 insertions(+), 48 deletions(-) diff --git a/map/bookmark.cpp b/map/bookmark.cpp index dde4e617be..b9bc568baf 100644 --- a/map/bookmark.cpp +++ b/map/bookmark.cpp @@ -15,13 +15,17 @@ #include "../std/algorithm.hpp" -void BookmarkCategory::AddBookmark(Bookmark const & bm, double scale) +void BookmarkCategory::AddBookmarkImpl(Bookmark const & bm, double scale) { Bookmark * p = new Bookmark(bm); p->SetScale(scale); m_bookmarks.push_back(p); +} +void BookmarkCategory::AddBookmark(Bookmark const & bm, double scale) +{ + AddBookmarkImpl(bm, scale); VERIFY ( SaveToKMLFile(), () ); } @@ -85,7 +89,7 @@ int BookmarkCategory::GetBookmark(m2::PointD const org, double const squareDista return -1; } -namespace +namespace bookmark_impl { // Fixes icons which are not supported by MapsWithMe static string GetSupportedBMType(string const & s) @@ -171,7 +175,7 @@ namespace if (tag == "Placemark" && IsValid()) { - m_category.AddBookmark(Bookmark(m_org, m_name, m_type), m_scale); + m_category.AddBookmarkImpl(Bookmark(m_org, m_name, m_type), m_scale); Reset(); } m_tags.pop_back(); @@ -222,7 +226,7 @@ namespace void BookmarkCategory::LoadFromKML(ReaderPtr const & reader) { ReaderSource > src(reader); - KMLParser parser(*this); + bookmark_impl::KMLParser parser(*this); ParseXML(src, parser, true); } @@ -350,18 +354,21 @@ void BookmarkCategory::SaveToKML(ostream & s) s << kmlFooter; } -static bool IsBadCharForPath(strings::UniChar const & c) +namespace { - static strings::UniChar const illegalChars[] = {':', '/', '\\', '<', '>', '\"', '|', '?', '*'}; + bool IsBadCharForPath(strings::UniChar const & c) + { + static strings::UniChar const illegalChars[] = {':', '/', '\\', '<', '>', '\"', '|', '?', '*'}; - for (size_t i = 0; i < ARRAY_SIZE(illegalChars); ++i) - if (c < ' ' || illegalChars[i] == c) - return true; + for (size_t i = 0; i < ARRAY_SIZE(illegalChars); ++i) + if (c < ' ' || illegalChars[i] == c) + return true; - return false; + return false; + } } -string BookmarkCategory::GenerateUniqueFileName(const string & path, string const & name) +string BookmarkCategory::GetValidFileName(string const & name) { // Remove not allowed symbols strings::UniString uniName = strings::MakeUniString(name); @@ -372,35 +379,68 @@ string BookmarkCategory::GenerateUniqueFileName(const string & path, string cons uniName.resize(distance(uniName.begin(), iEnd)); } - string const utfName = uniName.empty() ? "Bookmarks" : strings::ToUtf8(uniName); + return (uniName.empty() ? "Bookmarks" : strings::ToUtf8(uniName)); +} + +string BookmarkCategory::GenerateUniqueFileName(const string & path, string const & name) +{ size_t counter = 1; string suffix; - while (Platform::IsFileExistsByFullPath(path + utfName + suffix + ".kml")) + while (Platform::IsFileExistsByFullPath(path + name + suffix + ".kml")) suffix = strings::to_string(counter++); - return (path + utfName + suffix + ".kml"); + return (path + name + suffix + ".kml"); } bool BookmarkCategory::SaveToKMLFile() { - // always assign new file name according to category name - string fName = GenerateUniqueFileName(GetPlatform().WritableDir(), m_name); - m_file.swap(fName); + string oldFile; + + // Get valid file name from category name + string const name = GetValidFileName(m_name); + + if (!m_file.empty()) + { + size_t i2 = m_file.find_last_of('.'); + if (i2 == string::npos) i2 = m_file.size(); + size_t i1 = m_file.find_last_of("\\/"); + if (i1 == string::npos) i1 = 0; + else ++i1; + + // If m_file doesn't match name, assign new m_file for this category and save old file name. + if (m_file.substr(i1, i2-i1).find(name) != 0) + { + oldFile = GenerateUniqueFileName(GetPlatform().WritableDir(), name); + m_file.swap(oldFile); + } + } + else + m_file = GenerateUniqueFileName(GetPlatform().WritableDir(), name); try { /// @todo On Windows UTF-8 file names are not supported. - ofstream fileToSave(m_file.c_str()); - SaveToKML(fileToSave); + ofstream of(m_file.c_str()); + SaveToKML(of); + of.flush(); - // delete old file - if (!fName.empty() && fName != m_file) - VERIFY ( my::DeleteFileX(fName), (fName, m_file)); + if (!of.fail()) + { + // delete old file + if (!oldFile.empty()) + VERIFY ( my::DeleteFileX(oldFile), (oldFile, m_file)); - return true; + return true; + } } catch (std::exception const & e) { - LOG(LWARNING, ("Can't save bookmarks category", m_name, "to file", m_file)); - return false; } + + LOG(LWARNING, ("Can't save bookmarks category", m_name, "to file", m_file)); + + // return old file name in case of error + if (!oldFile.empty()) + m_file.swap(oldFile); + + return false; } diff --git a/map/bookmark.hpp b/map/bookmark.hpp index fef26b9c26..594ea20570 100644 --- a/map/bookmark.hpp +++ b/map/bookmark.hpp @@ -34,6 +34,11 @@ public: void SetScale(double scale) { m_scale = scale; } }; +namespace bookmark_impl +{ + class KMLParser; +} + class BookmarkCategory : private noncopyable { string m_name; @@ -42,6 +47,9 @@ class BookmarkCategory : private noncopyable /// Stores file name from which category was loaded string m_file; + friend class bookmark_impl::KMLParser; + void AddBookmarkImpl(Bookmark const & bm, double scale); + public: BookmarkCategory(string const & name) : m_name(name), m_visible(true) {} ~BookmarkCategory(); @@ -83,6 +91,9 @@ public: /// @return 0 in the case of error static BookmarkCategory * CreateFromKMLFile(string const & file); + /// Get valid file name from input (remove illegal symbols). + static string GetValidFileName(string const & name); + /// Get unique file name from path and valid file name. static string GenerateUniqueFileName(const string & path, string const & name); //@} }; diff --git a/map/map_tests/bookmarks_test.cpp b/map/map_tests/bookmarks_test.cpp index bd9e77b6b6..1b995f134d 100644 --- a/map/map_tests/bookmarks_test.cpp +++ b/map/map_tests/bookmarks_test.cpp @@ -166,6 +166,19 @@ UNIT_TEST(Bookmarks_ExportKML) TEST(my::DeleteFileX(catFileName), ()); } +namespace +{ + // Call this function to delete test category files. + void DeleteCategoryFiles() + { + string const path = GetPlatform().WritableDir(); + char const * arrFiles[] = { "cat1", "cat2", "cat3" }; + + for (size_t i = 0; i < ARRAY_SIZE(arrFiles); ++i) + FileWriter::DeleteFileX(path + arrFiles[i] + ".kml"); + } +} + UNIT_TEST(Bookmarks_Getting) { Framework fm; @@ -216,6 +229,8 @@ UNIT_TEST(Bookmarks_Getting) cat->DeleteBookmark(0); TEST_EQUAL(cat->GetBookmarksCount(), 0, ()); + + DeleteCategoryFiles(); } UNIT_TEST(Bookmarks_AddressInfo) @@ -242,11 +257,7 @@ UNIT_TEST(Bookmarks_IllegalFileName) for (size_t i = 0; i < ARRAY_SIZE(arrIllegal); ++i) { - string name = BookmarkCategory::GenerateUniqueFileName("", arrIllegal[i]); - - // remove extension - TEST_GREATER(name.size(), 4, ()); - name.erase(name.end() - 4, name.end()); + string const name = BookmarkCategory::GetValidFileName(arrIllegal[i]); if (strlen(arrLegal[i]) == 0) { @@ -261,30 +272,33 @@ UNIT_TEST(Bookmarks_IllegalFileName) UNIT_TEST(Bookmarks_UniqueFileName) { - string const FILENAME = "SomeUniqueFileName"; + string const BASE = "SomeUniqueFileName"; + string const FILENAME = BASE + ".kml"; + { FileWriter file(FILENAME); file.Write(FILENAME.data(), FILENAME.size()); } - string gen = BookmarkCategory::GenerateUniqueFileName("", FILENAME); - TEST_NOT_EQUAL(gen, FILENAME, ()); - TEST_EQUAL(gen, FILENAME + "1.kml", ()); - string const FILENAME1 = FILENAME + "1"; + string gen = BookmarkCategory::GenerateUniqueFileName("", BASE); + TEST_NOT_EQUAL(gen, FILENAME, ()); + TEST_EQUAL(gen, BASE + "1.kml", ()); + + string const FILENAME1 = gen; { FileWriter file(FILENAME1); file.Write(FILENAME1.data(), FILENAME1.size()); } - gen = BookmarkCategory::GenerateUniqueFileName("", FILENAME); + gen = BookmarkCategory::GenerateUniqueFileName("", BASE); TEST_NOT_EQUAL(gen, FILENAME, ()); TEST_NOT_EQUAL(gen, FILENAME1, ()); - TEST_EQUAL(gen, FILENAME + "2.kml", ()); + TEST_EQUAL(gen, BASE + "2.kml", ()); FileWriter::DeleteFileX(FILENAME); FileWriter::DeleteFileX(FILENAME1); - gen = BookmarkCategory::GenerateUniqueFileName("", FILENAME); - TEST_EQUAL(gen, FILENAME + ".kml", ()); + gen = BookmarkCategory::GenerateUniqueFileName("", BASE); + TEST_EQUAL(gen, FILENAME, ()); } UNIT_TEST(Bookmarks_AddingMoving) @@ -329,14 +343,6 @@ UNIT_TEST(Bookmarks_AddingMoving) pBm = fm.GetBmCategory(res.first)->GetBookmark(res.second); TEST_EQUAL(pBm->GetName(), "name3", ()); TEST_EQUAL(pBm->GetType(), "placemark-green", ()); -} -// This test should always be the last to delete temporary files from previous tests. -UNIT_TEST(Bookmarks_Finalizing) -{ - string const path = GetPlatform().WritableDir(); - char const * arrFiles[] = { "cat1", "cat2", "cat3" }; - - for (size_t i = 0; i < ARRAY_SIZE(arrFiles); ++i) - FileWriter::DeleteFileX(path + arrFiles[i] + ".kml"); + DeleteCategoryFiles(); }