Merge pull request #4080 from milchakov/place_page_wifi

[editor] populate FMD_INTERNET from type for MapObjects

9 files changed, 114 insertions, 40 deletions

diff --git a/indexer/editable_map_object.cpp b/indexer/editable_map_object.cpp
index e9f819de2e..e7866b6a12 100644
--- a/indexer/editable_map_object.cpp
+++ b/indexer/editable_map_object.cpp
@@ -30,7 +30,7 @@ bool ExtractName(StringUtf8Multilang const & names, int8_t const langCode,
 
   if (result.end() != it)
     return false;
-  
+
   string name;
   names.GetString(langCode, name);
   result.emplace_back(langCode, name);
@@ -49,7 +49,7 @@ size_t PushMwmLanguages(StringUtf8Multilang const & names, vector<int8_t> const
     if (ExtractName(names, mwmLanguages[i], result))
       ++count;
   }
-  
+
   return count;
 }
 
@@ -141,7 +141,7 @@ NamesDataSource EditableMapObject::GetNamesDataSource(StringUtf8Multilang const
   // Push english name.
   if (ExtractName(source, StringUtf8Multilang::kEnglishCode, names))
     ++mandatoryCount;
-  
+
   // Push user's language.
   if (ExtractName(source, userLangCode, names))
     ++mandatoryCount;
@@ -151,7 +151,7 @@ NamesDataSource EditableMapObject::GetNamesDataSource(StringUtf8Multilang const
     // Exclude default name.
     if (StringUtf8Multilang::kDefaultCode == code)
       return true;
-    
+
     auto const mandatoryNamesEnd = names.begin() + mandatoryCount;
     // Exclude languages which are already in container (languages with top priority).
     auto const it = find_if(
@@ -160,7 +160,7 @@ NamesDataSource EditableMapObject::GetNamesDataSource(StringUtf8Multilang const
 
     if (mandatoryNamesEnd == it)
       names.emplace_back(code, name);
-    
+
     return true;
   });
 
@@ -246,7 +246,7 @@ void EditableMapObject::SetType(uint32_t featureType)
     // TODO(mgsergio): Replace by correct sorting from editor's config.
     copy.SortBySpec();
     m_types.Remove(*copy.begin());
-    m_types.operator ()(featureType);
+    m_types.Add(featureType);
   }
 }
 
@@ -295,6 +295,13 @@ void EditableMapObject::SetWebsite(string website)
 void EditableMapObject::SetInternet(Internet internet)
 {
   m_metadata.Set(feature::Metadata::FMD_INTERNET, DebugPrint(internet));
+
+  static auto const wifiType = classif().GetTypeByPath({"internet_access", "wlan"});
+
+  if (m_types.Has(wifiType) && internet != Internet::Wlan)
+    m_types.Remove(wifiType);
+  else if (!m_types.Has(wifiType) && internet == Internet::Wlan)
+    m_types.Add(wifiType);
 }
 
 void EditableMapObject::SetStars(int stars)
diff --git a/indexer/feature.cpp b/indexer/feature.cpp
index 08ba0f2df0..e9e8e4a1a9 100644
--- a/indexer/feature.cpp
+++ b/indexer/feature.cpp
@@ -142,21 +142,11 @@ editor::XMLFeature FeatureType::ToXML(bool serializeType) const
     feature::TypesHolder th(*this);
     // TODO(mgsergio): Use correct sorting instead of SortBySpec based on the config.
     th.SortBySpec();
-    Classificator & cl = classif();
-    static const uint32_t internetType = cl.GetTypeByPath({"internet_access"});
     // TODO(mgsergio): Either improve "OSM"-compatible serialization for more complex types,
     // or save all our types directly, to restore and reuse them in migration of modified features.
     for (uint32_t const type : th)
     {
-      { // Avoid serialization of "internet" type, it is set separately in the Editor.
-        // Otherwise we can't reset "internet" to "Unknown" state.
-        uint32_t truncatedType = type;
-        ftype::TruncValue(truncatedType, 1);
-        if (truncatedType == internetType)
-          continue;
-      }
-
-      string const strType = cl.GetReadableObjectName(type);
+      string const strType = classif().GetReadableObjectName(type);
       strings::SimpleTokenizer iter(strType, "-");
       string const k = *iter;
       if (++iter)
diff --git a/indexer/feature_data.cpp b/indexer/feature_data.cpp
index b0f0ad9cc5..67e43f3b42 100644
--- a/indexer/feature_data.cpp
+++ b/indexer/feature_data.cpp
@@ -38,7 +38,7 @@ TypesHolder::TypesHolder(FeatureBase const & f)
 {
   f.ForEachType([this](uint32_t type)
   {
-    this->operator()(type);
+    Add(type);
   });
 }
 
diff --git a/indexer/feature_data.hpp b/indexer/feature_data.hpp
index a813403f50..3aae88d45d 100644
--- a/indexer/feature_data.hpp
+++ b/indexer/feature_data.hpp
@@ -66,10 +66,11 @@ namespace feature
     }
 
     /// Accumulation function.
