From 3d481801c1de0830d56def278385fa3bd6434bdb Mon Sep 17 00:00:00 2001 From: Arsentiy Milchakov Date: Fri, 26 Oct 2018 14:41:27 +0300 Subject: [PATCH] [eye] review fixes --- coding/serdes_json.hpp | 9 +- map/framework.cpp | 12 +-- metrics/eye.cpp | 93 ++++++++++++------- metrics/eye.hpp | 21 +++-- metrics/eye_info.hpp | 14 ++- metrics/eye_storage.cpp | 5 +- .../metrics_tests_support/eye_for_testing.hpp | 5 + 7 files changed, 101 insertions(+), 58 deletions(-) diff --git a/coding/serdes_json.hpp b/coding/serdes_json.hpp index cedcd56acf..65e4565027 100644 --- a/coding/serdes_json.hpp +++ b/coding/serdes_json.hpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include @@ -228,7 +229,7 @@ public: RestoreContext(outerContext); } - template + template > void operator()(std::unordered_set & dest, char const * name = nullptr) { json_t * outerContext = SaveContext(name); @@ -251,12 +252,6 @@ public: RestoreContext(outerContext); } - template - void operator()(std::unordered_set & dest, char const * name = nullptr) - { - (*this)::key_type, std::unordered_set::hasher>(dest, name); - } - template void operator()(std::array & dst, char const * name = nullptr) { diff --git a/map/framework.cpp b/map/framework.cpp index dd85e74c4c..7c09f94624 100644 --- a/map/framework.cpp +++ b/map/framework.cpp @@ -30,6 +30,7 @@ #include "search/locality_finder.hpp" #include "search/reverse_geocoder.hpp" +#include "storage/country_info_getter.hpp" #include "storage/downloader_search_params.hpp" #include "storage/routing_helpers.hpp" #include "storage/storage_helpers.hpp" @@ -49,19 +50,15 @@ #include "indexer/drawing_rules.hpp" #include "indexer/editable_map_object.hpp" #include "indexer/feature.hpp" +#include "indexer/feature_algo.hpp" #include "indexer/feature_source.hpp" +#include "indexer/feature_utils.hpp" #include "indexer/feature_visibility.hpp" #include "indexer/ftypes_sponsored.hpp" #include "indexer/map_style_reader.hpp" #include "indexer/scales.hpp" -/// @todo Probably it's better to join this functionality. -//@{ -#include "indexer/feature_algo.hpp" -#include "indexer/feature_utils.hpp" -//@} - -#include "storage/country_info_getter.hpp" +#include "metrics/eye.hpp" #include "platform/local_country_file_utils.hpp" #include "platform/measurement_utils.hpp" @@ -536,6 +533,7 @@ Framework::~Framework() m_user.ClearSubscribers(); // Must be destroyed implicitly, since it stores reference to m_user. m_bmManager.reset(); + eye::Eye::Instance().UnsubscribeAll(); } booking::Api * Framework::GetBookingApi(platform::NetworkPolicy const & policy) diff --git a/metrics/eye.cpp b/metrics/eye.cpp index 882b07f2fa..909ec5b98d 100644 --- a/metrics/eye.cpp +++ b/metrics/eye.cpp @@ -90,16 +90,21 @@ Eye::InfoType Eye::GetInfo() const void Eye::Subscribe(Subscriber * subscriber) { - GetPlatform().RunTask(Platform::Thread::File, [this, subscriber] - { - m_subscribers.push_back(subscriber); - }); + m_subscribers.push_back(subscriber); } -void Eye::Save(InfoType const & info) +void Eye::UnsubscribeAll() { - if (::Save(*info)) - m_info.Set(info); + m_subscribers.clear(); +} + +bool Eye::Save(InfoType const & info) +{ + if (!::Save(*info)) + return false; + + m_info.Set(info); + return true; } void Eye::RegisterTipClick(Tip::Type type, Tip::Event event) @@ -126,15 +131,19 @@ void Eye::RegisterTipClick(Tip::Type type, Tip::Event event) tip.m_type = type; tip.m_eventCounters.Increment(event); tip.m_lastShownTime = now; - editableTips.emplace_back(tip); + editableTips.push_back(tip); } - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, tip] { - subscriber->OnTipClicked(tip); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnTipClicked(tip); + } + }); } void Eye::UpdateBookingFilterUsedTime() @@ -145,12 +154,16 @@ void Eye::UpdateBookingFilterUsedTime() editableInfo->m_booking.m_lastFilterUsedTime = now; - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, now] { - subscriber->OnBookingFilterUsed(now); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnBookingFilterUsed(now); + } + }); } void Eye::UpdateBoomarksCatalogShownTime() @@ -161,12 +174,16 @@ void Eye::UpdateBoomarksCatalogShownTime() editableInfo->m_bookmarks.m_lastOpenedTime = now; - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, now] { - subscriber->OnBookmarksCatalogShown(now); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnBookmarksCatalogShown(now); + } + }); } void Eye::UpdateDiscoveryShownTime() @@ -177,12 +194,16 @@ void Eye::UpdateDiscoveryShownTime() editableInfo->m_discovery.m_lastOpenedTime = now; - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, now] { - subscriber->OnDiscoveryShown(now); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnDiscoveryShown(now); + } + }); } void Eye::IncrementDiscoveryItem(Discovery::Event event) @@ -193,12 +214,16 @@ void Eye::IncrementDiscoveryItem(Discovery::Event event) editableInfo->m_discovery.m_lastClickedTime = Clock::now(); editableInfo->m_discovery.m_eventCounters.Increment(event); - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, event] { - subscriber->OnDiscoveryItemClicked(event); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnDiscoveryItemClicked(event); + } + }); } void Eye::RegisterLayerShown(Layer::Type type) @@ -228,12 +253,16 @@ void Eye::RegisterLayerShown(Layer::Type type) editableLayers.emplace_back(layer); } - Save(editableInfo); + if (!Save(editableInfo)) + return; - for (auto subscriber : m_subscribers) + GetPlatform().RunTask(Platform::Thread::Gui, [this, layer] { - subscriber->OnLayerUsed(layer); - } + for (auto subscriber : m_subscribers) + { + subscriber->OnLayerShown(layer); + } + }); } void Eye::RegisterPlacePageOpened() diff --git a/metrics/eye.hpp b/metrics/eye.hpp index 97c9b88d65..3ca68ef44b 100644 --- a/metrics/eye.hpp +++ b/metrics/eye.hpp @@ -12,16 +12,16 @@ namespace eye class Subscriber { public: - virtual ~Subscriber(){}; + virtual ~Subscriber() = default; public: - virtual void OnTipClicked(Tip const & tip){}; - virtual void OnBookingFilterUsed(Time const & time){}; - virtual void OnBookmarksCatalogShown(Time const & time){}; - virtual void OnDiscoveryShown(Time const & time){}; - virtual void OnDiscoveryItemClicked(Discovery::Event event){}; - virtual void OnLayerUsed(Layer const & layer){}; - virtual void OnPlacePageOpened(MapObject const & poi){}; + virtual void OnTipClicked(Tip const & tip) {} + virtual void OnBookingFilterUsed(Time const & time) {} + virtual void OnBookmarksCatalogShown(Time const & time) {} + virtual void OnDiscoveryShown(Time const & time) {} + virtual void OnDiscoveryItemClicked(Discovery::Event event) {} + virtual void OnLayerShown(Layer const & layer) {} + virtual void OnPlacePageOpened(MapObject const & poi) {} }; // Note This class IS thread-safe. @@ -53,12 +53,15 @@ public: static Eye & Instance(); InfoType GetInfo() const; + + // Subscribe/Unsubscribe must be called from main thread only. void Subscribe(Subscriber * subscriber); + void UnsubscribeAll(); private: Eye(); - void Save(InfoType const & info); + bool Save(InfoType const & info); // Event processing: void RegisterTipClick(Tip::Type type, Tip::Event event); diff --git a/metrics/eye_info.hpp b/metrics/eye_info.hpp index 2abe03537d..fbbbc069ac 100644 --- a/metrics/eye_info.hpp +++ b/metrics/eye_info.hpp @@ -164,7 +164,7 @@ struct MapObject { struct Event { - enum Type + enum class Type : uint8_t { Open, AddToBookmark, @@ -248,4 +248,16 @@ inline std::string DebugPrint(Layer::Type const & type) case Layer::Type::PublicTransport: return "PublicTransport"; } } + +inline std::string DebugPrint(MapObject::Event::Type const & type) +{ + switch (type) + { + case MapObject::Event::Type::Open: return "Open"; + case MapObject::Event::Type::AddToBookmark: return "AddToBookmark"; + case MapObject::Event::Type::UgcEditorOpened: return "UgcEditorOpened"; + case MapObject::Event::Type::UgcSaved: return "UgcSaved"; + case MapObject::Event::Type::RouteToCreated: return "RouteToCreated"; + } +} } // namespace eye diff --git a/metrics/eye_storage.cpp b/metrics/eye_storage.cpp index 499b608dde..e46b44c74b 100644 --- a/metrics/eye_storage.cpp +++ b/metrics/eye_storage.cpp @@ -22,8 +22,9 @@ bool Save(std::string const & filename, std::vector const & src) FileWriter writer(fileName); writer.Write(src.data(), src.size()); } - catch (FileWriter::Exception const &) + catch (FileWriter::Exception const & ex) { + LOG(LERROR, (ex.what(), ex.Msg())); return false; } @@ -42,7 +43,7 @@ bool Load(std::string const & filename, std::vector & dst) reader.Read(0, dst.data(), dst.size()); } - catch (FileReader::Exception const & ex) + catch (FileReader::Exception const &) { dst.clear(); return false; diff --git a/metrics/metrics_tests_support/eye_for_testing.hpp b/metrics/metrics_tests_support/eye_for_testing.hpp index 87795d9692..cf171eb523 100644 --- a/metrics/metrics_tests_support/eye_for_testing.hpp +++ b/metrics/metrics_tests_support/eye_for_testing.hpp @@ -2,6 +2,8 @@ #include "metrics/eye_info.hpp" +#include "platform/platform.hpp" + namespace eye { class EyeForTesting @@ -22,5 +24,8 @@ class ScopedEyeForTesting public: ScopedEyeForTesting() { EyeForTesting::ResetEye(); } ~ScopedEyeForTesting() { EyeForTesting::ResetEye(); } + +private: + Platform::ThreadRunner m_runner; }; } // namespace eye