[platform, storage] Implemented cleanup of absent countries indexes.

map/framework.cpp

@@ -477,7 +477,11 @@ void Framework::RegisterAllMaps()
          ("Registering maps while map downloading leads to removing downloading maps from "
           "ActiveMapsListener::m_items."));
 
-  platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion());
+  auto const isCountryName = [this](string const & filename)
+  {
+    return m_storage.FindIndexByFile(filename).IsValid();
+  };
+  platform::CleanupMapsDirectory(m_storage.GetCurrentDataVersion(), isCountryName);
   m_storage.RegisterAllLocalMaps();
 
   int minFormat = numeric_limits<int>::max();

platform/local_country_file_utils.cpp

@@ -1,6 +1,8 @@
-#include "local_country_file_utils.hpp"
-#include "mwm_version.hpp"
-#include "platform.hpp"
+#include "platform/local_country_file_utils.hpp"
+
+#include "platform/country_file.hpp"
+#include "platform/mwm_version.hpp"
+#include "platform/platform.hpp"
 
 #include "coding/file_name_utils.hpp"
 #include "coding/internal/file_data.hpp"
@@ -15,6 +17,7 @@
 #include "std/cctype.hpp"
 #include "std/sstream.hpp"
 #include "std/unique_ptr.hpp"
+#include "std/unordered_set.hpp"
 
 #include "defines.hpp"
 
@@ -82,6 +85,31 @@ void DeleteDownloaderFilesForAllCountries(string const & directory)
   for (auto const & file : files)
     my::DeleteFileX(my::JoinFoldersToPath(directory, file));
 }
+
+void DeleteIndexesForAbsentCountries(string const & directory,
+                                     int64_t version,
+                                     function<bool(string const & filename)> const & isCountryName)
+{
+  vector<LocalCountryFile> files;
+  FindAllLocalMapsInDirectory(directory, version, files);
+
+  unordered_set<string> names;
+  for (auto const & file : files)
+    names.insert(file.GetCountryName());
+
+  Platform::FilesList subdirs;
+  Platform::GetFilesByType(directory, Platform::FILE_TYPE_DIRECTORY, subdirs);
+  for (auto const & subdir : subdirs)
+  {
+    if (subdir == "." || subdir == "..")
+      continue;
+    if (!isCountryName(subdir) || names.count(subdir) != 0)
+      continue;
+
+    LocalCountryFile absentCountry(directory, CountryFile(subdir), version);
+    CountryIndexes::DeleteFromDisk(absentCountry);
+  }
+}
 }  // namespace
 
 void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t version)
@@ -96,7 +124,8 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve
   }
 }
 
-void CleanupMapsDirectory(int64_t latestVersion)
+void CleanupMapsDirectory(int64_t latestVersion,
+                          function<bool(string const & filename)> const & isCountryName)
 {
   Platform & platform = GetPlatform();
 
@@ -137,6 +166,9 @@ void CleanupMapsDirectory(int64_t latestVersion)
     if (version != latestVersion)
       DeleteDownloaderFilesForAllCountries(subdirPath);
 
+    // It's OK to remove indexes for absent countries.
+    DeleteIndexesForAbsentCountries(subdirPath, version, isCountryName);
+
     // Remove subdirectory if it does not contain any files except "." and "..".
     if (subdir != "." && Platform::IsDirectoryEmpty(subdirPath))
     {
@@ -146,9 +178,6 @@ void CleanupMapsDirectory(int64_t latestVersion)
       UNUSED_VALUE(ret);
     }
   }
