From 904df3a2fa3eace49b50a4082e8c74a0cb24c554 Mon Sep 17 00:00:00 2001 From: Viktor Govako Date: Tue, 1 Nov 2022 21:52:26 +0300 Subject: [PATCH] [search] Fixed possible bug when canceling request. Signed-off-by: Viktor Govako --- indexer/mwm_set.cpp | 2 +- map/framework.cpp | 9 ++-- search/pre_ranker.cpp | 4 +- search/ranker.cpp | 67 ++++++++++++--------------- search/ranker.hpp | 2 +- search/result.cpp | 25 +++------- search/result.hpp | 12 ++--- search/search_tests/results_tests.cpp | 30 ++++++------ 8 files changed, 65 insertions(+), 86 deletions(-) diff --git a/indexer/mwm_set.cpp b/indexer/mwm_set.cpp index e901be08d5..faf6feaeaa 100644 --- a/indexer/mwm_set.cpp +++ b/indexer/mwm_set.cpp @@ -59,7 +59,7 @@ MwmSet::MwmHandle::MwmHandle(MwmSet & mwmSet, MwmId const & mwmId, } MwmSet::MwmHandle::MwmHandle(MwmHandle && handle) - : m_mwmId(handle.m_mwmId), m_mwmSet(handle.m_mwmSet), m_value(move(handle.m_value)) + : m_mwmId(std::move(handle.m_mwmId)), m_mwmSet(handle.m_mwmSet), m_value(move(handle.m_value)) { handle.m_mwmSet = nullptr; handle.m_mwmId.Reset(); diff --git a/map/framework.cpp b/map/framework.cpp index 51b3f12629..fa00e08fd9 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -2689,10 +2689,13 @@ bool Framework::ParseEditorDebugCommand(search::SearchParams const & params) return true; } - feature::TypesHolder const types(*ft); - results.AddResultNoChecks(search::Result(fid, feature::GetCenter(*ft), string(ft->GetReadableName()), - move(edit.second), types.GetBestType(), {})); + search::Result res(feature::GetCenter(*ft), string(ft->GetReadableName())); + res.SetAddress(move(edit.second)); + res.FromFeature(fid, feature::TypesHolder(*ft).GetBestType(), {}); + + results.AddResultNoChecks(std::move(res)); } + params.m_onResults(results); results.SetEndMarker(false /* isCancelled */); diff --git a/search/pre_ranker.cpp b/search/pre_ranker.cpp index 3794003e7e..ab1bb79465 100644 --- a/search/pre_ranker.cpp +++ b/search/pre_ranker.cpp @@ -129,9 +129,7 @@ void PreRanker::FillMissingFieldsInPreResults() } else { - /// @todo We always should have centers table for features (except newly created) or I miss something? - ASSERT(false, ("Centers table is missing?")); - + // Possible when search while MWM is reloading or updating (!IsAlive). if (!pivotFeaturesInitialized) { m_pivotFeatures.SetPosition(m_params.m_accuratePivotCenter, m_params.m_scale); diff --git a/search/ranker.cpp b/search/ranker.cpp index aed0779d14..e03aacac48 100644 --- a/search/ranker.cpp +++ b/search/ranker.cpp @@ -446,7 +446,10 @@ private: { auto ft = loader.GetFeatureByIndex(id.m_index); if (ft) + { + ASSERT(id.IsValid(), ()); ft->SetID(id); + } return ft; } @@ -708,45 +711,32 @@ void Ranker::Finish(bool cancelled) m_emitter.Finish(cancelled); } -Result Ranker::MakeResult(RankerResult rankerResult, bool needAddress, bool needHighlighting) const +Result Ranker::MakeResult(RankerResult const & rankerResult, bool needAddress, bool needHighlighting) const { - // todo(@m) Used because Result does not have a default constructor. Factor out? - auto mk = [&](RankerResult & r) -> Result + Result res(rankerResult.GetCenter(), rankerResult.m_str); + + if (needAddress) { - string address; - if (needAddress) - { - address = GetLocalizedRegionInfoForResult(rankerResult); + string address = GetLocalizedRegionInfoForResult(rankerResult); - // Format full address only for suitable results. - if (ftypes::IsAddressObjectChecker::Instance()(rankerResult.GetTypes())) - { - address = FormatFullAddress( - LazyAddressGetter(m_reverseGeocoder, rankerResult.GetCenter()).GetNearbyAddress(), address); - } - } + // Format full address only for suitable results. + if (ftypes::IsAddressObjectChecker::Instance()(rankerResult.GetTypes())) + address = FormatFullAddress(LazyAddressGetter(m_reverseGeocoder, rankerResult.GetCenter()).GetNearbyAddress(), address); - string & name = rankerResult.m_str; + res.SetAddress(std::move(address)); + } - switch (r.GetResultType()) - { - case RankerResult::Type::Feature: - case RankerResult::Type::Building: - { - auto const type = rankerResult.GetBestType(&m_params.m_preferredTypes); - return Result(r.GetID(), r.GetCenter(), move(name), move(address), type, move(r.m_details)); - } - case RankerResult::Type::LatLon: return Result(r.GetCenter(), move(name), move(address)); - case RankerResult::Type::Postcode: return Result(r.GetCenter(), move(name)); - } - ASSERT(false, ("Bad RankerResult type:", static_cast(r.GetResultType()))); - UNREACHABLE(); - }; + switch (rankerResult.GetResultType()) + { + case RankerResult::Type::Feature: + case RankerResult::Type::Building: + res.FromFeature(rankerResult.GetID(), rankerResult.GetBestType(&m_params.m_preferredTypes), rankerResult.m_details); + break; + case RankerResult::Type::LatLon: res.SetType(Result::Type::LatLon); break; + case RankerResult::Type::Postcode: res.SetType(Result::Type::Postcode); break; + } - auto res = mk(rankerResult); - - if (needAddress && - ftypes::IsLocalityChecker::Instance().GetType(rankerResult.GetTypes()) == ftypes::LocalityType::None) + if (needAddress && ftypes::IsLocalityChecker::Instance().GetType(rankerResult.GetTypes()) == ftypes::LocalityType::None) { m_localities.GetLocality(res.GetFeatureCenter(), [&](LocalityItem const & item) { @@ -829,11 +819,14 @@ void Ranker::UpdateResults(bool lastUpdate) if (count >= m_params.m_limit) break; - auto & rankerResult = m_tentativeResults[i]; - if (!m_params.m_viewportSearch) - LOG(LDEBUG, (rankerResult)); + auto const & rankerResult = m_tentativeResults[i]; - Result result = MakeResult(move(rankerResult), m_params.m_needAddress, m_params.m_needHighlighting); + // Uncomment for debug purpose. + //if (!m_params.m_viewportSearch) + // LOG(LDEBUG, (rankerResult)); + + // Don't make move here in case of BailIfCancelled() throw. Objects in m_tentativeResults should remain valid. + Result result = MakeResult(rankerResult, m_params.m_needAddress, m_params.m_needHighlighting); if (m_params.m_viewportSearch) { diff --git a/search/ranker.hpp b/search/ranker.hpp index 9d53b28718..abd547105c 100644 --- a/search/ranker.hpp +++ b/search/ranker.hpp @@ -84,7 +84,7 @@ public: // Makes the final result that is shown to the user from a ranker's result. // |needAddress| and |needHighlighting| enable filling of optional fields // that may take a considerable amount of time to compute. - Result MakeResult(RankerResult rankerResult, bool needAddress, bool needHighlighting) const; + Result MakeResult(RankerResult const & rankerResult, bool needAddress, bool needHighlighting) const; void SuggestStrings(); diff --git a/search/result.cpp b/search/result.cpp index f753a8c5d4..2276ef0102 100644 --- a/search/result.cpp +++ b/search/result.cpp @@ -15,27 +15,14 @@ namespace search { using namespace std; -// Result ------------------------------------------------------------------------------------------ -Result::Result(FeatureID const & id, m2::PointD const & pt, string && str, - string && address, uint32_t featureType, Details && details) - : m_resultType(Type::Feature) - , m_id(id) - , m_center(pt) - , m_str(move(str)) - , m_address(move(address)) - , m_featureType(featureType) - , m_details(move(details)) +void Result::FromFeature(FeatureID const & id, uint32_t featureType, Details const & details) { -} + m_id = id; + ASSERT(m_id.IsValid(), ()); -Result::Result(m2::PointD const & pt, string && latlon, string && address) - : m_resultType(Type::LatLon), m_center(pt), m_str(move(latlon)), m_address(move(address)) -{ -} - -Result::Result(m2::PointD const & pt, string && postcode) - : m_resultType(Type::Postcode), m_center(pt), m_str(move(postcode)) -{ + m_featureType = featureType; + m_details = details; + m_resultType = Type::Feature; } Result::Result(string str, string && suggest) diff --git a/search/result.hpp b/search/result.hpp index 97d348320f..b37117fc25 100644 --- a/search/result.hpp +++ b/search/result.hpp @@ -73,15 +73,11 @@ public: // Min distance to search result when popularity label has a higher priority (in meters). static auto constexpr kPopularityHighPriorityMinDistance = 50000.0; - // For Type::Feature. - Result(FeatureID const & id, m2::PointD const & pt, std::string && str, - std::string && address, uint32_t featureType, Details && meta); + Result(m2::PointD const & pt, std::string const & name) : m_center(pt), m_str(name) {} + void FromFeature(FeatureID const & id, uint32_t featureType, Details const & details); - // For Type::LatLon. - Result(m2::PointD const & pt, std::string && latlon, std::string && address); - - // For Type::Postcode. - Result(m2::PointD const & pt, std::string && postcode); + void SetAddress(std::string && address) { m_address = std::move(address); } + void SetType(Result::Type type) { m_resultType = type; } // For Type::PureSuggest. Result(std::string str, std::string && suggest); diff --git a/search/search_tests/results_tests.cpp b/search/search_tests/results_tests.cpp index 92448f5782..7edafb1a3b 100644 --- a/search/search_tests/results_tests.cpp +++ b/search/search_tests/results_tests.cpp @@ -2,18 +2,24 @@ #include "search/result.hpp" +#include "indexer/data_source.hpp" + #include -namespace search +namespace search_tests { + UNIT_TEST(Results_Sorting) { - Results r; - MwmSet::MwmId id; + FrozenDataSource dataSource; + MwmSet::MwmId const id = dataSource.Register(platform::LocalCountryFile::MakeForTesting("minsk-pass")).first; + + search::Results r; for (uint32_t i = 5; i != 0; --i) { - r.AddResultNoChecks({{id, i}, {} /* pt */, {} /* str */, {} /* address */, - {} /* featureType */, {} /* metadata */}); + search::Result res(m2::PointD::Zero(), {}); + res.FromFeature({id, i}, 0, {}); + r.AddResultNoChecks(std::move(res)); } for (auto it = r.begin(); it != r.end(); ++it) @@ -37,22 +43,18 @@ UNIT_TEST(Results_Sorting) UNIT_TEST(Result_PrependCity) { - FeatureID const fid; - { - Result r(fid, m2::PointD::Zero(), "" /* str */, "Moscow, Russia" /* address */, - 0 /* featureType */, {} /* details */); - + search::Result r(m2::PointD::Zero(), {}); + r.SetAddress("Moscow, Russia"); r.PrependCity("Moscow"); TEST_EQUAL(r.GetAddress(), "Moscow, Russia", ()); } { - Result r(fid, m2::PointD::Zero(), "улица Михася Лынькова" /* str */, - "Минская область, Беларусь" /* address */, 0 /* featureType */, {} /* details */); - + search::Result r(m2::PointD::Zero(), "улица Михася Лынькова"); + r.SetAddress("Минская область, Беларусь"); r.PrependCity("Минск"); TEST_EQUAL(r.GetAddress(), "Минск, Минская область, Беларусь", ()); } } -} // namespace search +} // namespace search_tests