-    inline void operator() (uint32_t type)
+    void Add(uint32_t type)
     {
       ASSERT_LESS(m_size, kMaxTypesCount, ());
-      m_types[m_size++] = type;
+      if (m_size < kMaxTypesCount)
+        m_types[m_size++] = type;
     }
 
     /// @name Selectors.
diff --git a/indexer/ftypes_matcher.cpp b/indexer/ftypes_matcher.cpp
index 20d03af41e..67a1291059 100644
--- a/indexer/ftypes_matcher.cpp
+++ b/indexer/ftypes_matcher.cpp
@@ -146,7 +146,7 @@ IsPeakChecker::IsPeakChecker()
 
 IsPeakChecker const & IsPeakChecker::Instance()
 {
-  static const IsPeakChecker inst;
+  static IsPeakChecker const inst;
   return inst;
 }
 
@@ -158,7 +158,7 @@ IsATMChecker::IsATMChecker()
 
 IsATMChecker const & IsATMChecker::Instance()
 {
-  static const IsATMChecker inst;
+  static IsATMChecker const inst;
   return inst;
 }
 
@@ -171,7 +171,7 @@ IsSpeedCamChecker::IsSpeedCamChecker()
 // static
 IsSpeedCamChecker const & IsSpeedCamChecker::Instance()
 {
-  static const IsSpeedCamChecker instance;
+  static IsSpeedCamChecker const instance;
   return instance;
 }
 
@@ -183,7 +183,7 @@ IsFuelStationChecker::IsFuelStationChecker()
 
 IsFuelStationChecker const & IsFuelStationChecker::Instance()
 {
-  static const IsFuelStationChecker inst;
+  static IsFuelStationChecker const inst;
   return inst;
 }
 
@@ -195,7 +195,7 @@ IsRailwayStationChecker::IsRailwayStationChecker()
 
 IsRailwayStationChecker const & IsRailwayStationChecker::Instance()
 {
-  static const IsRailwayStationChecker inst;
+  static IsRailwayStationChecker const inst;
   return inst;
 }
 
@@ -230,7 +230,7 @@ IsStreetChecker::IsStreetChecker()
 
 IsStreetChecker const & IsStreetChecker::Instance()
 {
-  static const IsStreetChecker inst;
+  static IsStreetChecker const inst;
   return inst;
 }
 
@@ -245,7 +245,7 @@ IsAddressObjectChecker::IsAddressObjectChecker() : BaseChecker(1 /* level */)
 
 IsAddressObjectChecker const & IsAddressObjectChecker::Instance()
 {
-  static const IsAddressObjectChecker inst;
+  static IsAddressObjectChecker const inst;
   return inst;
 }
 
@@ -263,7 +263,7 @@ IsVillageChecker::IsVillageChecker()
 
 IsVillageChecker const & IsVillageChecker::Instance()
 {
-  static const IsVillageChecker inst;
+  static IsVillageChecker const inst;
   return inst;
 }
 
@@ -275,7 +275,7 @@ IsOneWayChecker::IsOneWayChecker()
 
 IsOneWayChecker const & IsOneWayChecker::Instance()
 {
-  static const IsOneWayChecker inst;
+  static IsOneWayChecker const inst;
   return inst;
 }
 
@@ -287,7 +287,7 @@ IsRoundAboutChecker::IsRoundAboutChecker()
 
 IsRoundAboutChecker const & IsRoundAboutChecker::Instance()
 {
-  static const IsRoundAboutChecker inst;
+  static IsRoundAboutChecker const inst;
   return inst;
 }
 