-
-  /// @todo Cleanup temporary index files for already absent mwm files.
-  /// https://trello.com/c/PKiiOsB4/28--
 }
 
 void FindAllLocalMapsInDirectory(string const & directory, int64_t version,

platform/local_country_file_utils.hpp

@@ -3,6 +3,7 @@
 #include "platform/country_defines.hpp"
 #include "platform/local_country_file.hpp"
 
+#include "std/function.hpp"
 #include "std/shared_ptr.hpp"
 #include "std/utility.hpp"
 #include "std/vector.hpp"
@@ -17,8 +18,11 @@ void DeleteDownloaderFilesForCountry(CountryFile const & countryFile, int64_t ve
 // Removes partially downloaded maps, empty directories and old
 // (format v1) maps.  Also, removes old (splitted) Japan and Brazil
 // maps. |version| must be set to the latest data version this app can
-// work with.
-void CleanupMapsDirectory(int64_t latestVersion);
+// work with. |isCountryName| predicate is used to detect and remove
+// indexes for absent countries. It must return true for file names
+// corresponding to a country.
+void CleanupMapsDirectory(int64_t latestVersion,
+                          function<bool(string const & filename)> const & isCountryName);
 
 // Finds all local map files in a directory. Version of these files is
 // passed as an argument.

platform/platform_tests/local_country_file_tests.cpp

@@ -137,6 +137,13 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles)
   Platform & platform = GetPlatform();
   string const mapsDir = platform.WritableDir();
 
+  // Two fake directories for test country files and indexes.
+  ScopedDir dir3("3");
+  ScopedDir dir4("4");
+
+  ScopedDir absentCountryIndexesDir(dir4, "Absent");
+  ScopedDir irelandIndexesDir(dir4, "Ireland");
+
   CountryFile japanFile("Japan");
   CountryFile brazilFile("Brazil");
   CountryFile irelandFile("Ireland");
@@ -147,10 +154,8 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles)
   LocalCountryFile brazilLocalFile(mapsDir, brazilFile, 0 /* version */);
   ScopedFile brazilMapFile("Brazil.mwm", "Brazil");
 
-  LocalCountryFile irelandLocalFile(mapsDir, irelandFile, 0 /* version */);
-  ScopedFile irelandMapFile("Ireland.mwm", "Ireland");
-
-  ScopedDir emptyDir("3");
+  LocalCountryFile irelandLocalFile(dir4.GetFullPath(), irelandFile, 4 /* version */);
+  ScopedFile irelandMapFile(dir4, irelandFile, MapOptions::Map, "Ireland");
 
   // Check that FindAllLocalMaps()
   vector<LocalCountryFile> localFiles;
@@ -159,7 +164,10 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles)
   TEST(Contains(localFiles, brazilLocalFile), (brazilLocalFile, localFiles));
   TEST(Contains(localFiles, irelandLocalFile), (irelandLocalFile, localFiles));
 
-  CleanupMapsDirectory(0 /* latestVersion */);
+  CleanupMapsDirectory(0 /* latestVersion */, [](string const filename)
+                       {
+                         return filename == "Ireland" || filename == "Absent";
+                       });
 
   japanLocalFile.SyncWithDisk();
   TEST_EQUAL(MapOptions::Nothing, japanLocalFile.GetFiles(), ());
@@ -177,8 +185,15 @@ UNIT_TEST(LocalCountryFile_CleanupMapFiles)
   TEST(!irelandMapFile.Exists(), (irelandMapFile));
   irelandMapFile.Reset();
 
-  TEST(!emptyDir.Exists(), ("Empty directory", emptyDir, "wasn't removed."));
-  emptyDir.Reset();
+  TEST(!dir3.Exists(), ("Empty directory", dir3, "wasn't removed."));
+  dir3.Reset();
+
+  TEST(dir4.Exists(), ());
+
+  TEST(!absentCountryIndexesDir.Exists(), ("Indexes for absent country weren't deleted."));
+  absentCountryIndexesDir.Reset();
+
+  TEST(irelandIndexesDir.Exists(), ());
 }
 
 UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles)
@@ -200,7 +215,11 @@ UNIT_TEST(LocalCountryFile_CleanupPartiallyDownloadedFiles)
       {my::JoinFoldersToPath(latestDir.GetRelativePath(), "Russia_Southern.mwm.downloading"),
        "Southern Russia map"}};
 
-  CleanupMapsDirectory(101010 /* latestVersion */);
+  auto const isCountryName = [&](string const & /* filename */)
+  {
+    return false;
+  };
+  CleanupMapsDirectory(101010 /* latestVersion */, isCountryName);
 
   for (ScopedFile & file : toBeDeleted)
   {

platform/platform_tests_support/scoped_dir.cpp

@@ -34,6 +34,11 @@ ScopedDir::ScopedDir(string const & relativePath)
   }
 }
 
