From 3baed0ecef6614e147a7c4294f7c7da8883f170a Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Tue, 9 Feb 2016 17:08:15 +0300 Subject: [PATCH] [new downloader] Review fixes. The second part. --- storage/country_info_getter.cpp | 2 +- storage/country_info_getter.hpp | 8 ++--- storage/storage.cpp | 12 +++---- storage/storage.hpp | 9 ++--- storage/storage_helpers.cpp | 10 +++--- storage/storage_helpers.hpp | 4 +-- .../country_info_getter_test.cpp | 14 ++++---- .../create_country_info_getter.cpp | 33 ------------------- storage/storage_tests/helpers.cpp | 33 +++++++++++++++++++ ...te_country_info_getter.hpp => helpers.hpp} | 2 +- storage/storage_tests/storage_tests.cpp | 10 +++--- storage/storage_tests/storage_tests.pro | 4 +-- 12 files changed, 71 insertions(+), 70 deletions(-) delete mode 100644 storage/storage_tests/create_country_info_getter.cpp create mode 100644 storage/storage_tests/helpers.cpp rename storage/storage_tests/{create_country_info_getter.hpp => helpers.hpp} (78%) diff --git a/storage/country_info_getter.cpp b/storage/country_info_getter.cpp index c9dd6d84a2..be5c1fe9c1 100644 --- a/storage/country_info_getter.cpp +++ b/storage/country_info_getter.cpp @@ -102,7 +102,7 @@ m2::RectD CountryInfoGetter::CalcLimitRect(string const & prefix) const return rect; } -m2::RectD CountryInfoGetter::CalcLimitRectForLeaf(TCountryId leafCountryId) const +m2::RectD CountryInfoGetter::GetLimitRectForLeaf(TCountryId const & leafCountryId) const { auto const it = this->m_countryIndex.find(leafCountryId); ASSERT(it != this->m_countryIndex.end(), ()); diff --git a/storage/country_info_getter.hpp b/storage/country_info_getter.hpp index f7f19d5085..f5016e83a3 100644 --- a/storage/country_info_getter.hpp +++ b/storage/country_info_getter.hpp @@ -53,10 +53,10 @@ public: // Calculates limit rect for all countries whose name starts with // |prefix|. m2::RectD CalcLimitRect(string const & prefix) const; - // Calculates limit rect for |countryId| (non-expandable node). + // Returns limit rect for |countryId| (non-expandable node). // Returns bounding box in mercator coordinates if |countryId| is a country id of non-expandable node // and zero rect otherwise. - m2::RectD CalcLimitRectForLeaf(TCountryId leafCountryId) const; + m2::RectD GetLimitRectForLeaf(TCountryId const & leafCountryId) const; // Returns identifiers for all regions matching to |enNamePrefix|. void GetMatchedRegions(string const & enNamePrefix, IdSet & regions) const; @@ -88,11 +88,11 @@ protected: // Returns true when |pt| belongs to a country identified by |id|. virtual bool IsBelongToRegionImpl(size_t id, m2::PointD const & pt) const = 0; - // @TODO(bykoianko) Probably it's possible to redesign the class to get rid of m_countryIndex. + // @TODO(bykoianko): consider to get rid of m_countryIndex. // The possibility should be considered. // List of all known countries. vector m_countries; - // Maps all leaf country id (file names) to their index in m_countries. + // Maps all leaf country id (file names) to their indices in m_countries. unordered_map m_countryIndex; // Maps country file name without an extension to a country info. diff --git a/storage/storage.cpp b/storage/storage.cpp index 5251e47148..7df8dd63ca 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -1146,18 +1146,18 @@ void Storage::DoClickOnDownloadMap(TCountryId const & countryId) m_downloadMapOnTheMap(countryId); } -void Storage::ForEachInSubtree(TCountryId const & parent, TForEachFunction const & forEach) const +void Storage::ForEachInSubtree(TCountryId const & root, TForEachFunction && toDo) const { - TCountriesContainer const * const parentNode = m_countries.Find(Country(parent)); - if (parentNode == nullptr) + TCountriesContainer const * const rootNode = m_countries.Find(Country(root)); + if (rootNode == nullptr) { - ASSERT(false, ("TCountryId =", parent, "not found in m_countries.")); + ASSERT(false, ("TCountryId =", root, "not found in m_countries.")); return; } - parentNode->ForEachInSubtree([&forEach](TCountriesContainer const & countryContainer) + rootNode->ForEachInSubtree([&toDo](TCountriesContainer const & countryContainer) { Country const & value = countryContainer.Value(); - forEach(value.Name(), value.GetSubtreeMwmCounter() != 1 /* expandableNode. */); + toDo(value.Name(), value.GetSubtreeMwmCounter() != 1 /* expandableNode. */); }); } } // namespace storage diff --git a/storage/storage.hpp b/storage/storage.hpp index 76444510eb..adba9168ce 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -76,7 +76,7 @@ public: using TUpdate = function; using TChangeCountryFunction = function; using TProgressFunction = function; - using TForEachFunction = function; + using TForEachFunction = function; private: /// We support only one simultaneous request at the moment @@ -232,9 +232,10 @@ public: /// a list of available maps. They are all available countries expect for fully downloaded /// countries. That means all mwm of the countries have been downloaded. void GetCountyListToDownload(TCountriesVec & countryList) const; - /// \brief Calls |forEach| for each node for subtree with parent == |parent|. - /// For example ForEachInSubtree(GetRootId()) calls |forEach| for every node including the root. - void ForEachInSubtree(TCountryId const & parent, TForEachFunction const & forEach) const; + /// \brief Calls |toDo| for each node for subtree with |root|. + /// For example ForEachInSubtree(GetRootId()) calls |toDo| for every node including + /// the result of GetRootId() call. + void ForEachInSubtree(TCountryId const & root, TForEachFunction && toDo) const; /// \brief Returns current version for mwms which are available on the server. inline int64_t GetCurrentDataVersion() const { return m_currentVersion; } diff --git a/storage/storage_helpers.cpp b/storage/storage_helpers.cpp index 00d45504ff..ad01d4e004 100644 --- a/storage/storage_helpers.cpp +++ b/storage/storage_helpers.cpp @@ -18,19 +18,19 @@ bool IsDownloadFailed(Status status) status == Status::EUnknown; } -m2::RectD CalcLimitRect(TCountryId countryId, +m2::RectD CalcLimitRect(TCountryId const & countryId, Storage const & storage, CountryInfoGetter const & countryInfoGetter) { m2::RectD boundingBox; - auto const accumulater = - [&countryInfoGetter, &boundingBox](TCountryId const & descendantCountryId, bool expandableNode) + auto const accumulator = + [&countryInfoGetter, &boundingBox](TCountryId const & descendantId, bool expandableNode) { if (!expandableNode) - boundingBox.Add(countryInfoGetter.CalcLimitRectForLeaf(descendantCountryId)); + boundingBox.Add(countryInfoGetter.GetLimitRectForLeaf(descendantId)); }; - storage.ForEachInSubtree(countryId, accumulater); + storage.ForEachInSubtree(countryId, accumulator); ASSERT(boundingBox.IsValid(), ()); return boundingBox; diff --git a/storage/storage_helpers.hpp b/storage/storage_helpers.hpp index 445d9f82ac..a1c886ec70 100644 --- a/storage/storage_helpers.hpp +++ b/storage/storage_helpers.hpp @@ -20,9 +20,9 @@ bool IsPointCoveredByDownloadedMaps(m2::PointD const & position, bool IsDownloadFailed(Status status); -/// \brief Calculates limit rect for |countryId| (non expandable or not). +/// \brief Calculates limit rect for |countryId| (expandable or not). /// \returns bounding box in mercator coordinates. -m2::RectD CalcLimitRect(TCountryId countryId, +m2::RectD CalcLimitRect(TCountryId const & countryId, Storage const & storage, CountryInfoGetter const & countryInfoGetter); } // namespace storage diff --git a/storage/storage_tests/country_info_getter_test.cpp b/storage/storage_tests/country_info_getter_test.cpp index 45cea8a532..fdbcfa075d 100644 --- a/storage/storage_tests/country_info_getter_test.cpp +++ b/storage/storage_tests/country_info_getter_test.cpp @@ -1,6 +1,6 @@ #include "testing/testing.hpp" -#include "storage/storage_tests/create_country_info_getter.hpp" +#include "storage/storage_tests/helpers.hpp" #include "storage/country_info_getter.hpp" #include "storage/country.hpp" @@ -82,30 +82,30 @@ UNIT_TEST(CountryInfoGetter_SomeRects) LOG(LINFO, ("Canada: ", getter->CalcLimitRect("Canada_"))); } -UNIT_TEST(CountryInfoGetter_CalcLimitRectForLeafSingleMwm) +UNIT_TEST(CountryInfoGetter_GetLimitRectForLeafSingleMwm) { auto const getter = CreateCountryInfoGetterMigrate(); Storage storage(COUNTRIES_MIGRATE_FILE); if (!version::IsSingleMwm(storage.GetCurrentDataVersion())) return; - m2::RectD const boundingBox = getter->CalcLimitRectForLeaf("Angola"); + m2::RectD const boundingBox = getter->GetLimitRectForLeaf("Angola"); m2::RectD const expectedBoundingBox = {9.205259 /* minX */, -18.34456 /* minY */, 24.08212 /* maxX */, -4.393187 /* maxY */}; - TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); + TEST(AlmostEqualRectsAbs(boundingBox, expectedBoundingBox), ()); } -UNIT_TEST(CountryInfoGetter_CalcLimitRectForLeafTwoComponentMwm) +UNIT_TEST(CountryInfoGetter_GetLimitRectForLeafTwoComponentMwm) { auto const getter = CreateCountryInfoGetter(); Storage storage(COUNTRIES_FILE); if (version::IsSingleMwm(storage.GetCurrentDataVersion())) return; - m2::RectD const boundingBox = getter->CalcLimitRectForLeaf("Angola"); + m2::RectD const boundingBox = getter->GetLimitRectForLeaf("Angola"); m2::RectD const expectedBoundingBox = {11.50151 /* minX */, -18.344569 /* minY */, 24.08212 /* maxX */, -4.393187 /* maxY */}; - TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); + TEST(AlmostEqualRectsAbs(boundingBox, expectedBoundingBox), ()); } diff --git a/storage/storage_tests/create_country_info_getter.cpp b/storage/storage_tests/create_country_info_getter.cpp deleted file mode 100644 index b7b668de1e..0000000000 --- a/storage/storage_tests/create_country_info_getter.cpp +++ /dev/null @@ -1,33 +0,0 @@ -#include "testing/testing.hpp" - -#include "storage/storage_tests/create_country_info_getter.hpp" - -#include "storage/country_info_getter.hpp" - -#include "platform/platform.hpp" - -namespace storage -{ -unique_ptr CreateCountryInfoGetter() -{ - Platform & platform = GetPlatform(); - return unique_ptr(new CountryInfoReader(platform.GetReader(PACKED_POLYGONS_FILE), - platform.GetReader(COUNTRIES_FILE))); -} - -unique_ptr CreateCountryInfoGetterMigrate() -{ - Platform & platform = GetPlatform(); - return unique_ptr(new CountryInfoReader(platform.GetReader(PACKED_POLYGONS_MIGRATE_FILE), - platform.GetReader(COUNTRIES_MIGRATE_FILE))); -} - -void TestAlmostEqualRectsAbs(const m2::RectD & r1, const m2::RectD & r2) -{ - double constexpr kEpsilon = 1e-3; - TEST(my::AlmostEqualAbs(r1.maxX(), r2.maxX(), kEpsilon), ()); - TEST(my::AlmostEqualAbs(r1.maxY(), r2.maxY(), kEpsilon), ()); - TEST(my::AlmostEqualAbs(r1.minX(), r2.minX(), kEpsilon), ()); - TEST(my::AlmostEqualAbs(r1.minY(), r2.minY(), kEpsilon), ()); -} -} // namespace storage diff --git a/storage/storage_tests/helpers.cpp b/storage/storage_tests/helpers.cpp new file mode 100644 index 0000000000..6d935454d8 --- /dev/null +++ b/storage/storage_tests/helpers.cpp @@ -0,0 +1,33 @@ +#include "testing/testing.hpp" + +#include "storage/storage_tests/helpers.hpp" + +#include "storage/country_info_getter.hpp" + +#include "platform/platform.hpp" + +namespace storage +{ +unique_ptr CreateCountryInfoGetter() +{ + Platform & platform = GetPlatform(); + return make_unique(platform.GetReader(PACKED_POLYGONS_FILE), + platform.GetReader(COUNTRIES_FILE)); +} + +unique_ptr CreateCountryInfoGetterMigrate() +{ + Platform & platform = GetPlatform(); + return make_unique(platform.GetReader(PACKED_POLYGONS_MIGRATE_FILE), + platform.GetReader(COUNTRIES_MIGRATE_FILE)); +} + +bool AlmostEqualRectsAbs(const m2::RectD & r1, const m2::RectD & r2) +{ + double constexpr kEpsilon = 1e-3; + return my::AlmostEqualAbs(r1.maxX(), r2.maxX(), kEpsilon) + && my::AlmostEqualAbs(r1.maxY(), r2.maxY(), kEpsilon) + && my::AlmostEqualAbs(r1.minX(), r2.minX(), kEpsilon) + && my::AlmostEqualAbs(r1.minY(), r2.minY(), kEpsilon); +} +} // namespace storage diff --git a/storage/storage_tests/create_country_info_getter.hpp b/storage/storage_tests/helpers.hpp similarity index 78% rename from storage/storage_tests/create_country_info_getter.hpp rename to storage/storage_tests/helpers.hpp index 68b0739bbd..c8fbdffafb 100644 --- a/storage/storage_tests/create_country_info_getter.hpp +++ b/storage/storage_tests/helpers.hpp @@ -11,5 +11,5 @@ class CountryInfoGetter; unique_ptr CreateCountryInfoGetter(); unique_ptr CreateCountryInfoGetterMigrate(); -void TestAlmostEqualRectsAbs(const m2::RectD & r1, const m2::RectD & r2); +bool AlmostEqualRectsAbs(const m2::RectD & r1, const m2::RectD & r2); } // namespace storage diff --git a/storage/storage_tests/storage_tests.cpp b/storage/storage_tests/storage_tests.cpp index 675393b157..2c2a3baaf0 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -5,8 +5,8 @@ #include "storage/storage_defines.hpp" #include "storage/storage_helpers.hpp" -#include "storage/storage_tests/create_country_info_getter.hpp" #include "storage/storage_tests/fake_map_files_downloader.hpp" +#include "storage/storage_tests/helpers.hpp" #include "storage/storage_tests/task_runner.hpp" #include "storage/storage_tests/test_map_files_downloader.hpp" @@ -1227,10 +1227,10 @@ UNIT_TEST(StorageTest_ForEachInSubtree) Storage storage(kSingleMwmCountriesTxt, make_unique()); TCountriesVec leafVec; - auto const forEach = [&leafVec](TCountryId const & descendantCountryId, bool expandableNode) + auto const forEach = [&leafVec](TCountryId const & descendantId, bool expandableNode) { if (!expandableNode) - leafVec.push_back(descendantCountryId); + leafVec.push_back(descendantId); }; storage.ForEachInSubtree(storage.GetRootId(), forEach); @@ -1240,7 +1240,7 @@ UNIT_TEST(StorageTest_ForEachInSubtree) UNIT_TEST(StorageTest_CalcLimitRect) { - Storage storage(COUNTRIES_FILE); + Storage storage(COUNTRIES_MIGRATE_FILE); if (!version::IsSingleMwm(storage.GetCurrentDataVersion())) return; @@ -1254,6 +1254,6 @@ UNIT_TEST(StorageTest_CalcLimitRect) m2::RectD const expectedBoundingBox = {-8.6689 /* minX */, 19.32443 /* minY */, 11.99734 /* maxX */, 40.2488 /* maxY */}; - TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); + TEST(AlmostEqualRectsAbs(boundingBox, expectedBoundingBox), ()); } } // namespace storage diff --git a/storage/storage_tests/storage_tests.pro b/storage/storage_tests/storage_tests.pro index b1782f867d..de4509a56a 100644 --- a/storage/storage_tests/storage_tests.pro +++ b/storage/storage_tests/storage_tests.pro @@ -25,16 +25,16 @@ win32*|linux* { } HEADERS += \ - create_country_info_getter.hpp \ fake_map_files_downloader.hpp \ + helpers.hpp \ task_runner.hpp \ test_map_files_downloader.hpp \ SOURCES += \ ../../testing/testingmain.cpp \ country_info_getter_test.cpp \ - create_country_info_getter.cpp \ fake_map_files_downloader.cpp \ + helpers.cpp \ queued_country_tests.cpp \ simple_tree_test.cpp \ storage_tests.cpp \