@@ -306,7 +306,7 @@ IsLinkChecker::IsLinkChecker()
 
 IsLinkChecker const & IsLinkChecker::Instance()
 {
-  static const IsLinkChecker inst;
+  static IsLinkChecker const inst;
   return inst;
 }
 
@@ -317,7 +317,7 @@ IsBuildingChecker::IsBuildingChecker() : BaseChecker(1 /* level */)
 
 IsBuildingChecker const & IsBuildingChecker::Instance()
 {
-  static const IsBuildingChecker inst;
+  static IsBuildingChecker const inst;
   return inst;
 }
 
@@ -346,14 +346,14 @@ IsBuildingPartChecker::IsBuildingPartChecker() : BaseChecker(1 /* level */)
 
 IsBuildingPartChecker const & IsBuildingPartChecker::Instance()
 {
-  static const IsBuildingPartChecker inst;
+  static IsBuildingPartChecker const inst;
   return inst;
 }
 
 IsBridgeChecker::IsBridgeChecker() : BaseChecker(3 /* level */) {}
 IsBridgeChecker const & IsBridgeChecker::Instance()
 {
-  static const IsBridgeChecker inst;
+  static IsBridgeChecker const inst;
   return inst;
 }
 
@@ -365,7 +365,7 @@ bool IsBridgeChecker::IsMatched(uint32_t type) const
 IsTunnelChecker::IsTunnelChecker() : BaseChecker(3 /* level */) {}
 IsTunnelChecker const & IsTunnelChecker::Instance()
 {
-  static const IsTunnelChecker inst;
+  static IsTunnelChecker const inst;
   return inst;
 }
 
@@ -421,7 +421,7 @@ IsBookingChecker::IsBookingChecker()
 
 IsBookingChecker const & IsBookingChecker::Instance()
 {
-  static const IsBookingChecker inst;
+  static IsBookingChecker const inst;
   return inst;
 }
 
@@ -434,7 +434,7 @@ IsHotelChecker::IsHotelChecker()
 
 IsHotelChecker const & IsHotelChecker::Instance()
 {
-  static const IsHotelChecker inst;
+  static IsHotelChecker const inst;
   return inst;
 }
 
@@ -445,6 +445,17 @@ vector<string> const & IsHotelChecker::GetHotelTags()
   return hotelTags;
 }
 
+IsWifiChecker::IsWifiChecker()
+{
+  m_types.push_back(classif().GetTypeByPath({"internet_access", "wlan"}));
+}
+
+IsWifiChecker const & IsWifiChecker::Instance()
+{
+  static IsWifiChecker const instance;
+  return instance;
+}
+
 uint32_t GetPopulation(FeatureType const & ft)
 {
   uint32_t population = ft.GetPopulation();
diff --git a/indexer/ftypes_matcher.hpp b/indexer/ftypes_matcher.hpp
index 16e80b813f..5effd53211 100644
--- a/indexer/ftypes_matcher.hpp
+++ b/indexer/ftypes_matcher.hpp
@@ -173,6 +173,16 @@ public:
   static vector<string> const & GetHotelTags();
 };
 
+// WiFi is a type in classificator.txt,
+// it should be checked for filling metadata in MapObject.
+class IsWifiChecker : public BaseChecker
+{
+  IsWifiChecker();
+
+public:
+  static IsWifiChecker const & Instance();
+};
+
 /// Type of locality (do not change values and order - they have detalization order)
 /// COUNTRY < STATE < CITY < ...
 enum Type { NONE = -1, COUNTRY = 0, STATE, CITY, TOWN, VILLAGE, LOCALITY_COUNT };
diff --git a/indexer/indexer_tests/checker_test.cpp b/indexer/indexer_tests/checker_test.cpp
index 703ed22ad0..c08e550817 100644
--- a/indexer/indexer_tests/checker_test.cpp
+++ b/indexer/indexer_tests/checker_test.cpp
@@ -148,14 +148,14 @@ UNIT_TEST(GetHighwayClassTest)
   Classificator const & c = classif();
 
   feature::TypesHolder types1;
-  types1(c.GetTypeByPath({"highway", "motorway_link", "tunnel"}));
+  types1.Add(c.GetTypeByPath({"highway", "motorway_link", "tunnel"}));
   TEST_EQUAL(ftypes::GetHighwayClass(types1), ftypes::HighwayClass::Trunk, ());
 
   feature::TypesHolder types2;