+ScopedDir::ScopedDir(ScopedDir const & parent, string const & name)
+  : ScopedDir(my::JoinFoldersToPath(parent.GetRelativePath(), name))
+{
+}
+
 ScopedDir::~ScopedDir()
 {
   if (m_reset)

platform/platform_tests_support/scoped_dir.hpp

@@ -18,6 +18,8 @@ public:
   /// @param path Path for a testing directory, should be relative to writable-dir.
   ScopedDir(string const & relativePath);
 
+  ScopedDir(ScopedDir const & parent, string const & name);
+
   ~ScopedDir();
 
   inline void Reset() { m_reset = true; }

storage/storage.cpp

@@ -66,6 +66,12 @@ void DeleteCountryIndexes(LocalCountryFile const & localFile)
   platform::CountryIndexes::DeleteFromDisk(localFile);
 }
 
+void DeleteFromDiskWithIndexes(LocalCountryFile const & localFile, MapOptions options)
+{
+  DeleteCountryIndexes(localFile);
+  localFile.DeleteFromDisk(options);
+}
+
 class EqualFileName
 {
   string const & m_name;
@@ -130,7 +136,7 @@ void Storage::RegisterAllLocalMaps()
       LocalCountryFile & localFile = *j;
       LOG(LINFO, ("Removing obsolete", localFile));
       localFile.SyncWithDisk();
-      localFile.DeleteFromDisk(MapOptions::MapWithCarRouting);
+      DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting);
       ++j;
     }
 
@@ -340,8 +346,7 @@ void Storage::DeleteCountry(TIndex const & index, MapOptions opt)
 void Storage::DeleteCustomCountryVersion(LocalCountryFile const & localFile)
 {
   CountryFile const countryFile = localFile.GetCountryFile();
-  localFile.DeleteFromDisk(MapOptions::MapWithCarRouting);
-  CountryIndexes::DeleteFromDisk(localFile);
+  DeleteFromDiskWithIndexes(localFile, MapOptions::MapWithCarRouting);
 
   {
     auto it = m_localFilesForFakeCountries.find(countryFile);
@@ -844,9 +849,8 @@ void Storage::DeleteCountryFiles(TIndex const & index, MapOptions opt)
   auto & localFiles = it->second;
   for (auto & localFile : localFiles)
   {
-    localFile->DeleteFromDisk(opt);
+    DeleteFromDiskWithIndexes(*localFile, opt);
     localFile->SyncWithDisk();
-    DeleteCountryIndexes(*localFile);
     if (localFile->GetFiles() == MapOptions::Nothing)
       localFile.reset();
   }

storage/storage_tests/storage_tests.cpp

@@ -12,6 +12,7 @@
 #include "platform/local_country_file.hpp"
 #include "platform/local_country_file_utils.hpp"
 #include "platform/platform.hpp"
+#include "platform/platform_tests_support/scoped_dir.hpp"
 #include "platform/platform_tests_support/scoped_file.hpp"
 
 #include "coding/file_name_utils.hpp"
@@ -732,4 +733,30 @@ UNIT_TEST(StorageTest_EmptyRoutingFile)
   checker->StartDownload();
   runner.Run();
 }
+
+UNIT_TEST(StorageTest_ObsoleteMapsRemoval)
+{
+  CountryFile country("Azerbaijan");
+
+  tests_support::ScopedDir dir1("1");
+  tests_support::ScopedFile map1(dir1, country, MapOptions::Map, "map1");
+  LocalCountryFile file1(dir1.GetFullPath(), country, 1 /* version */);
+  CountryIndexes::PreparePlaceOnDisk(file1);
+
+  tests_support::ScopedDir dir2("2");
+  tests_support::ScopedFile map2(dir2, country, MapOptions::Map, "map2");
+  LocalCountryFile file2(dir2.GetFullPath(), country, 2 /* version */);
+  CountryIndexes::PreparePlaceOnDisk(file2);
+
+  TEST(map1.Exists(), ());
+  TEST(map2.Exists(), ());
+
+  Storage storage;
+  storage.RegisterAllLocalMaps();
+
+  TEST(!map1.Exists(), ());
+  map1.Reset();
+
+  TEST(map2.Exists(), ());
+}
 }  // namespace storage