Set bookmarks file name equal with category name. Fix some unit tests for this case.

map/bookmark.cpp

@@ -4,6 +4,7 @@
 
 #include "../coding/file_reader.hpp"
 #include "../coding/parse_xml.hpp"  // LoadFromKML
+#include "../coding/internal/file_data.hpp"
 
 #include "../platform/platform.hpp"
 
@@ -349,48 +350,57 @@ void BookmarkCategory::SaveToKML(ostream & s)
   s << kmlFooter;
 }
 
-static bool IsValidCharForPath(strings::UniChar c)
+static 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 false;
-  return true;
+      return true;
+
+  return false;
 }
 
 string BookmarkCategory::GenerateUniqueFileName(const string & path, string const & name)
 {
   // Remove not allowed symbols
   strings::UniString uniName = strings::MakeUniString(name);
-  size_t const charCount = uniName.size();
-  strings::UniString uniFixedName;
-  for (size_t i = 0; i < charCount; ++i)
-    if (IsValidCharForPath(uniName[i]))
-      uniFixedName.push_back(uniName[i]);
+  strings::UniString::iterator iEnd = remove_if(uniName.begin(), uniName.end(), &IsBadCharForPath);
+  if (iEnd != uniName.end())
+  {
+    // buffer_vector doesn't have erase function - call resize here (it's even better in this case).
+    uniName.resize(distance(uniName.begin(), iEnd));
+  }
 
-  string const fixedName = uniFixedName.empty() ? "Bookmarks" : strings::ToUtf8(uniFixedName);
+  string const utfName = uniName.empty() ? "Bookmarks" : strings::ToUtf8(uniName);
   size_t counter = 1;
   string suffix;
-  while (Platform::IsFileExistsByFullPath(path + fixedName + suffix))
+  while (Platform::IsFileExistsByFullPath(path + utfName + suffix))
     suffix = strings::to_string(counter++);
-  return path + fixedName + suffix + ".kml";
+  return (path + utfName + suffix + ".kml");
 }
 
 bool BookmarkCategory::SaveToKMLFile()
 {
-  if (m_file.empty())
-    m_file = GenerateUniqueFileName(GetPlatform().WritableDir(), m_name);
+  // always assign new file name according to category name
+  string fName = GenerateUniqueFileName(GetPlatform().WritableDir(), m_name);
+  m_file.swap(fName);
 
   try
   {
-    // @TODO On Windows UTF-8 file names are not supported.
+    /// @todo On Windows UTF-8 file names are not supported.
     ofstream fileToSave(m_file.c_str());
     SaveToKML(fileToSave);
+
+    // delete old file
+    if (!fName.empty() && fName != m_file)
+      VERIFY ( my::DeleteFileX(fName), (fName, m_file));
+
+    return true;
   }
   catch (std::exception const & e)
   {
     LOG(LWARNING, ("Can't save bookmarks category", m_name, "to file", m_file));
     return false;
   }
-  return true;
 }

map/map_tests/bookmarks_test.cpp

@@ -2,6 +2,10 @@
 
 #include "../../map/framework.hpp"
 
+#include "../../platform/platform.hpp"
+
+#include "../../coding/internal/file_data.hpp"
+
 #include "../../std/fstream.hpp"
 
 
@@ -125,9 +129,9 @@ UNIT_TEST(Bookmarks_ImportKML)
 UNIT_TEST(Bookmarks_ExportKML)
 {
   char const * BOOKMARKS_FILE_NAME = "UnitTestBookmarks.kml";
+
   BookmarkCategory cat("Default");
   cat.LoadFromKML(new MemReader(kmlString, strlen(kmlString)));
-
   CheckBookmarks(cat);
 
   TEST_EQUAL(cat.IsVisible(), false, ());
@@ -141,24 +145,25 @@ UNIT_TEST(Bookmarks_ExportKML)
   }
 
   cat.ClearBookmarks();
-
   TEST_EQUAL(cat.GetBookmarksCount(), 0, ());
 
   cat.LoadFromKML(new FileReader(BOOKMARKS_FILE_NAME));
-
   CheckBookmarks(cat);
   TEST_EQUAL(cat.IsVisible(), true, ());
 
-  BookmarkCategory * cat2 = BookmarkCategory::CreateFromKMLFile(BOOKMARKS_FILE_NAME);
+  scoped_ptr<BookmarkCategory> cat2(BookmarkCategory::CreateFromKMLFile(BOOKMARKS_FILE_NAME));
   CheckBookmarks(*cat2);
-  FileWriter::DeleteFileX(BOOKMARKS_FILE_NAME);
 
   cat2->SaveToKMLFile();
-  delete cat2;
+  // old file should be deleted if we save bookmarks with new category name
+  uint64_t dummy;
+  TEST(!my::GetFileSize(BOOKMARKS_FILE_NAME, dummy), ());
 
-  cat2 = BookmarkCategory::CreateFromKMLFile(BOOKMARKS_FILE_NAME);
+  // MapName is the <name> tag in test kml data.
+  string const catFileName = GetPlatform().WritableDir() + "MapName.kml";
+  cat2.reset(BookmarkCategory::CreateFromKMLFile(catFileName));
   CheckBookmarks(*cat2);
-  FileWriter::DeleteFileX(BOOKMARKS_FILE_NAME);
+  TEST(my::DeleteFileX(catFileName), ());
 }
 
 UNIT_TEST(Bookmarks_Getting)
@@ -230,6 +235,30 @@ UNIT_TEST(Bookmarks_AddressInfo)
   TEST(info.m_types[0] == "cafe" || info.m_types[0] == "кафе", (info.m_types[0]));
 }
 
+UNIT_TEST(Bookmarks_IllegalFileName)
+{
+  char const * arrIllegal[] = { "?", "?|", "\"x", "|x:", "x<>y", "xy*"};
+  char const * arrLegal[] =   { "",  "",   "x",   "x",   "xy",   "xy"};
+
+  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());
+
+    if (strlen(arrLegal[i]) == 0)
+    {
+      TEST_EQUAL("Bookmarks", name, ());
+    }
+    else
+    {
+      TEST_EQUAL(arrLegal[i], name, ());
+    }
+  }
+}
+
 UNIT_TEST(Bookmarks_UniqueFileName)
 {
   string const FILENAME = "SomeUniqueFileName";
@@ -300,5 +329,14 @@ 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");
 }