diff --git a/android/jni/com/mapswithme/maps/MapManager.cpp b/android/jni/com/mapswithme/maps/MapManager.cpp index 416692ad36..2beded5d80 100644 --- a/android/jni/com/mapswithme/maps/MapManager.cpp +++ b/android/jni/com/mapswithme/maps/MapManager.cpp @@ -342,7 +342,14 @@ Java_com_mapswithme_maps_downloader_MapManager_nativeIsDownloading(JNIEnv * env, JNIEXPORT jstring JNICALL Java_com_mapswithme_maps_downloader_MapManager_nativeGetCurrentDownloadingCountryId(JNIEnv * env, jclass) { - return jni::ToJavaString(env, GetStorage().GetCurrentDownloadingCountryId()); + auto const & downloadingCountries = GetStorage().GetCurrentDownloadingCountries(); + + if (downloadingCountries.empty()) + return nullptr; + + ASSERT_EQUAL(downloadingCountries.size(), 1, ()); + + return jni::ToJavaString(env, downloadingCountries.begin()->first); } static void StartBatchingCallbacks() diff --git a/platform/downloader_defines.hpp b/platform/downloader_defines.hpp index 625fde5ebd..df948234f9 100644 --- a/platform/downloader_defines.hpp +++ b/platform/downloader_defines.hpp @@ -28,6 +28,7 @@ inline std::string DebugPrint(DownloadStatus status) UNREACHABLE(); } -/// , total can be -1 if size is unknown +int64_t constexpr kUnknownTotalSize = -1; +/// , total can be kUnknownTotalSize if size is unknown using Progress = std::pair; } // namespace downloader diff --git a/search/search_integration_tests/downloader_search_test.cpp b/search/search_integration_tests/downloader_search_test.cpp index ddf5459cb3..fb61ae1ead 100644 --- a/search/search_integration_tests/downloader_search_test.cpp +++ b/search/search_integration_tests/downloader_search_test.cpp @@ -87,12 +87,6 @@ class TestMapFilesDownloader : public storage::MapFilesDownloader { public: // MapFilesDownloader overrides: - downloader::Progress GetDownloadingProgress() override { return {}; } - - bool IsIdle() override { return false; } - - void Pause() override {} - void Resume() override {} void Remove(storage::CountryId const & id) override {} void Clear() override {} diff --git a/storage/http_map_files_downloader.cpp b/storage/http_map_files_downloader.cpp index 2083d06673..662487914b 100644 --- a/storage/http_map_files_downloader.cpp +++ b/storage/http_map_files_downloader.cpp @@ -86,33 +86,6 @@ void HttpMapFilesDownloader::Download() } } -downloader::Progress HttpMapFilesDownloader::GetDownloadingProgress() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - ASSERT(nullptr != m_request, ()); - return m_request->GetProgress(); -} - -bool HttpMapFilesDownloader::IsIdle() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - return m_request.get() == nullptr; -} - -void HttpMapFilesDownloader::Pause() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - m_request.reset(); -} - -void HttpMapFilesDownloader::Resume() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - - if (!m_request && !m_queue.empty()) - Download(); -} - void HttpMapFilesDownloader::Remove(CountryId const & id) { CHECK_THREAD_CHECKER(m_checker, ()); @@ -134,6 +107,10 @@ void HttpMapFilesDownloader::Remove(CountryId const & id) for (auto const subscriber : m_subscribers) subscriber->OnFinishDownloading(); } + else if (!m_request) + { + Download(); + } } void HttpMapFilesDownloader::Clear() diff --git a/storage/http_map_files_downloader.hpp b/storage/http_map_files_downloader.hpp index 5a1157c25c..6f9ec5587a 100644 --- a/storage/http_map_files_downloader.hpp +++ b/storage/http_map_files_downloader.hpp @@ -23,10 +23,6 @@ public: virtual ~HttpMapFilesDownloader(); // MapFilesDownloader overrides: - downloader::Progress GetDownloadingProgress() override; - bool IsIdle() override; - void Pause() override; - void Resume() override; void Remove(CountryId const & id) override; void Clear() override; Queue const & GetQueue() const override; diff --git a/storage/map_files_downloader.hpp b/storage/map_files_downloader.hpp index 439128b5c0..0e245ce30d 100644 --- a/storage/map_files_downloader.hpp +++ b/storage/map_files_downloader.hpp @@ -45,15 +45,6 @@ public: /// callback. Both callbacks will be invoked on the main thread. void DownloadMapFile(QueuedCountry & queuedCountry); - /// Returns current downloading progress. - virtual downloader::Progress GetDownloadingProgress() = 0; - - /// Returns true when downloader does not perform any job. - virtual bool IsIdle() = 0; - - virtual void Pause() = 0; - virtual void Resume() = 0; - // Removes item from m_quarantine queue when list of servers is not received. // Parent method must be called into override method. virtual void Remove(CountryId const & id); diff --git a/storage/storage.cpp b/storage/storage.cpp index d7abeaf3c7..af36350b55 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -384,10 +384,10 @@ LocalAndRemoteSize Storage::CountrySizeInBytes(CountryId const & countryId) cons if (!IsCountryInQueue(countryId) && !IsDiffApplyingInProgressToCountry(countryId)) sizes.first = localFile ? localFile->GetSize(MapFileType::Map) : 0; - if (!m_downloader->IsIdle() && IsCountryFirstInQueue(countryId)) - { - sizes.first = m_downloader->GetDownloadingProgress().first + GetRemoteSize(countryFile); - } + auto const it = m_downloadingCountries.find(countryId); + if (it != m_downloadingCountries.cend()) + sizes.first = it->second.first + GetRemoteSize(countryFile); + return sizes; } @@ -444,7 +444,7 @@ Status Storage::CountryStatus(CountryId const & countryId) const // Check if we are already downloading this country or have it in the queue. if (IsCountryInQueue(countryId)) { - if (IsCountryFirstInQueue(countryId)) + if (m_downloadingCountries.find(countryId) != m_downloadingCountries.cend()) return Status::EDownloading; return Status::EInQueue; @@ -559,9 +559,9 @@ void Storage::DeleteCountry(CountryId const & countryId, MapFileType type) DeleteCountryFilesFromDownloader(countryId); m_diffsDataSource->RemoveDiffForCountry(countryId); - NotifyStatusChangedForHierarchy(countryId); + m_downloadingCountries.erase(countryId); - m_downloader->Resume(); + NotifyStatusChangedForHierarchy(countryId); } void Storage::DeleteCustomCountryVersion(LocalCountryFile const & localFile) @@ -616,12 +616,11 @@ bool Storage::IsDownloadInProgress() const return !m_downloader->GetQueue().empty(); } -CountryId Storage::GetCurrentDownloadingCountryId() const +Storage::DownloadingCountries const & Storage::GetCurrentDownloadingCountries() const { CHECK_THREAD_CHECKER(m_threadChecker, ()); - return IsDownloadInProgress() ? m_downloader->GetQueue().front().GetCountryId() - : storage::CountryId(); + return m_downloadingCountries; } void Storage::LoadCountriesFile(string const & pathToCountriesFile) @@ -670,6 +669,8 @@ void Storage::OnMapFileDownloadFinished(QueuedCountry const & queuedCountry, Dow { CHECK_THREAD_CHECKER(m_threadChecker, ()); + m_downloadingCountries.erase(queuedCountry.GetCountryId()); + OnMapDownloadFinished(queuedCountry.GetCountryId(), status, queuedCountry.GetFileType()); } @@ -695,8 +696,7 @@ void Storage::ReportProgressForHierarchy(CountryId const & countryId, Progress c descendants.push_back(container.Value().Name()); }); - Progress localAndRemoteBytes = - CalculateProgress(countryId, descendants, leafProgress, setQueue); + Progress localAndRemoteBytes = CalculateProgress(descendants); ReportProgress(parentId, localAndRemoteBytes); }; @@ -711,6 +711,8 @@ void Storage::OnMapFileDownloadProgress(QueuedCountry const & queuedCountry, if (m_observers.empty()) return; + m_downloadingCountries[queuedCountry.GetCountryId()] = progress; + ReportProgressForHierarchy(queuedCountry.GetCountryId(), progress); } @@ -841,14 +843,6 @@ bool Storage::IsCountryInQueue(CountryId const & countryId) const return find(queue.cbegin(), queue.cend(), countryId) != queue.cend(); } -bool Storage::IsCountryFirstInQueue(CountryId const & countryId) const -{ - CHECK_THREAD_CHECKER(m_threadChecker, ()); - - auto const & queue = m_downloader->GetQueue(); - return !queue.empty() && queue.front().GetCountryId() == countryId; -} - bool Storage::IsDiffApplyingInProgressToCountry(CountryId const & countryId) const { CHECK_THREAD_CHECKER(m_threadChecker, ()); @@ -1364,6 +1358,7 @@ void Storage::OnCountryInQueue(CountryId const & id) void Storage::OnStartDownloadingCountry(CountryId const & id) { + m_downloadingCountries[id] = {0, kUnknownTotalSize}; NotifyStatusChangedForHierarchy(id); } @@ -1468,21 +1463,8 @@ void Storage::GetNodeAttrs(CountryId const & countryId, NodeAttrs & nodeAttrs) c CountriesVec subtree; node->ForEachInSubtree( [&subtree](CountryTree::Node const & d) { subtree.push_back(d.Value().Name()); }); - CountryId const & downloadingMwm = - IsDownloadInProgress() ? GetCurrentDownloadingCountryId() : kInvalidCountryId; - Progress downloadingMwmProgress(0, 0); - if (!m_downloader->IsIdle()) - { - downloadingMwmProgress = m_downloader->GetDownloadingProgress(); - // If we don't know estimated file size then we ignore its progress. - if (downloadingMwmProgress.second == -1) - downloadingMwmProgress = {}; - } - CountriesSet setQueue; - GetQueuedCountries(m_downloader->GetQueue(), setQueue); - nodeAttrs.m_downloadingProgress = - CalculateProgress(downloadingMwm, subtree, downloadingMwmProgress, setQueue); + nodeAttrs.m_downloadingProgress = CalculateProgress(subtree); } // Local mwm information and information about downloading mwms. @@ -1568,19 +1550,23 @@ void Storage::DoClickOnDownloadMap(CountryId const & countryId) m_downloadMapOnTheMap(countryId); } -Progress Storage::CalculateProgress(CountryId const & downloadingMwm, CountriesVec const & mwms, - Progress const & downloadingMwmProgress, - CountriesSet const & mwmsInQueue) const +Progress Storage::CalculateProgress(CountriesVec const & descendants) const { // Function calculates progress correctly ONLY if |downloadingMwm| is leaf. Progress localAndRemoteBytes = {}; - for (auto const & d : mwms) + CountriesSet mwmsInQueue; + GetQueuedCountries(m_downloader->GetQueue(), mwmsInQueue); + + for (auto const & d : descendants) { - if (downloadingMwm == d && downloadingMwm != kInvalidCountryId) + auto const downloadingIt = m_downloadingCountries.find(d); + if (downloadingIt != m_downloadingCountries.cend()) { - localAndRemoteBytes.first += downloadingMwmProgress.first; + if (downloadingIt->second.second != downloader::kUnknownTotalSize) + localAndRemoteBytes.first += downloadingIt->second.first; + localAndRemoteBytes.second += GetRemoteSize(GetCountryFile(d)); } else if (mwmsInQueue.count(d) != 0) @@ -1626,11 +1612,11 @@ void Storage::CancelDownloadNode(CountryId const & countryId) needNotify = true; } + m_downloadingCountries.erase(countryId); + if (needNotify) NotifyStatusChangedForHierarchy(countryId); }); - - m_downloader->Resume(); } void Storage::RetryDownloadNode(CountryId const & countryId) diff --git a/storage/storage.hpp b/storage/storage.hpp index 4bf2cf40eb..bd098bac2a 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -157,6 +157,7 @@ public: using DeleteCallback = std::function; using ChangeCountryFunction = std::function; using ProgressFunction = std::function; + using DownloadingCountries = std::unordered_map; private: /// We support only one simultaneous request at the moment @@ -251,6 +252,8 @@ private: StartDownloadingCallback m_startDownloadingCallback; + DownloadingCountries m_downloadingCountries; + void LoadCountriesFile(std::string const & pathToCountriesFile); void ReportProgress(CountryId const & countryId, downloader::Progress const & p); @@ -534,9 +537,9 @@ public: /// Notifies observers about country status change. void DeleteCustomCountryVersion(platform::LocalCountryFile const & localFile); - bool IsDownloadInProgress() const; + DownloadingCountries const & GetCurrentDownloadingCountries() const; - CountryId GetCurrentDownloadingCountryId() const; + bool IsDownloadInProgress() const; /// @param[out] res Populated with oudated countries. void GetOutdatedCountries(std::vector & countries) const; @@ -574,9 +577,6 @@ private: // Returns true when country is in the downloader's queue. bool IsCountryInQueue(CountryId const & countryId) const; - // Returns true when country is first in the downloader's queue. - bool IsCountryFirstInQueue(CountryId const & countryId) const; - // Returns true if we started the diff applying procedure for an mwm with countryId. bool IsDiffApplyingInProgressToCountry(CountryId const & countryId) const; @@ -632,14 +632,7 @@ private: /// |descendants| All descendants of the parent node. /// |downloadingMwm| Downloading leaf node country id if any. If not, downloadingMwm == kInvalidCountryId. /// |downloadingMwm| Must be only leaf. - /// If downloadingMwm != kInvalidCountryId |downloadingMwmProgress| is a progress of downloading - /// the leaf node in bytes. |downloadingMwmProgress.first| == number of downloaded bytes. - /// |downloadingMwmProgress.second| == number of bytes in downloading files. - /// |mwmsInQueue| hash table made from |m_queue|. - downloader::Progress CalculateProgress(CountryId const & downloadingMwm, - CountriesVec const & descendants, - downloader::Progress const & downloadingMwmProgress, - CountriesSet const & mwmsInQueue) const; + downloader::Progress CalculateProgress(CountriesVec const & descendants) const; template void ForEachAncestorExceptForTheRoot(std::vector const & nodes, diff --git a/storage/storage_tests/fake_map_files_downloader.cpp b/storage/storage_tests/fake_map_files_downloader.cpp index 248b9588f2..6d5b223883 100644 --- a/storage/storage_tests/fake_map_files_downloader.cpp +++ b/storage/storage_tests/fake_map_files_downloader.cpp @@ -27,13 +27,11 @@ void FakeMapFilesDownloader::Download(QueuedCountry & queuedCountry) m_queue.push_back(queuedCountry); - if (m_queue.size() != 1) - { - for (auto const subscriber : m_subscribers) - subscriber->OnCountryInQueue(queuedCountry.GetCountryId()); + for (auto const subscriber : m_subscribers) + subscriber->OnCountryInQueue(queuedCountry.GetCountryId()); + if (m_queue.size() != 1) return; - } for (auto const subscriber : m_subscribers) subscriber->OnStartDownloading(); @@ -41,40 +39,6 @@ void FakeMapFilesDownloader::Download(QueuedCountry & queuedCountry) Download(); } -downloader::Progress FakeMapFilesDownloader::GetDownloadingProgress() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - - return m_progress; -} - -bool FakeMapFilesDownloader::IsIdle() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - - return m_writer.get() == nullptr; -} - -void FakeMapFilesDownloader::Pause() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - - m_writer.reset(); - ++m_timestamp; -} - -void FakeMapFilesDownloader::Resume() -{ - CHECK_THREAD_CHECKER(m_checker, ()); - - if (m_queue.empty() || m_writer) - return; - - m_writer.reset(new FileWriter(m_queue.front().GetFileDownloadPath())); - ++m_timestamp; - m_taskRunner.PostTask(std::bind(&FakeMapFilesDownloader::DownloadNextChunk, this, m_timestamp)); -} - void FakeMapFilesDownloader::Remove(CountryId const & id) { CHECK_THREAD_CHECKER(m_checker, ()); diff --git a/storage/storage_tests/fake_map_files_downloader.hpp b/storage/storage_tests/fake_map_files_downloader.hpp index e99eac93c8..1ada40b2a9 100644 --- a/storage/storage_tests/fake_map_files_downloader.hpp +++ b/storage/storage_tests/fake_map_files_downloader.hpp @@ -34,10 +34,6 @@ public: ~FakeMapFilesDownloader(); // MapFilesDownloader overrides: - downloader::Progress GetDownloadingProgress() override; - bool IsIdle() override; - void Pause() override; - void Resume() override; void Remove(CountryId const & id) override; void Clear() override; Queue const & GetQueue() const override; diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index b559a03196..c36260d707 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -315,7 +315,7 @@ public: TaskRunner & runner) : CountryDownloaderChecker( storage, countryId, MapFileType::Map, - vector{Status::ENotDownloaded, Status::EDownloading, Status::ENotDownloaded}) + vector{Status::ENotDownloaded, Status::EInQueue, Status::EDownloading, Status::ENotDownloaded}) , m_runner(runner) { } @@ -345,7 +345,7 @@ unique_ptr AbsentCountryDownloaderChecker(Storage & st { return make_unique( storage, countryId, type, - vector{Status::ENotDownloaded, Status::EDownloading, Status::EOnDisk}); + vector{Status::ENotDownloaded, Status::EInQueue, Status::EDownloading, Status::EOnDisk}); } // Checks following state transitions: @@ -356,7 +356,7 @@ unique_ptr CancelledCountryDownloaderChecker(Storage & { return make_unique( storage, countryId, type, - vector{Status::ENotDownloaded, Status::EDownloading, Status::ENotDownloaded}); + vector{Status::ENotDownloaded, Status::EInQueue, Status::EDownloading, Status::ENotDownloaded}); } class CountryStatusChecker @@ -770,7 +770,8 @@ UNIT_CLASS_TEST(StorageTest, DownloadedMap) { auto algeriaCentralChecker = make_unique( storage, algeriaCentralCountryId, MapFileType::Map, - vector{Status::ENotDownloaded, Status::EDownloading, Status::EOnDisk}); + vector{Status::ENotDownloaded, Status::EInQueue, Status::EDownloading, + Status::EOnDisk}); auto algeriaCoastChecker = make_unique( storage, algeriaCoastCountryId, MapFileType::Map,