From 4b2f8d3edb0ea4f4c9e7aa07d63f514709634237 Mon Sep 17 00:00:00 2001 From: Vladimir Byko-Ianko Date: Tue, 9 Feb 2016 08:42:32 +0300 Subject: [PATCH] [new downloader] Review fixes. --- storage/country_info_getter.cpp | 8 ++--- storage/country_info_getter.hpp | 2 +- storage/storage.cpp | 10 +++--- storage/storage.hpp | 4 +-- storage/storage_helpers.cpp | 36 ++++++------------- storage/storage_helpers.hpp | 2 +- .../country_info_getter_test.cpp | 26 +++++++------- .../create_country_info_getter.cpp | 11 ++++++ .../create_country_info_getter.hpp | 4 +++ storage/storage_tests/storage_tests.cpp | 15 ++++---- 10 files changed, 57 insertions(+), 61 deletions(-) diff --git a/storage/country_info_getter.cpp b/storage/country_info_getter.cpp index eee3f07866..c9dd6d84a2 100644 --- a/storage/country_info_getter.cpp +++ b/storage/country_info_getter.cpp @@ -104,10 +104,10 @@ m2::RectD CountryInfoGetter::CalcLimitRect(string const & prefix) const m2::RectD CountryInfoGetter::CalcLimitRectForLeaf(TCountryId leafCountryId) const { - auto const index = this->m_countryIndex.find(leafCountryId); - ASSERT(index != this->m_countryIndex.end(), ()); - ASSERT_LESS(index->second, this->m_countries.size(), ()); - return m_countries[index->second].m_rect; + auto const it = this->m_countryIndex.find(leafCountryId); + ASSERT(it != this->m_countryIndex.end(), ()); + ASSERT_LESS(it->second, this->m_countries.size(), ()); + return m_countries[it->second].m_rect; } void CountryInfoGetter::GetMatchedRegions(string const & enNamePrefix, IdSet & regions) const diff --git a/storage/country_info_getter.hpp b/storage/country_info_getter.hpp index f3d7835475..f7f19d5085 100644 --- a/storage/country_info_getter.hpp +++ b/storage/country_info_getter.hpp @@ -54,7 +54,7 @@ public: // |prefix|. m2::RectD CalcLimitRect(string const & prefix) const; // Calculates limit rect for |countryId| (non-expandable node). - // Returns bound box in mercator coordinates if |countryId| is a country id of 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; diff --git a/storage/storage.cpp b/storage/storage.cpp index a2185985c4..5251e47148 100644 --- a/storage/storage.cpp +++ b/storage/storage.cpp @@ -976,7 +976,7 @@ TCountryId const Storage::GetRootId() const void Storage::GetChildren(TCountryId const & parent, TCountriesVec & childrenId) const { - TCountriesContainer const * parentNode = m_countries.Find(Country(parent)); + TCountriesContainer const * const parentNode = m_countries.Find(Country(parent)); if (parentNode == nullptr) { ASSERT(false, ("TCountryId =", parent, "not found in m_countries.")); @@ -1001,7 +1001,7 @@ void Storage::GetLocalRealMaps(TCountriesVec & localMaps) const void Storage::GetDownloadedChildren(TCountryId const & parent, TCountriesVec & localChildren) const { - TCountriesContainer const * parentNode = m_countries.Find(Country(parent)); + TCountriesContainer const * const parentNode = m_countries.Find(Country(parent)); if (parentNode == nullptr) { ASSERT(false, ("TCountryId =", parent, "not found in m_countries.")); @@ -1146,9 +1146,9 @@ void Storage::DoClickOnDownloadMap(TCountryId const & countryId) m_downloadMapOnTheMap(countryId); } -void Storage::ForEachInSubtree(TCountryId const & parent, TForEachFunction && forEach) const +void Storage::ForEachInSubtree(TCountryId const & parent, TForEachFunction const & forEach) const { - TCountriesContainer const * parentNode = m_countries.Find(Country(parent)); + TCountriesContainer const * const parentNode = m_countries.Find(Country(parent)); if (parentNode == nullptr) { ASSERT(false, ("TCountryId =", parent, "not found in m_countries.")); @@ -1157,7 +1157,7 @@ void Storage::ForEachInSubtree(TCountryId const & parent, TForEachFunction && fo parentNode->ForEachInSubtree([&forEach](TCountriesContainer const & countryContainer) { Country const & value = countryContainer.Value(); - forEach(value.Name(), value.GetSubtreeMwmCounter() != 1 /* It's a leaf. */); + forEach(value.Name(), value.GetSubtreeMwmCounter() != 1 /* expandableNode. */); }); } } // namespace storage diff --git a/storage/storage.hpp b/storage/storage.hpp index 074e4d6697..76444510eb 100644 --- a/storage/storage.hpp +++ b/storage/storage.hpp @@ -233,8 +233,8 @@ public: /// 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 GetAllLeavesInSubtree(GetRootId()) calls |forEach| for every node including the root. - void ForEachInSubtree(TCountryId const & parent, TForEachFunction && forEach) const; + /// For example ForEachInSubtree(GetRootId()) calls |forEach| for every node including the root. + void ForEachInSubtree(TCountryId const & parent, TForEachFunction const & forEach) 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 1807a61e24..00d45504ff 100644 --- a/storage/storage_helpers.cpp +++ b/storage/storage_helpers.cpp @@ -3,27 +3,6 @@ #include "storage/country_info_getter.hpp" #include "storage/storage.hpp" -namespace -{ -using namespace storage; - -class CalcLimitRectAccumulator -{ - m2::RectD m_boundBox; - CountryInfoGetter const & m_countryInfoGetter; -public: - CalcLimitRectAccumulator(CountryInfoGetter const & countryInfoGetter) - : m_countryInfoGetter(countryInfoGetter) {} - - m2::RectD GetBoundBox() { return m_boundBox; } - void operator()(TCountryId const & descendantCountryId, bool expandableNode) - { - if (!expandableNode) - m_boundBox.Add(m_countryInfoGetter.CalcLimitRectForLeaf(descendantCountryId)); - } -}; -} // namespace - namespace storage { bool IsPointCoveredByDownloadedMaps(m2::PointD const & position, @@ -43,12 +22,17 @@ m2::RectD CalcLimitRect(TCountryId countryId, Storage const & storage, CountryInfoGetter const & countryInfoGetter) { - CalcLimitRectAccumulator accumulater(countryInfoGetter); + m2::RectD boundingBox; + auto const accumulater = + [&countryInfoGetter, &boundingBox](TCountryId const & descendantCountryId, bool expandableNode) + { + if (!expandableNode) + boundingBox.Add(countryInfoGetter.CalcLimitRectForLeaf(descendantCountryId)); + }; + storage.ForEachInSubtree(countryId, accumulater); - m2::RectD const boundBox = accumulater.GetBoundBox(); - ASSERT(boundBox.IsValid(), ()); - - return accumulater.GetBoundBox(); + ASSERT(boundingBox.IsValid(), ()); + return boundingBox; } } // namespace storage diff --git a/storage/storage_helpers.hpp b/storage/storage_helpers.hpp index 72cc07563a..445d9f82ac 100644 --- a/storage/storage_helpers.hpp +++ b/storage/storage_helpers.hpp @@ -21,7 +21,7 @@ bool IsPointCoveredByDownloadedMaps(m2::PointD const & position, bool IsDownloadFailed(Status status); /// \brief Calculates limit rect for |countryId| (non expandable or not). -/// \returns bound box in mercator coordinates. +/// \returns bounding box in mercator coordinates. m2::RectD CalcLimitRect(TCountryId countryId, Storage const & storage, CountryInfoGetter const & countryInfoGetter); diff --git a/storage/storage_tests/country_info_getter_test.cpp b/storage/storage_tests/country_info_getter_test.cpp index c5166cfd42..45cea8a532 100644 --- a/storage/storage_tests/country_info_getter_test.cpp +++ b/storage/storage_tests/country_info_getter_test.cpp @@ -20,8 +20,6 @@ using namespace storage; namespace { -double constexpr kEpsilon = 1e-3; - bool IsEmptyName(map const & id2info, string const & id) { auto const it = id2info.find(id); @@ -91,23 +89,23 @@ UNIT_TEST(CountryInfoGetter_CalcLimitRectForLeafSingleMwm) if (!version::IsSingleMwm(storage.GetCurrentDataVersion())) return; - m2::RectD leafBoundBox = getter->CalcLimitRectForLeaf("Angola"); - TEST(my::AlmostEqualAbs(leafBoundBox.maxX(), 24.08212, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.maxY(), -4.393187, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.minX(), 9.205259, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.minY(), -18.34456, kEpsilon), ()); + m2::RectD const boundingBox = getter->CalcLimitRectForLeaf("Angola"); + m2::RectD const expectedBoundingBox = {9.205259 /* minX */, -18.34456 /* minY */, + 24.08212 /* maxX */, -4.393187 /* maxY */}; + + TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); } UNIT_TEST(CountryInfoGetter_CalcLimitRectForLeafTwoComponentMwm) { - auto const getter = CreateCountryInfoGetterMigrate(); - Storage storage(COUNTRIES_MIGRATE_FILE); + auto const getter = CreateCountryInfoGetter(); + Storage storage(COUNTRIES_FILE); if (version::IsSingleMwm(storage.GetCurrentDataVersion())) return; - m2::RectD leafBoundBox = getter->CalcLimitRectForLeaf("Angola"); - TEST(my::AlmostEqualAbs(leafBoundBox.maxX(), 24.08212, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.maxY(), -4.393187, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.minX(), 11.50151, kEpsilon), ()); - TEST(my::AlmostEqualAbs(leafBoundBox.minY(), -18.344569, kEpsilon), ()); + m2::RectD const boundingBox = getter->CalcLimitRectForLeaf("Angola"); + m2::RectD const expectedBoundingBox = {11.50151 /* minX */, -18.344569 /* minY */, + 24.08212 /* maxX */, -4.393187 /* maxY */}; + + TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); } diff --git a/storage/storage_tests/create_country_info_getter.cpp b/storage/storage_tests/create_country_info_getter.cpp index aad0700f02..b7b668de1e 100644 --- a/storage/storage_tests/create_country_info_getter.cpp +++ b/storage/storage_tests/create_country_info_getter.cpp @@ -1,3 +1,5 @@ +#include "testing/testing.hpp" + #include "storage/storage_tests/create_country_info_getter.hpp" #include "storage/country_info_getter.hpp" @@ -19,4 +21,13 @@ unique_ptr CreateCountryInfoGetterMigrate() 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/create_country_info_getter.hpp b/storage/storage_tests/create_country_info_getter.hpp index 8c36452221..68b0739bbd 100644 --- a/storage/storage_tests/create_country_info_getter.hpp +++ b/storage/storage_tests/create_country_info_getter.hpp @@ -1,5 +1,7 @@ #pragma once +#include "geometry/mercator.hpp" + #include "std/unique_ptr.hpp" namespace storage @@ -8,4 +10,6 @@ class CountryInfoGetter; unique_ptr CreateCountryInfoGetter(); unique_ptr CreateCountryInfoGetterMigrate(); + +void TestAlmostEqualRectsAbs(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 e9975dd72c..675393b157 100644 --- a/storage/storage_tests/storage_tests.cpp +++ b/storage/storage_tests/storage_tests.cpp @@ -1227,20 +1227,19 @@ UNIT_TEST(StorageTest_ForEachInSubtree) Storage storage(kSingleMwmCountriesTxt, make_unique()); TCountriesVec leafVec; - auto forEach = [&leafVec](TCountryId const & descendantCountryId, bool expandableNode) + auto const forEach = [&leafVec](TCountryId const & descendantCountryId, bool expandableNode) { if (!expandableNode) leafVec.push_back(descendantCountryId); }; storage.ForEachInSubtree(storage.GetRootId(), forEach); - TCountriesVec expectedLeafVec = {"Abkhazia", "Algeria_Central", "Algeria_Coast", "South Korea_South"}; + TCountriesVec const expectedLeafVec = {"Abkhazia", "Algeria_Central", "Algeria_Coast", "South Korea_South"}; TEST_EQUAL(leafVec, expectedLeafVec, ()); } UNIT_TEST(StorageTest_CalcLimitRect) { - double constexpr kEpsilon = 1e-3; Storage storage(COUNTRIES_FILE); if (!version::IsSingleMwm(storage.GetCurrentDataVersion())) return; @@ -1251,10 +1250,10 @@ UNIT_TEST(StorageTest_CalcLimitRect) auto const countryInfoGetter = CreateCountryInfoGetterMigrate(); ASSERT(countryInfoGetter, ()); - m2::RectD algeriaBoundBox = CalcLimitRect("Algeria", storage, *countryInfoGetter); - TEST(my::AlmostEqualAbs(algeriaBoundBox.maxX(), 11.99734, kEpsilon), ()); - TEST(my::AlmostEqualAbs(algeriaBoundBox.maxY(), 40.2488, kEpsilon), ()); - TEST(my::AlmostEqualAbs(algeriaBoundBox.minX(), -8.6689, kEpsilon), ()); - TEST(my::AlmostEqualAbs(algeriaBoundBox.minY(), 19.32443, kEpsilon), ()); + m2::RectD const boundingBox = CalcLimitRect("Algeria", storage, *countryInfoGetter); + m2::RectD const expectedBoundingBox = {-8.6689 /* minX */, 19.32443 /* minY */, + 11.99734 /* maxX */, 40.2488 /* maxY */}; + + TestAlmostEqualRectsAbs(boundingBox, expectedBoundingBox); } } // namespace storage