-  types2(c.GetTypeByPath({"highway", "unclassified"}));
+  types2.Add(c.GetTypeByPath({"highway", "unclassified"}));
   TEST_EQUAL(ftypes::GetHighwayClass(types2), ftypes::HighwayClass::LivingStreet, ());
 
   feature::TypesHolder types3;
-  types3(c.GetTypeByPath({"highway"}));
+  types3.Add(c.GetTypeByPath({"highway"}));
   TEST_EQUAL(ftypes::GetHighwayClass(types3), ftypes::HighwayClass::Error, ());
 }
diff --git a/indexer/indexer_tests/editable_map_object_test.cpp b/indexer/indexer_tests/editable_map_object_test.cpp
index 7df4730871..7f0dae3af6 100644
--- a/indexer/indexer_tests/editable_map_object_test.cpp
+++ b/indexer/indexer_tests/editable_map_object_test.cpp
@@ -1,5 +1,7 @@
 #include "testing/testing.hpp"
 
+#include "indexer/classificator.hpp"
+#include "indexer/classificator_loader.hpp"
 #include "indexer/editable_map_object.hpp"
 
 namespace
@@ -244,4 +246,47 @@ UNIT_TEST(EditableMapObject_GetNamesDataSource)
                 "The English language + user`s language. Excluding repetiton"));
   }
 }
+
+UNIT_TEST(EditableMapObject_SetInternet)
+{
+  classificator::Load();
+
+  EditableMapObject emo;
+  auto const wifiType = classif().GetTypeByPath({"internet_access", "wlan"});
+  emo.SetType(wifiType);
+
+  auto types = emo.GetTypes();
+  TEST(types.Has(wifiType), ());
+
+  auto const setInternetAndCheck = [wifiType](EditableMapObject & emo, osm::Internet internet, bool hasWifi)
+  {
+    emo.SetInternet(internet);
+
+    TEST_EQUAL(emo.GetInternet(), internet, ());
+    auto const & types = emo.GetTypes();
+    TEST_EQUAL(types.Has(wifiType), hasWifi, ());
+  };
+
+  setInternetAndCheck(emo, osm::Internet::No, false);
+  setInternetAndCheck(emo, osm::Internet::Yes, false);
+  setInternetAndCheck(emo, osm::Internet::Wired, false);
+  setInternetAndCheck(emo, osm::Internet::Wlan, true);
+  setInternetAndCheck(emo, osm::Internet::Unknown, false);
+
+  EditableMapObject bunkerEmo;
+  bunkerEmo.SetType(classif().GetTypeByPath({"military", "bunker"}));
+  types = bunkerEmo.GetTypes();
+  TEST(!types.Has(wifiType), ());
+
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+  setInternetAndCheck(bunkerEmo, osm::Internet::No, false);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Yes, false);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wired, false);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Unknown, false);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+  setInternetAndCheck(bunkerEmo, osm::Internet::Wlan, true);
+}
 }  // namespace
diff --git a/indexer/map_object.cpp b/indexer/map_object.cpp
index 5f75841144..8ab26b6dea 100644
--- a/indexer/map_object.cpp
+++ b/indexer/map_object.cpp
@@ -20,7 +20,15 @@ constexpr char const * kWlan = "wlan";
 constexpr char const * kWired = "wired";
 constexpr char const * kYes = "yes";
 constexpr char const * kNo = "no";
+
+void SetInetIfNeeded(FeatureType const & ft, feature::Metadata & metadata)
+{
+  if (!ftypes::IsWifiChecker::Instance()(ft) || metadata.Has(feature::Metadata::FMD_INTERNET))
+    return;
+
+  metadata.Set(feature::Metadata::FMD_INTERNET, kWlan);
 }
+}  // namespace
 
 string DebugPrint(osm::Internet internet)
 {
@@ -66,6 +74,8 @@ void MapObject::SetFromFeatureType(FeatureType const & ft)
   m_featureID = ft.GetID();
   ASSERT(m_featureID.IsValid(), ());
   m_geomType = ft.GetFeatureType();
+
+  SetInetIfNeeded(ft, m_metadata);
 }
 
 FeatureID const & MapObject::GetID() const { return m_featureID; }