diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index 4d2e90d7d9..35b6d2b9e8 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -106,8 +106,9 @@ pair MwmSet::Register(LocalCountryFile con // Update the status of the mwm with the same version. if (info->GetVersion() == localFile.GetVersion()) { - LOG(LWARNING, ("Trying to add already registered mwm:", name)); + LOG(LINFO, ("Updating already registered mwm:", name)); info->SetStatus(MwmInfo::STATUS_REGISTERED); + info->m_file = localFile; return make_pair(GetLock(id), RegResult::VersionAlreadyExists); } diff --git a/map/framework.cpp b/map/framework.cpp index 5e499a0925..d7ecd36b81 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -252,7 +252,7 @@ Framework::Framework() LOG(LDEBUG, ("Maps initialized")); // Init storage with needed callback. - m_storage.Init(bind(&Framework::UpdateAfterDownload, this, _1)); + m_storage.Init(bind(&Framework::UpdateLatestCountryFile, this, _1)); LOG(LDEBUG, ("Storage initialized")); #ifdef USE_PEDESTRIAN_ROUTER @@ -421,7 +421,7 @@ void Framework::ShowCountry(TIndex const & index) ShowRectEx(GetCountryBounds(index)); } -void Framework::UpdateAfterDownload(LocalCountryFile const & localFile) +void Framework::UpdateLatestCountryFile(LocalCountryFile const & localFile) { // TODO (@ldragunov, @gorshenin): rewrite routing session to use MwmHandles. Thus, // it won' be needed to reset it after maps update. diff --git a/map/framework.hpp b/map/framework.hpp index 7a8055d3bb..b71a26bb41 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -136,8 +136,9 @@ protected: /// How many pixels around touch point are used to get bookmark or POI static const int TOUCH_PIXEL_RADIUS = 20; - /// This function is called by m_storage to notify that country downloading is finished. - void UpdateAfterDownload(platform::LocalCountryFile const & localFile); + /// This function is called by m_storage when latest local files + /// were changed. + void UpdateLatestCountryFile(platform::LocalCountryFile const & localFile); /// This function is called by m_model when the map file is deregistered. void OnMapDeregistered(platform::LocalCountryFile const & localFile); diff --git a/omim.pro b/omim.pro index 652ff37877..8c2d68e98e 100644 --- a/omim.pro +++ b/omim.pro @@ -42,6 +42,7 @@ SUBDIRS = 3party \ generator/generator_tool \ indexer/indexer_tests \ graphics/graphics_tests \ + routing/routing_tests \ gui/gui_tests \ qt \ drape_head \ diff --git a/storage/storage.cpp b/storage/storage.cpp index cd317cbe50..3ba47a0915 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -44,7 +44,7 @@ void RemoveIf(vector & v, function const & p) uint64_t GetLocalSize(shared_ptr file, TMapOptions opt) { - if (file.get() == nullptr) + if (!file) return 0; uint64_t size = 0; for (TMapOptions bit : {TMapOptions::EMap, TMapOptions::ECarRouting}) @@ -66,6 +66,15 @@ uint64_t GetRemoteSize(CountryFile const & file, TMapOptions opt) return size; } +void DeleteCountryIndexes(LocalCountryFile const & localFile) +{ + // TODO (@gorshenin, @ldragunov): delete localFile instead of rootFile when routing will be + // switched to new country indexes API. + LocalCountryFile rootFile(GetPlatform().WritableDir(), localFile.GetCountryFile(), + localFile.GetVersion()); + platform::CountryIndexes::DeleteFromDisk(rootFile); +} + class EqualFileName { string const & m_name; @@ -88,7 +97,7 @@ Storage::Storage() : m_downloader(new HttpMapFilesDownloader()), m_currentSlotId LoadCountriesFile(false /* forceReload */); } -void Storage::Init(TUpdateAfterDownload const & updateFn) { m_updateAfterDownload = updateFn; } +void Storage::Init(TUpdate const & update) { m_update = update; } void Storage::Clear() { @@ -232,7 +241,7 @@ shared_ptr Storage::GetLatestLocalFile(CountryFile const & cou TIndex const index = FindIndexByFile(countryFile.GetNameWithoutExt()); { shared_ptr localFile = GetLatestLocalFile(index); - if (localFile.get()) + if (localFile) return localFile; } { @@ -290,7 +299,7 @@ void Storage::CountryStatusEx(TIndex const & index, TStatus & status, TMapOption options = TMapOptions::EMap; shared_ptr localFile = GetLatestLocalFile(index); - ASSERT(localFile.get(), ("Invariant violation: local file out of sync with disk.")); + ASSERT(localFile, ("Invariant violation: local file out of sync with disk.")); if (localFile->OnDisk(TMapOptions::ECarRouting)) options = SetOptions(options, TMapOptions::ECarRouting); } @@ -321,6 +330,11 @@ void Storage::DeleteCountry(TIndex const & index, TMapOptions opt) opt = NormalizeDeleteFileSet(opt); DeleteCountryFiles(index, opt); DeleteCountryFilesFromDownloader(index, opt); + + auto localFile = GetLatestLocalFile(index); + if (localFile) + m_update(*localFile); + NotifyStatusChanged(index); } @@ -451,12 +465,6 @@ void Storage::OnMapFileDownloadFinished(bool success, QueuedCountry & queuedCountry = m_queue.front(); TIndex const index = queuedCountry.GetIndex(); - Platform & platform = GetPlatform(); - string const path = GetFileDownloadPath(index, queuedCountry.GetCurrentFile()); - - success = success && platform.IsFileExistsByFullPath(path) && - RegisterDownloadedFile(path, progress.first /* size */, GetCurrentDataVersion()); - if (success && queuedCountry.SwitchToNextFile()) { DownloadNextFile(queuedCountry); @@ -516,35 +524,48 @@ void Storage::OnMapFileDownloadProgress(MapFilesDownloader::TProgress const & pr } } -bool Storage::RegisterDownloadedFile(string const & path, uint64_t size, int64_t version) +bool Storage::RegisterDownloadedFiles(TIndex const & index, TMapOptions files) { - QueuedCountry & queuedCountry = m_queue.front(); - TIndex const & index = queuedCountry.GetIndex(); - - ASSERT_EQUAL(size, GetDownloadSize(queuedCountry), ("Downloaded file size mismatch with expected")); - CountryFile const countryFile = GetCountryFile(index); - shared_ptr localFile = GetLocalFile(index, version); - if (localFile.get() == nullptr) - localFile = PreparePlaceForCountryFiles(countryFile, version); - if (localFile.get() == nullptr) + shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion()); + if (!localFile) + localFile = PreparePlaceForCountryFiles(countryFile, GetCurrentDataVersion()); + if (!localFile) { - LOG(LERROR, ("Local file data structure can't prepared for downloaded file(", path, ").")); + LOG(LERROR, ("Local file data structure can't be prepared for downloaded file(", countryFile, + files, ").")); return false; } - if (!my::RenameFileX(path, localFile->GetPath(queuedCountry.GetCurrentFile()))) + + bool ok = true; + for (TMapOptions file : {TMapOptions::EMap, TMapOptions::ECarRouting}) + { + if (!HasOptions(files, file)) + continue; + string const path = GetFileDownloadPath(index, file); + if (!my::RenameFileX(path, localFile->GetPath(file))) + { + ok = false; + break; + } + } + localFile->SyncWithDisk(); + if (!ok) + { + localFile->DeleteFromDisk(files); return false; + } RegisterCountryFiles(localFile); return true; } -void Storage::OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions opt) +void Storage::OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions files) { - ASSERT_NOT_EQUAL(TMapOptions::ENothing, opt, + ASSERT_NOT_EQUAL(TMapOptions::ENothing, files, ("This method should not be called for empty files set.")); { string optionsName; - switch (opt) + switch (files) { case TMapOptions::ENothing: optionsName = "Nothing"; @@ -567,17 +588,18 @@ void Storage::OnMapDownloadFinished(TIndex const & index, bool success, TMapOpti {"option", optionsName}})); } - if (success) - { - shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion()); - ASSERT(localFile.get(), ()); - platform::CountryIndexes::DeleteFromDisk(*localFile); - m_updateAfterDownload(*localFile); - } - else + success = success && RegisterDownloadedFiles(index, files); + + if (!success) { m_failedCountries.insert(index); + return; } + + shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion()); + ASSERT(localFile, ()); + DeleteCountryIndexes(*localFile); + m_update(*localFile); } string Storage::GetFileDownloadUrl(string const & baseUrl, TIndex const & index, @@ -589,7 +611,8 @@ string Storage::GetFileDownloadUrl(string const & baseUrl, TIndex const & index, string Storage::GetFileDownloadUrl(string const & baseUrl, string const & fName) const { - return baseUrl + OMIM_OS_NAME "/" + strings::to_string(GetCurrentDataVersion()) + "/" + UrlEncode(fName); + return baseUrl + OMIM_OS_NAME "/" + strings::to_string(GetCurrentDataVersion()) + "/" + + UrlEncode(fName); } TIndex Storage::FindIndexByFile(string const & name) const @@ -650,7 +673,7 @@ void Storage::GetOutdatedCountries(vector & countries) const TIndex const & index = p.first; string const name = GetCountryFile(index).GetNameWithoutExt(); shared_ptr const & file = GetLatestLocalFile(index); - if (file.get() && file->GetVersion() != GetCurrentDataVersion() && + if (file && file->GetVersion() != GetCurrentDataVersion() && name != WORLD_COASTS_FILE_NAME && name != WORLD_FILE_NAME) { countries.push_back(&CountryByIndex(index)); @@ -672,7 +695,7 @@ TStatus Storage::CountryStatusFull(TIndex const & index, TStatus const status) c return status; shared_ptr localFile = GetLatestLocalFile(index); - if (localFile.get() == nullptr || !localFile->OnDisk(TMapOptions::EMap)) + if (!localFile || !localFile->OnDisk(TMapOptions::EMap)) return TStatus::ENotDownloaded; CountryFile const & countryFile = GetCountryFile(index); @@ -694,7 +717,7 @@ TMapOptions Storage::NormalizeDownloadFileSet(TIndex const & index, TMapOptions for (TMapOptions file : {TMapOptions::EMap, TMapOptions::ECarRouting}) { // Check whether requested files are on disk and up-to-date. - if (HasOptions(opt, file) && localCountryFile.get() && localCountryFile->OnDisk(file) && + if (HasOptions(opt, file) && localCountryFile && localCountryFile->OnDisk(file) && localCountryFile->GetVersion() == GetCurrentDataVersion()) { opt = UnsetOptions(opt, file); @@ -754,12 +777,12 @@ shared_ptr Storage::GetLocalFile(TIndex const & index, int64_t void Storage::RegisterCountryFiles(shared_ptr localFile) { - CHECK(localFile.get(), ()); + CHECK(localFile, ()); localFile->SyncWithDisk(); TIndex const index = FindIndexByFile(localFile->GetCountryName()); shared_ptr existingFile = GetLocalFile(index, localFile->GetVersion()); - if (existingFile.get() != nullptr) + if (existingFile) ASSERT_EQUAL(localFile.get(), existingFile.get(), ()); else m_localFiles[index].push_front(localFile); @@ -794,13 +817,13 @@ void Storage::DeleteCountryFiles(TIndex const & index, TMapOptions opt) { localFile->DeleteFromDisk(opt); localFile->SyncWithDisk(); - platform::CountryIndexes::DeleteFromDisk(*localFile); + DeleteCountryIndexes(*localFile); if (localFile->GetFiles() == TMapOptions::ENothing) localFile.reset(); } auto isNull = [](shared_ptr const & localFile) { - return !localFile.get(); + return !localFile; }; localFiles.remove_if(isNull); if (localFiles.empty()) @@ -822,21 +845,6 @@ bool Storage::DeleteCountryFilesFromDownloader(TIndex const & index, TMapOptions m_downloader->Reset(); } - // Remove already downloaded files. - if (shared_ptr localFile = GetLocalFile(index, GetCurrentDataVersion())) - { - localFile->DeleteFromDisk(opt); - localFile->SyncWithDisk(); - if (localFile->GetFiles() == TMapOptions::ENothing) - { - auto equalsToLocalFile = [&localFile](shared_ptr const & rhs) - { - return *localFile == *rhs; - }; - m_localFiles[index].remove_if(equalsToLocalFile); - } - } - queuedCountry->RemoveOptions(opt); // Remove country from the queue if there's nothing to download. diff --git a/storage/storage.hpp b/storage/storage.hpp index a8f8161852..652611741b 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -20,6 +20,10 @@ namespace storage /// Can be used to store local maps and/or maps available for download class Storage { +public: + using TUpdate = function; + +private: /// We support only one simultaneous request at the moment unique_ptr m_downloader; @@ -67,11 +71,9 @@ class Storage ObserversContT m_observers; //@} - typedef function TUpdateAfterDownload; - // This function is called each time all files requested for a // country were successfully downloaded. - TUpdateAfterDownload m_updateAfterDownload; + TUpdate m_update; void DownloadNextCountryFromQueue(); @@ -91,8 +93,8 @@ class Storage /// during the downloading process. void OnMapFileDownloadProgress(MapFilesDownloader::TProgress const & progress); - bool RegisterDownloadedFile(string const & path, uint64_t size, int64_t version); - void OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions opt); + bool RegisterDownloadedFiles(TIndex const & index, TMapOptions files); + void OnMapDownloadFinished(TIndex const & index, bool success, TMapOptions files); /// Initiates downloading of the next file from the queue. void DownloadNextFile(QueuedCountry const & country); @@ -100,7 +102,7 @@ class Storage public: Storage(); - void Init(TUpdateAfterDownload const & updateFn); + void Init(TUpdate const & update); // Clears local files registry and downloader's queue. void Clear(); diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index a5ec6086af..a901d467f5 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -5,6 +5,8 @@ #include "storage/storage_tests/fake_map_files_downloader.hpp" #include "storage/storage_tests/task_runner.hpp" +#include "indexer/indexer_tests/test_mwm_set.hpp" + #include "platform/country_file.hpp" #include "platform/local_country_file.hpp" #include "platform/local_country_file_utils.hpp" @@ -253,9 +255,10 @@ private: int m_slot; }; -void InitStorage(Storage & storage, TaskRunner & runner) +void InitStorage(Storage & storage, TaskRunner & runner, + Storage::TUpdate const & update = &OnCountryDownloaded) { - storage.Init(&OnCountryDownloaded); + storage.Init(update); storage.RegisterAllLocalMaps(); storage.SetDownloaderForTesting(make_unique(runner)); } @@ -418,10 +421,16 @@ UNIT_TEST(StorageTest_DownloadMapAndRoutingSeparately) { Storage storage; TaskRunner runner; - InitStorage(storage, runner); + tests::TestMwmSet mwmSet; + InitStorage(storage, runner, [&mwmSet](LocalCountryFile const & localFile) + { + mwmSet.Register(localFile); + }); TIndex const index = storage.FindIndexByFile("Azerbaijan"); TEST(index.IsValid(), ()); + CountryFile const countryFile = storage.GetCountryFile(index); + storage.DeleteCountry(index, TMapOptions::EMapWithCarRouting); // Download map file only. @@ -436,6 +445,10 @@ UNIT_TEST(StorageTest_DownloadMapAndRoutingSeparately) TEST(localFileA.get(), ()); TEST_EQUAL(TMapOptions::EMap, localFileA->GetFiles(), ()); + MwmSet::MwmId id = mwmSet.GetMwmIdByCountryFile(countryFile); + TEST(id.IsAlive(), ()); + TEST_EQUAL(TMapOptions::EMap, id.GetInfo()->GetLocalFile().GetFiles(), ()); + // Download routing file in addition to exising map file. { unique_ptr checker = @@ -449,6 +462,9 @@ UNIT_TEST(StorageTest_DownloadMapAndRoutingSeparately) TEST_EQUAL(localFileA.get(), localFileB.get(), (*localFileA, *localFileB)); TEST_EQUAL(TMapOptions::EMapWithCarRouting, localFileB->GetFiles(), ()); + TEST(id.IsAlive(), ()); + TEST_EQUAL(TMapOptions::EMapWithCarRouting, id.GetInfo()->GetLocalFile().GetFiles(), ()); + // Delete routing file and check status update. { CountryStatusChecker checker(storage, index, TStatus::EOnDisk); @@ -459,11 +475,19 @@ UNIT_TEST(StorageTest_DownloadMapAndRoutingSeparately) TEST_EQUAL(localFileB.get(), localFileC.get(), (*localFileB, *localFileC)); TEST_EQUAL(TMapOptions::EMap, localFileC->GetFiles(), ()); + TEST(id.IsAlive(), ()); + TEST_EQUAL(TMapOptions::EMap, id.GetInfo()->GetLocalFile().GetFiles(), ()); + // Delete map file and check status update. { CountryStatusChecker checker(storage, index, TStatus::ENotDownloaded); storage.DeleteCountry(index, TMapOptions::EMap); } + + // Framework should notify MwmSet about deletion of a map file. + // As there're no framework, there should not be any changes in MwmInfo. + TEST(id.IsAlive(), ()); + TEST_EQUAL(TMapOptions::EMap, id.GetInfo()->GetLocalFile().GetFiles(), ()); } UNIT_TEST(StorageTest_DeletePendingCountry) @@ -517,7 +541,7 @@ UNIT_TEST(StorageTest_DownloadTwoCountriesAndDelete) unique_ptr venezuelaChecker = make_unique( storage, venezuelaIndex, TMapOptions::EMapWithCarRouting, vector{TStatus::ENotDownloaded, TStatus::EInQueue, TStatus::EDownloading, - TStatus::EOnDisk}); + TStatus::EDownloading, TStatus::EOnDisk}); uruguayChecker->StartDownload(); venezuelaChecker->StartDownload(); storage.DeleteCountry(uruguayIndex, TMapOptions::EMap); @@ -541,7 +565,7 @@ UNIT_TEST(StorageTest_CancelDownloadingWhenAlmostDone) TIndex const index = storage.FindIndexByFile("Uruguay"); TEST(index.IsValid(), ()); storage.DeleteCountry(index, TMapOptions::EMapWithCarRouting); - MY_SCOPE_GUARD(cleanupUruguayFiles, + MY_SCOPE_GUARD(cleanupFiles, bind(&Storage::DeleteCountry, &storage, index, TMapOptions::EMapWithCarRouting)); {