From a5d305092d1edca03ffae49c80b79203eafca99d Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Fri, 25 May 2018 18:03:56 +0300 Subject: [PATCH] [booking] filter refactoring. Review fixes --- .../jni/com/mapswithme/maps/SearchEngine.cpp | 26 +++---- iphone/Maps/Core/Search/MWMSearch.mm | 4 +- map/booking_availability_filter.cpp | 71 ++++++++++--------- map/booking_availability_filter.hpp | 6 +- map/booking_filter.hpp | 10 ++- map/booking_filter_params.hpp | 8 +-- map/booking_filter_processor.cpp | 30 ++++---- map/booking_filter_processor.hpp | 9 +-- map/framework.cpp | 16 ++--- map/framework.hpp | 5 +- map/map_tests/booking_filter_test.cpp | 23 +++--- map/search_api.cpp | 4 +- map/search_api.hpp | 7 +- partners_api/booking_api.hpp | 3 +- partners_api/booking_availability_params.hpp | 2 +- partners_api/booking_params_base.hpp | 1 - 16 files changed, 114 insertions(+), 111 deletions(-) diff --git a/android/jni/com/mapswithme/maps/SearchEngine.cpp b/android/jni/com/mapswithme/maps/SearchEngine.cpp index 49848aaa2f..116eea106b 100644 --- a/android/jni/com/mapswithme/maps/SearchEngine.cpp +++ b/android/jni/com/mapswithme/maps/SearchEngine.cpp @@ -489,17 +489,19 @@ void OnBookmarksSearchResults(search::BookmarksSearchParams::Results const & res env->CallVoidMethod(g_javaListener, method, jResults.get(), static_cast(timestamp)); } -void OnBookingFilterResults(std::shared_ptr const & params, +void OnBookingFilterResults(std::shared_ptr const & apiParams, vector const & featuresSorted) { // Ignore obsolete booking filter results. - if (!g_lastBookingFilterParams.Equals(*params)) + if (!g_lastBookingFilterParams.Equals(*apiParams)) return; - JNIEnv * env = jni::GetEnv(); - jni::TScopedLocalObjectArrayRef jResults(env, - usermark_helper::ToFeatureIdArray(env, featuresSorted)); - env->CallVoidMethod(g_javaListener, g_onFilterAvailableHotelsId, jResults.get()); + ASSERT(std::is_sorted(featuresSorted.cbegin(), featuresSorted.cend()), ()); + + JNIEnv * env = jni::GetEnv(); + jni::TScopedLocalObjectArrayRef jResults(env, + usermark_helper::ToFeatureIdArray(env, featuresSorted)); + env->CallVoidMethod(g_javaListener, g_onFilterAvailableHotelsId, jResults.get()); } } // namespace @@ -571,8 +573,8 @@ extern "C" params.m_onResults = bind(&OnResults, _1, _2, timestamp, false, hasPosition, lat, lon); params.m_hotelsFilter = g_hotelsFilterBuilder.Build(env, hotelsFilter); g_lastBookingFilterParams = g_bookingAvailabilityParamsBuilder.Build(env, bookingFilterParams); - params.m_bookingFilterParams.m_params = - std::make_shared(g_lastBookingFilterParams); + params.m_bookingFilterParams.m_apiParams = + std::make_shared(g_lastBookingFilterParams); params.m_bookingFilterParams.m_callback = bind(&OnBookingFilterResults, _1, _2); bool const searchStarted = g_framework->NativeFramework()->SearchEverywhere(params); @@ -590,8 +592,8 @@ extern "C" vparams.m_inputLocale = jni::ToNativeString(env, lang); vparams.m_hotelsFilter = g_hotelsFilterBuilder.Build(env, hotelsFilter); g_lastBookingFilterParams = g_bookingAvailabilityParamsBuilder.Build(env, bookingFilterParams); - vparams.m_bookingFilterParams.m_params = - std::make_shared(g_lastBookingFilterParams); + vparams.m_bookingFilterParams.m_apiParams = + std::make_shared(g_lastBookingFilterParams); vparams.m_bookingFilterParams.m_callback = bind(&OnBookingFilterResults, _1, _2); // TODO (@alexzatsepin): set up vparams.m_onCompleted here and use @@ -606,8 +608,8 @@ extern "C" eparams.m_onResults = bind(&OnResults, _1, _2, timestamp, isMapAndTable, false /* hasPosition */, 0.0 /* lat */, 0.0 /* lon */); eparams.m_hotelsFilter = vparams.m_hotelsFilter; - eparams.m_bookingFilterParams.m_params = - std::make_shared(g_lastBookingFilterParams); + eparams.m_bookingFilterParams.m_apiParams = + std::make_shared(g_lastBookingFilterParams); eparams.m_bookingFilterParams.m_callback = bind(&OnBookingFilterResults, _1, _2); if (g_framework->NativeFramework()->SearchEverywhere(eparams)) diff --git a/iphone/Maps/Core/Search/MWMSearch.mm b/iphone/Maps/Core/Search/MWMSearch.mm index 15bf3933cf..14c055bc39 100644 --- a/iphone/Maps/Core/Search/MWMSearch.mm +++ b/iphone/Maps/Core/Search/MWMSearch.mm @@ -102,10 +102,10 @@ using Observers = NSHashTable; }; m_everywhereParams.m_bookingFilterParams.m_callback = - [self](shared_ptr const & params, + [self](shared_ptr const & apiParams, std::vector const & sortedFeatures) { auto const & p = self->m_everywhereParams.m_bookingFilterParams; - if (p.m_params->IsEmpty() || !p.m_params->Equals(*params)) + if (p.m_apiParams->IsEmpty() || !p.m_apiParams->Equals(*apiParams)) return; self->m_bookingAvailableFeatureIDs = sortedFeatures; [self onSearchResultsUpdated]; diff --git a/map/booking_availability_filter.cpp b/map/booking_availability_filter.cpp index 93ed80eb94..46965984b8 100644 --- a/map/booking_availability_filter.cpp +++ b/map/booking_availability_filter.cpp @@ -58,29 +58,29 @@ void FillResults(HotelToResults && hotelToResults, std::vector cons { switch (hotelToResult.m_cacheStatus) { - case Cache::HotelStatus::Unavailable: continue; - case Cache::HotelStatus::Available: - { + case Cache::HotelStatus::Unavailable: continue; + case Cache::HotelStatus::Available: + { + results.AddResult(std::move(hotelToResult.m_result)); + continue; + } + case Cache::HotelStatus::NotReady: + { + auto hotelStatus = cache.Get(hotelToResult.m_hotelId); + CHECK_NOT_EQUAL(hotelStatus, Cache::HotelStatus::Absent, ()); + + if (hotelStatus == Cache::HotelStatus::Available) results.AddResult(std::move(hotelToResult.m_result)); - continue; - } - case Cache::HotelStatus::NotReady: - { - auto hotelStatus = cache.Get(hotelToResult.m_hotelId); - CHECK_NOT_EQUAL(hotelStatus, Cache::HotelStatus::Absent, ()); - if (hotelStatus == Cache::HotelStatus::Available) - results.AddResult(std::move(hotelToResult.m_result)); + continue; + } + case Cache::HotelStatus::Absent: + { + if (std::binary_search(hotelIds.cbegin(), hotelIds.cend(), hotelToResult.m_hotelId)) + results.AddResult(std::move(hotelToResult.m_result)); - continue; - } - case Cache::HotelStatus::Absent: - { - if (std::binary_search(hotelIds.cbegin(), hotelIds.cend(), hotelToResult.m_hotelId)) - results.AddResult(std::move(hotelToResult.m_result)); - - continue; - } + continue; + } } } } @@ -148,24 +148,25 @@ namespace filter { AvailabilityFilter::AvailabilityFilter(Delegate const & d) : FilterBase(d) {} -void AvailabilityFilter::ApplyFilter(search::Results const & results, ParamsInternal const & params) +void AvailabilityFilter::ApplyFilter(search::Results const & results, + ParamsInternal const & filterParams) { - ASSERT(params.m_params, ()); + ASSERT(filterParams.m_apiParams, ()); - auto const & p = *params.m_params; - auto const & cb = params.m_callback; + auto const & p = *filterParams.m_apiParams; + auto const & cb = filterParams.m_callback; UpdateParams(p); - m_params.m_hotelIds.clear(); + m_apiParams.m_hotelIds.clear(); HotelToResults hotelToResults; - PrepareData(GetDelegate().GetIndex(), results, hotelToResults, *m_cache, m_params); + PrepareData(GetDelegate().GetIndex(), results, hotelToResults, *m_cache, m_apiParams); - if (m_params.m_hotelIds.empty()) + if (m_apiParams.m_hotelIds.empty()) { search::Results result; - FillResults(std::move(hotelToResults), {}, *m_cache, result); + FillResults(std::move(hotelToResults), {} /* hotelIds */, *m_cache, result); cb(result); return; @@ -188,21 +189,21 @@ void AvailabilityFilter::ApplyFilter(search::Results const & results, ParamsInte }); }; - GetDelegate().GetApi().GetHotelAvailability(m_params, apiCallback); + GetDelegate().GetApi().GetHotelAvailability(m_apiParams, apiCallback); m_cache->RemoveOutdated(); } -void AvailabilityFilter::UpdateParams(ParamsBase const & params) +void AvailabilityFilter::UpdateParams(ParamsBase const & apiParams) { - if (m_params.Equals(params)) + if (m_apiParams.Equals(apiParams)) return; - m_params.Set(params); + m_apiParams.Set(apiParams); m_cache = std::make_shared(); } void AvailabilityFilter::GetFeaturesFromCache(search::Results const & results, - std::vector & sortedResults) + std::vector & sortedResults) { std::vector features; @@ -222,8 +223,8 @@ void AvailabilityFilter::GetFeaturesFromCache(search::Results const & results, { if (mwmId != featureId.m_mwmId) { - guard = my::make_unique(GetDelegate().GetIndex(), - featureId.m_mwmId); + guard = + my::make_unique(GetDelegate().GetIndex(), featureId.m_mwmId); mwmId = featureId.m_mwmId; } diff --git a/map/booking_availability_filter.hpp b/map/booking_availability_filter.hpp index 2ba4405add..dc986460ea 100644 --- a/map/booking_availability_filter.hpp +++ b/map/booking_availability_filter.hpp @@ -21,17 +21,17 @@ class AvailabilityFilter : public FilterBase { public: explicit AvailabilityFilter(Delegate const & d); - void ApplyFilter(search::Results const & results, ParamsInternal const & params) override; + void ApplyFilter(search::Results const & results, ParamsInternal const & filterParams) override; void GetFeaturesFromCache(search::Results const & results, std::vector & sortedResults) override; - void UpdateParams(ParamsBase const & params) override; + void UpdateParams(ParamsBase const & apiParams) override; private: using CachePtr = std::shared_ptr; CachePtr m_cache = std::make_shared(); - AvailabilityParams m_params; + AvailabilityParams m_apiParams; }; } // namespace filter } // namespace booking diff --git a/map/booking_filter.hpp b/map/booking_filter.hpp index e75756f51d..365abce237 100644 --- a/map/booking_filter.hpp +++ b/map/booking_filter.hpp @@ -35,15 +35,13 @@ public: explicit FilterBase(Delegate const & d) : m_delegate(d) {} virtual ~FilterBase() = default; - virtual void ApplyFilter(search::Results const & results, ParamsInternal const & params) = 0; + virtual void ApplyFilter(search::Results const & results, + ParamsInternal const & filterParams) = 0; virtual void GetFeaturesFromCache(search::Results const & results, std::vector & sortedResults) = 0; - virtual void UpdateParams(ParamsBase const & params) = 0; + virtual void UpdateParams(ParamsBase const & apiParams) = 0; - Delegate const & GetDelegate() const - { - return m_delegate; - } + Delegate const & GetDelegate() const { return m_delegate; } private: Delegate const & m_delegate; diff --git a/map/booking_filter_params.hpp b/map/booking_filter_params.hpp index c87ff5657c..69d99ca6a8 100644 --- a/map/booking_filter_params.hpp +++ b/map/booking_filter_params.hpp @@ -29,15 +29,15 @@ template struct ParamsImpl { ParamsImpl() = default; - ParamsImpl(std::shared_ptr params, R const & cb) - : m_params(move(params)) + ParamsImpl(std::shared_ptr apiParams, R const & cb) + : m_apiParams(apiParams) , m_callback(cb) { } - bool IsEmpty() const { return !m_params || m_params->IsEmpty(); } + bool IsEmpty() const { return !m_apiParams || m_apiParams->IsEmpty(); } - std::shared_ptr m_params; + std::shared_ptr m_apiParams; R m_callback; }; diff --git a/map/booking_filter_processor.cpp b/map/booking_filter_processor.cpp index deb62e46db..fa58a641ea 100644 --- a/map/booking_filter_processor.cpp +++ b/map/booking_filter_processor.cpp @@ -25,31 +25,33 @@ void FilterProcessor::ApplyFilters(search::Results const & results, // Run provided filters consecutively. for (size_t i = tasks.size() - 1; i > 0; --i) { - auto const & cb = tasks[i - 1].m_params.m_callback; + auto const & cb = tasks[i - 1].m_filterParams.m_callback; - tasks[i - 1].m_params.m_callback = - [this, cb, nextTask = std::move(tasks[i])](search::Results const & results) mutable + tasks[i - 1].m_filterParams.m_callback = + [ this, cb, nextTask = std::move(tasks[i]) ](search::Results const & results) mutable { cb(results); // Run the next filter with obtained results from the previous one. // Post different task on the file thread to increase granularity. GetPlatform().RunTask(Platform::Thread::File, [this, results, nextTask = std::move(nextTask)]() { - m_filters.at(nextTask.m_type)->ApplyFilter(results, nextTask.m_params); + m_filters.at(nextTask.m_type)->ApplyFilter(results, nextTask.m_filterParams); }); }; } // Run first filter. - m_filters.at(tasks.front().m_type)->ApplyFilter(results, tasks.front().m_params); + m_filters.at(tasks.front().m_type)->ApplyFilter(results, tasks.front().m_filterParams); }); } -void FilterProcessor::OnParamsUpdated(Type const type, std::shared_ptr const & params) +void FilterProcessor::OnParamsUpdated(Type const type, + std::shared_ptr const & apiParams) { - GetPlatform().RunTask(Platform::Thread::File, [this, type, params = std::move(params)]() mutable - { - m_filters.at(type)->UpdateParams(*params); - }); + GetPlatform().RunTask(Platform::Thread::File, + [this, type, apiParams = std::move(apiParams)]() mutable + { + m_filters.at(type)->UpdateParams(*apiParams); + }); } void FilterProcessor::GetFeaturesFromCache(Type const type, search::Results const & results, @@ -57,9 +59,11 @@ void FilterProcessor::GetFeaturesFromCache(Type const type, search::Results cons { GetPlatform().RunTask(Platform::Thread::File, [this, type, results, callback]() { - std::vector resultSorted; - m_filters.at(type)->GetFeaturesFromCache(results, resultSorted); - callback(resultSorted); + std::vector featuresSorted; + m_filters.at(type)->GetFeaturesFromCache(results, featuresSorted); + + ASSERT(std::is_sorted(featuresSorted.begin(), featuresSorted.end()), ()); + callback(featuresSorted); }); } diff --git a/map/booking_filter_processor.hpp b/map/booking_filter_processor.hpp index a50607d84b..d901be1e6e 100644 --- a/map/booking_filter_processor.hpp +++ b/map/booking_filter_processor.hpp @@ -33,17 +33,18 @@ enum class Type struct FilterTask { - FilterTask(Type const type, ParamsInternal && params) + FilterTask(Type const type, ParamsInternal && filterParams) : m_type(type) - , m_params(std::move(params)) + , m_filterParams(std::move(filterParams)) { } Type const m_type; - ParamsInternal m_params; + ParamsInternal m_filterParams; }; -using FillSearchMarksCallback = platform::SafeCallback availableHotelsSorted)>; +using FillSearchMarksCallback = + platform::SafeCallback availableHotelsSorted)>; class FilterProcessor : public FilterBase::Delegate { diff --git a/map/framework.cpp b/map/framework.cpp index 60fd35573d..645c88aa98 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -3151,8 +3151,8 @@ void Framework::ShowViewportSearchResults(bool clear, search::Results::ConstIter FillSearchResultsMarks(clear, results.begin(), results.end(), postProcessing); }; - m_bookingFilterProcessor.GetFeaturesFromCache(booking::filter::Type::Availability, - results, fillCallback); + m_bookingFilterProcessor.GetFeaturesFromCache(booking::filter::Type::Availability, results, + fillCallback); } void Framework::ClearViewportSearchResults() @@ -3397,17 +3397,17 @@ ugc::Reviews Framework::FilterUGCReviews(ugc::Reviews const & reviews) const return result; } -void Framework::FilterSearchResultsOnBooking(booking::filter::Params const & params, +void Framework::FilterSearchResultsOnBooking(booking::filter::Params const & filterParams, search::Results const & results, bool inViewport) { using namespace booking::filter; - auto const & p = params.m_params; - auto const & cb = params.m_callback; + auto const & apiParams = filterParams.m_apiParams; + auto const & cb = filterParams.m_callback; ParamsInternal paramsInternal { - p, - [this, p, cb, inViewport](search::Results const & results) + apiParams, + [this, apiParams, cb, inViewport](search::Results const & results) { if (results.GetCount() == 0) return; @@ -3428,7 +3428,7 @@ void Framework::FilterSearchResultsOnBooking(booking::filter::Params const & par }); } - cb(p, features); + cb(apiParams, features); } }; diff --git a/map/framework.hpp b/map/framework.hpp index adf3ee0f38..193ae54288 100644 --- a/map/framework.hpp +++ b/map/framework.hpp @@ -841,9 +841,10 @@ private: ugc::Reviews FilterUGCReviews(ugc::Reviews const & reviews) const; public: - void FilterSearchResultsOnBooking(booking::filter::Params const & params, + void FilterSearchResultsOnBooking(booking::filter::Params const & filterParams, search::Results const & results, bool inViewport) override; - void OnBookingAvailabilityParamsUpdate(std::shared_ptr const & params) override; + void OnBookingAvailabilityParamsUpdate( + std::shared_ptr const & params) override; private: // m_discoveryManager must be bellow m_searchApi, m_viatorApi, m_localsApi diff --git a/map/map_tests/booking_filter_test.cpp b/map/map_tests/booking_filter_test.cpp index f3686af8da..ebc31db00f 100644 --- a/map/map_tests/booking_filter_test.cpp +++ b/map/map_tests/booking_filter_test.cpp @@ -22,19 +22,13 @@ using namespace generator::tests_support; namespace { -class TestMwmEnvironment : public indexer::tests_support::TestWithCustomMwms - , public FilterBase::Delegate +class TestMwmEnvironment : public indexer::tests_support::TestWithCustomMwms, + public FilterBase::Delegate { public: - Index const & GetIndex() const override - { - return m_index; - } + Index const & GetIndex() const override { return m_index; } - booking::Api const & GetApi() const override - { - return m_api; - } + booking::Api const & GetApi() const override { return m_api; } protected: TestMwmEnvironment() @@ -93,16 +87,15 @@ UNIT_CLASS_TEST(TestMwmEnvironment, BookingFilter_AvailabilitySmoke) expectedResults.AddResult(std::move(copy)); }, rect, scales::GetUpperScale()); - ParamsInternal params; + ParamsInternal filterParams; search::Results filteredResults; - params.m_params = make_shared(); - params.m_callback = [&filteredResults](search::Results const & results) - { + filterParams.m_apiParams = make_shared(); + filterParams.m_callback = [&filteredResults](search::Results const & results) { filteredResults = results; testing::Notify(); }; - filter.ApplyFilter(results, params); + filter.ApplyFilter(results, filterParams); testing::Wait(); diff --git a/map/search_api.cpp b/map/search_api.cpp index 503dc9c22c..e563537bf4 100644 --- a/map/search_api.cpp +++ b/map/search_api.cpp @@ -181,7 +181,7 @@ bool SearchAPI::SearchEverywhere(EverywhereSearchParams const & params) }); if (m_sponsoredMode == SponsoredMode::Booking) - m_delegate.OnBookingAvailabilityParamsUpdate(params.m_bookingFilterParams.m_params); + m_delegate.OnBookingAvailabilityParamsUpdate(params.m_bookingFilterParams.m_apiParams); return Search(p, true /* forceSearch */); } @@ -220,7 +220,7 @@ bool SearchAPI::SearchInViewport(ViewportSearchParams const & params) }); if (m_sponsoredMode == SponsoredMode::Booking) - m_delegate.OnBookingAvailabilityParamsUpdate(params.m_bookingFilterParams.m_params); + m_delegate.OnBookingAvailabilityParamsUpdate(params.m_bookingFilterParams.m_apiParams); return Search(p, false /* forceSearch */); } diff --git a/map/search_api.hpp b/map/search_api.hpp index 001c92c6c4..277ee77d45 100644 --- a/map/search_api.hpp +++ b/map/search_api.hpp @@ -1,7 +1,7 @@ #pragma once -#include "map/bookmark.hpp" #include "map/booking_filter_params.hpp" +#include "map/bookmark.hpp" #include "search/downloader_search_callback.hpp" #include "search/engine.hpp" @@ -80,7 +80,10 @@ public: { } - virtual void OnBookingAvailabilityParamsUpdate(std::shared_ptr const & params) {} + virtual void OnBookingAvailabilityParamsUpdate( + std::shared_ptr const & params) + { + } virtual search::ProductInfo GetProductInfo(search::Result const & result) const { return {}; }; }; diff --git a/partners_api/booking_api.hpp b/partners_api/booking_api.hpp index 6778e1defd..5c4302c3b6 100644 --- a/partners_api/booking_api.hpp +++ b/partners_api/booking_api.hpp @@ -74,7 +74,8 @@ public: std::string GetDescriptionUrl(std::string const & baseUrl) const; std::string GetHotelReviewsUrl(std::string const & hotelId, std::string const & baseUrl) const; std::string GetSearchUrl(std::string const & city, std::string const & name) const; - std::string ApplyAvailabilityParams(std::string const & url, AvailabilityParams const & params) const; + std::string ApplyAvailabilityParams(std::string const & url, + AvailabilityParams const & params) const; /// Real-time information methods (used for retrieving rapidly changing information). /// These methods send requests directly to Booking. diff --git a/partners_api/booking_availability_params.hpp b/partners_api/booking_availability_params.hpp index 5585449a07..1412165a5a 100644 --- a/partners_api/booking_availability_params.hpp +++ b/partners_api/booking_availability_params.hpp @@ -13,7 +13,7 @@ namespace booking { /// Params for checking availability of hotels. /// [m_hotelIds], [m_checkin], [m_checkout], [m_rooms] are required. -struct AvailabilityParams : ParamsBase +struct AvailabilityParams : public ParamsBase { struct Room { diff --git a/partners_api/booking_params_base.hpp b/partners_api/booking_params_base.hpp index a86e3ebb2f..a740197b34 100644 --- a/partners_api/booking_params_base.hpp +++ b/partners_api/booking_params_base.hpp @@ -3,7 +3,6 @@ #include "base/macros.hpp" #include -#include namespace booking {