[search] Some refactoring of ReverseGeocoder routing according to reviews.

15 files changed, 136 insertions, 95 deletions

diff --git a/android/jni/com/mapswithme/maps/Framework.cpp b/android/jni/com/mapswithme/maps/Framework.cpp
index 61f5f3663a..2de146ca9e 100644
--- a/android/jni/com/mapswithme/maps/Framework.cpp
+++ b/android/jni/com/mapswithme/maps/Framework.cpp
@@ -635,7 +635,9 @@ extern "C"
   JNIEXPORT jstring JNICALL
   Java_com_mapswithme_maps_Framework_nativeGetNameAndAddress(JNIEnv * env, jclass clazz, jdouble lat, jdouble lon)
   {
-    search::AddressInfo const info = frm()->GetAddressInfoAtPoint(MercatorBounds::FromLatLon(lat, lon));
+    search::AddressInfo info = frm()->GetAddressInfoAtPoint(MercatorBounds::FromLatLon(lat, lon));
+    if (info.m_distanceMeters > 200.0)
+      info.Clear();
     return jni::ToJavaString(env, info.FormatNameAndAddress());
   }
 
diff --git a/android/jni/com/mapswithme/maps/UserMarkHelper.cpp b/android/jni/com/mapswithme/maps/UserMarkHelper.cpp
index d5c5dc1ca4..a10aa7d11f 100644
--- a/android/jni/com/mapswithme/maps/UserMarkHelper.cpp
+++ b/android/jni/com/mapswithme/maps/UserMarkHelper.cpp
@@ -51,7 +51,7 @@ pair<jintArray, jobjectArray> NativeMetadataToJavaMetadata(JNIEnv * env, Metadat
 void FillAddressAndMetadata(UserMark const * mark, AddressInfo & info, Metadata & metadata)
 {
   Framework * frm = g_framework->NativeFramework();
-  auto const * feature = mark->GetFeature();
+  auto * feature = mark->GetFeature();
   if (feature)
   {
     info = frm->GetFeatureAddressInfo(*feature);
diff --git a/indexer/osm_editor.cpp b/indexer/osm_editor.cpp
index 3c982c7489..fabbb091dc 100644
--- a/indexer/osm_editor.cpp
+++ b/indexer/osm_editor.cpp
@@ -590,9 +590,8 @@ bool Editor::GetEditedFeature(MwmSet::MwmId const & mwmId, uint32_t index, Featu
   return true;
 }
 
-bool Editor::GetEditedFeatureStreet(FeatureType const & feature, string & outFeatureStreet) const
+bool Editor::GetEditedFeatureStreet(FeatureID const & fid, string & outFeatureStreet) const
 {
-  FeatureID const & fid = feature.GetID();
   // TODO(AlexZ): Reuse common code or better make better getters/setters for edited features.
   auto const matchedMwm = m_features.find(fid.m_mwmId);
   if (matchedMwm == m_features.end())
diff --git a/indexer/osm_editor.hpp b/indexer/osm_editor.hpp
index 053fd5080b..67300dc452 100644
--- a/indexer/osm_editor.hpp
+++ b/indexer/osm_editor.hpp
@@ -80,7 +80,7 @@ public:
 
   /// @returns false if feature wasn't edited.
   /// @param outFeatureStreet is valid only if true was returned.
-  bool GetEditedFeatureStreet(FeatureType const & feature, string & outFeatureStreet) const;
+  bool GetEditedFeatureStreet(FeatureID const & fid, string & outFeatureStreet) const;
 
   /// @returns sorted features indices with specified status.
   vector<uint32_t> GetFeaturesByStatus(MwmSet::MwmId const & mwmId, FeatureStatus status) const;
diff --git a/iphone/Maps/Classes/MWMPlacePageEntity.mm b/iphone/Maps/Classes/MWMPlacePageEntity.mm
index ce12f61506..787c7eada0 100644
--- a/iphone/Maps/Classes/MWMPlacePageEntity.mm
+++ b/iphone/Maps/Classes/MWMPlacePageEntity.mm
@@ -217,7 +217,7 @@ void initFieldsMap()
   [self addMetaField:MWMPlacePageCellTypeCoordinate];
 }
 
-- (void)configureWithFeature:(FeatureType const *)feature andCustomName:(NSString *)customName
+- (void)configureWithFeature:(FeatureType *)feature andCustomName:(NSString *)customName
 {
   NSString * emptyName = L(@"dropped_pin");
   // Custom name is used in shared links and should override default feature's name in PP.
@@ -326,7 +326,7 @@ void initFieldsMap()
 
 - (void)processStreets
 {
-  FeatureType const * feature = self.delegate.userMark->GetFeature();
+  FeatureType * feature = self.delegate.userMark->GetFeature();
   if (!feature)
     return;
 
diff --git a/iphone/Maps/Classes/Share/MWMShareLocationActivityItem.mm b/iphone/Maps/Classes/Share/MWMShareLocationActivityItem.mm
index b416547bfb..5ec676cf31 100644
--- a/iphone/Maps/Classes/Share/MWMShareLocationActivityItem.mm
+++ b/iphone/Maps/Classes/Share/MWMShareLocationActivityItem.mm
@@ -85,7 +85,12 @@ NSString * httpGe0Url(NSString * shortUrl)
   NSString * url = [self url:NO];
   if (!self.myPosition)
     return [NSString stringWithFormat:L(@"bookmark_share_email"), self.title, url, httpGe0Url(url)];
-  search::AddressInfo const info = GetFramework().GetAddressInfoAtPoint(MercatorBounds::FromLatLon(self.location.longitude, self.location.latitude));
+
+  search::AddressInfo info = GetFramework().GetAddressInfoAtPoint(
+                                              MercatorBounds::FromLatLon(self.location.latitude, self.location.longitude));
+  if (info.m_distanceMeters > 200.0)
+    info.Clear();
+
   NSString * nameAndAddress = @(info.FormatNameAndAddress().c_str());
   return [NSString stringWithFormat:L(@"my_position_share_email"), nameAndAddress, url, httpGe0Url(url)];
 }
diff --git a/map/address_finder.cpp b/map/address_finder.cpp
index e0d97ed58b..5d8026b180 100644
--- a/map/address_finder.cpp
+++ b/map/address_finder.cpp
@@ -453,28 +453,21 @@ namespace
   }
 }
 */
-search::AddressInfo Framework::GetAddressInfoAtPoint(m2::PointD const & mercator) const
+search::AddressInfo Framework::GetAddressInfoAtPoint(m2::PointD const & pt) const
 {
   search::AddressInfo info;
-  // @TODO(vng): insert correct implementation from new search.
-  //info.m_country = GetCountryName(mercator);
-  // @TODO(vng): Rewrite code to get it from LocalityFinder.
-  //GetLocality(pt, info);
 
-  search::ReverseGeocoder coder(m_model.GetIndex());
+  search::ReverseGeocoder const coder(m_model.GetIndex());
   search::ReverseGeocoder::Address addr;
-  coder.GetNearbyAddress(mercator, addr);
+  coder.GetNearbyAddress(pt, addr);
+  info.m_house = addr.GetHouseNumber();
+  info.m_street = addr.GetStreetName();
+  info.m_distanceMeters = addr.GetDistance();
 
-  // Limit distance to nearest address with 200 meters.
-  if (addr.GetDistance() < 200.0)
-  {
-    info.m_house = addr.GetHouseNumber();
-    info.m_street = addr.GetStreetName();
-  }
   return info;
 }
 
-search::AddressInfo Framework::GetFeatureAddressInfo(FeatureType const & ft) const
+search::AddressInfo Framework::GetFeatureAddressInfo(FeatureType & ft) const
 {
   search::AddressInfo info;
   // @TODO(vng): insert correct implementation from new search.
@@ -482,15 +475,11 @@ search::AddressInfo Framework::GetFeatureAddressInfo(FeatureType const & ft) con
   // @TODO(vng): Temporarily commented - it's slow and not used in UI.
   //GetLocality(pt, info);
 
-  info.m_house = ft.GetHouseNumber();
-  // Street can be added/edited by user in the Editor.
-  if (!osm::Editor::Instance().GetEditedFeatureStreet(ft, info.m_street))
-  {
-    search::ReverseGeocoder const coder(m_model.GetIndex());
-    auto const streets = coder.GetNearbyFeatureStreets(ft);
-    if (streets.second < streets.first.size())
-      info.m_street = streets.first[streets.second].m_name;
-  }
+  search::ReverseGeocoder const coder(m_model.GetIndex());
+  search::ReverseGeocoder::Address addr;
+  coder.GetNearbyAddress(ft, addr);
+  info.m_house = addr.GetHouseNumber();
+  info.m_street = addr.GetStreetName();
 
   // TODO(vng): Why AddressInfo is responsible for types and names? Refactor out.
   string defaultName, intName;
diff --git a/map/framework.cpp b/map/framework.cpp
index 3f93e5234c..3fadad8d32 100644
--- a/map/framework.cpp
+++ b/map/framework.cpp
@@ -1311,8 +1311,8 @@ search::AddressInfo Framework::GetSearchResultAddress(search::Result const & res
   if (res.IsSuggest())
     return info;
 
-  /// @todo Optimize this stuff according to the facct, that feature is
-  /// already reading in many cases during process search result.
+  /// @todo Optimize this stuff according to the fact that feature is
+  /// already reading in many cases during search results processing.
   auto const & id = res.GetFeatureID();
   if (id.IsValid())
   {
@@ -1326,7 +1326,9 @@ search::AddressInfo Framework::GetSearchResultAddress(search::Result const & res
     }
   }
 
-  info = GetMercatorAddressInfo(res.GetFeatureCenter());
+  info = GetAddressInfoAtPoint(res.GetFeatureCenter());
+  if (info.m_distanceMeters > 50.0)
+    info.Clear();
 
   string const & type = res.GetFeatureType();
   if (!type.empty())
diff --git a/map/framework.hpp b/map/framework.hpp
index 5a228b4a2a..191665f33c 100644
--- a/map/framework.hpp
+++ b/map/framework.hpp
@@ -483,9 +483,9 @@ private:
   bool ParseEditorDebugCommand(search::SearchParams const & params);
 public:
   /// @returns address of nearby building with house number in approx 1km distance.
-  search::AddressInfo GetAddressInfoAtPoint(m2::PointD const & mercator) const;
+  search::AddressInfo GetAddressInfoAtPoint(m2::PointD const & pt) const;
   /// @returns valid street address only if it was specified in OSM for given feature; used in the editor.
-  search::AddressInfo GetFeatureAddressInfo(FeatureType const & ft) const;
+  search::AddressInfo GetFeatureAddressInfo(FeatureType & ft) const;
   vector<string> GetPrintableFeatureTypes(FeatureType const & ft) const;
   /// If feature does not have explicit street in OSM data, first value can be a closest named street.
   /// If it does have explicit street name in OSM, it goes first in the returned vector.
diff --git a/qt/editor_dialog.cpp b/qt/editor_dialog.cpp
index f0f5571e06..b6add86099 100644
--- a/qt/editor_dialog.cpp
+++ b/qt/editor_dialog.cpp
@@ -29,7 +29,7 @@ using feature::Metadata;
 constexpr char const * kStreetObjectName = "addr:street";
 constexpr char const * kHouseNumberObjectName = "addr:housenumber";
 
-EditorDialog::EditorDialog(QWidget * parent, FeatureType const & feature, Framework & frm) : QDialog(parent)
+EditorDialog::EditorDialog(QWidget * parent, FeatureType & feature, Framework & frm) : QDialog(parent)
 {
   osm::Editor & editor = osm::Editor::Instance();
 
diff --git a/qt/editor_dialog.hpp b/qt/editor_dialog.hpp
index 964467065f..040522d1d5 100644
--- a/qt/editor_dialog.hpp
+++ b/qt/editor_dialog.hpp
@@ -16,7 +16,7 @@ class EditorDialog : public QDialog
 {
   Q_OBJECT
 public:
-  EditorDialog(QWidget * parent, FeatureType const & feature, Framework & frm);
+  EditorDialog(QWidget * parent, FeatureType & feature, Framework & frm);
   StringUtf8Multilang GetEditedNames() const;
   feature::Metadata GetEditedMetadata() const;
   string GetEditedStreet() const;
diff --git a/search/result.hpp b/search/result.hpp
index 54b44ef4c4..872056695f 100644
--- a/search/result.hpp
+++ b/search/result.hpp
@@ -185,6 +185,7 @@ struct AddressInfo
 {
   string m_country, m_city, m_street, m_house, m_name;
   vector<string> m_types;
+  double m_distanceMeters = -1.0;
 
   string GetPinName() const;    // Caroline
   string GetPinType() const;    // shop
diff --git a/search/reverse_geocoder.cpp b/search/reverse_geocoder.cpp
index d011c1bfe2..2c795d89a4 100644
--- a/search/reverse_geocoder.cpp
+++ b/search/reverse_geocoder.cpp
@@ -101,84 +101,74 @@ size_t ReverseGeocoder::GetMatchedStreetIndex(string const & keyName,
   return result;
 }
 
+pair<vector<ReverseGeocoder::Street>, uint32_t>
+ReverseGeocoder::GetNearbyFeatureStreets(FeatureType const & ft) const
+{
+  pair<vector<ReverseGeocoder::Street>, uint32_t> result;
+
+  GetNearbyStreets(const_cast<FeatureType &>(ft), result.first);
+
+  HouseTable table;
+  if (!table.Get(m_index, ft.GetID(), result.first, result.second))
+    result.second = numeric_limits<uint32_t>::max();
+
+  return result;
+}
+
 void ReverseGeocoder::GetNearbyAddress(m2::PointD const & center, Address & addr) const
 {
   vector<Building> buildings;
   GetNearbyBuildings(center, buildings);
 
-  vector<Street> streets;
-  unique_ptr<search::v2::HouseToStreetTable> table;
-  MwmSet::MwmHandle mwmHandle;
-
+  HouseTable table;
   int triesCount = 0;
+
   for (auto const & b : buildings)
   {
-    if (!table || mwmHandle.GetId() != b.m_id.m_mwmId)
-    {
-      mwmHandle = m_index.GetMwmHandleById(b.m_id.m_mwmId);
-      if (!mwmHandle.IsAlive())
-        continue;
-      table = search::v2::HouseToStreetTable::Load(*mwmHandle.GetValue<MwmValue>());
-    }
-
-    GetNearbyStreets(b.m_id.m_mwmId, b.m_center, streets);
-
-    uint32_t ind;
-    if (table->Get(b.m_id.m_index, ind) && ind < streets.size())
-    {
-      addr.m_building = b;
-      addr.m_street = streets[ind];
-      return;
-    }
-
-    // Do not analyze more than 5 houses to get exact address.
-    if (++triesCount == 5)
-      return;
+    if (GetNearbyAddress(table, b, addr) || (++triesCount == 5))
+      break;
   }
 }
 
-pair<vector<ReverseGeocoder::Street>, uint32_t>
-ReverseGeocoder::GetNearbyFeatureStreets(FeatureType const & feature) const
+void ReverseGeocoder::GetNearbyAddress(FeatureType & ft, Address & addr) const
 {
-  pair<vector<ReverseGeocoder::Street>, uint32_t> result;
-  auto & streetIndex = result.second;
-  streetIndex = numeric_limits<uint32_t>::max();
+  HouseTable table;
+  (void)GetNearbyAddress(table, FromFeature(ft, 0.0 /* distMeters */), addr);
+}
 
-  FeatureID const fid = feature.GetID();
-  MwmSet::MwmHandle const mwmHandle = m_index.GetMwmHandleById(fid.m_mwmId);
-  if (!mwmHandle.IsAlive())
+bool ReverseGeocoder::GetNearbyAddress(HouseTable & table, Building const & bld,
+                                       Address & addr) const
+{
+  string street;
+  if (osm::Editor::Instance().GetEditedFeatureStreet(bld.m_id, street))
   {
-    LOG(LWARNING, ("MWM for", feature, "is dead"));
-    return result;
+    addr.m_building = bld;
+    addr.m_street.m_name = street;
+    return true;
   }
 
-  auto & streets = result.first;
-  GetNearbyStreets(const_cast<FeatureType &>(feature), streets);
+  vector<Street> streets;
+  GetNearbyStreets(bld.m_id.m_mwmId, bld.m_center, streets);
 
-  unique_ptr<search::v2::HouseToStreetTable> const table =
-      search::v2::HouseToStreetTable::Load(*mwmHandle.GetValue<MwmValue>());
+  uint32_t ind;
+  if (table.Get(m_index, bld.m_id, streets, ind))
+  {
+    addr.m_building = bld;
+    addr.m_street = streets[ind];
+    return true;
+  }
 
-  if (table->Get(fid.m_index, streetIndex) && streetIndex >= streets.size())
-    LOG(LWARNING, ("Out of bound index", streetIndex, "for", feature));
-  return result;
+  return false;
 }
 
 void ReverseGeocoder::GetNearbyBuildings(m2::PointD const & center, vector<Building> & buildings) const
 {
   m2::RectD const rect = GetLookupRect(center, kLookupRadiusM);
 
-  auto const addBuilding = [&](FeatureType const & ft)
+  auto const addBuilding = [&](FeatureType & ft)
   {
-    if (!ftypes::IsBuildingChecker::Instance()(ft))
-      return;
-
-    // Skip empty house numbers.
-    string const number = ft.GetHouseNumber();
-    if (number.empty())
-      return;
-
-    buildings.push_back({ft.GetID(), feature::GetMinDistanceMeters(ft, center),
-                         number, feature::GetCenter(ft)});
+    if (!ft.GetHouseNumber().empty())
+      buildings.push_back(FromFeature(ft, feature::GetMinDistanceMeters(ft, center)));
   };
 
   m_index.ForEachInRect(addBuilding, rect, kQueryScale);
@@ -186,9 +176,41 @@ void ReverseGeocoder::GetNearbyBuildings(m2::PointD const & center, vector<Build
 }
 
 // static
+ReverseGeocoder::Building ReverseGeocoder::FromFeature(FeatureType & ft, double distMeters)
+{
+  return { ft.GetID(), distMeters, ft.GetHouseNumber(), feature::GetCenter(ft) };
+}
+
+// static
 m2::RectD ReverseGeocoder::GetLookupRect(m2::PointD const & center, double radiusM)
 {
   return MercatorBounds::RectByCenterXYAndSizeInMeters(center, radiusM);
 }
 
+bool ReverseGeocoder::HouseTable::Get(Index const & index, FeatureID fId,
+                                      vector<Street> const & streets, uint32_t & stIndex)
+{
+  if (!m_table || m_mwmHandle.GetId() != fId.m_mwmId)
+  {
+    m_mwmHandle = index.GetMwmHandleById(fId.m_mwmId);
+    if (!m_mwmHandle.IsAlive())
+    {
+      LOG(LWARNING, ("MWM", fId, "is dead"));
+      return false;
+    }
+    m_table = search::v2::HouseToStreetTable::Load(*m_mwmHandle.GetValue<MwmValue>());
+  }
+
+  if (!m_table->Get(fId.m_index, stIndex))
+    return false;
+
+  if (stIndex >= streets.size())
+  {
+    LOG(LWARNING, ("Out of bound index", stIndex, "for", fId));
+    return false;
+  }
+
+  return true;
+}
+
 } // namespace search
diff --git a/search/reverse_geocoder.hpp b/search/reverse_geocoder.hpp
index 1a7ce0dde1..ee1157b274 100644
--- a/search/reverse_geocoder.hpp
+++ b/search/reverse_geocoder.hpp
@@ -12,6 +12,10 @@ class Index;
 
 namespace search
 {
+namespace v2
+{
+class HouseToStreetTable;
+}
 
 class ReverseGeocoder
 {
@@ -23,7 +27,7 @@ class ReverseGeocoder
     double m_distanceMeters;
     string m_name;
 
-    Object() : m_distanceMeters(0.0) {}
+    Object() : m_distanceMeters(-1.0) {}
     Object(FeatureID const & id, double dist, string const & name)
       : m_id(id), m_distanceMeters(dist), m_name(name)
     {
@@ -75,15 +79,32 @@ public:
 
   /// @todo Leave const reference for now to support client's legacy code.
   /// It's better to use honest non-const reference when feature can be modified in any way.
-  pair<vector<Street>, uint32_t> GetNearbyFeatureStreets(FeatureType const & feature) const;
+  pair<vector<Street>, uint32_t> GetNearbyFeatureStreets(FeatureType const & ft) const;
 
   /// @return The nearest exact address where building has house number and valid street match.
   void GetNearbyAddress(m2::PointD const & center, Address & addr) const;
+  /// @return The exact address for feature.
+  /// @precondition ft Should have house number.
+  void GetNearbyAddress(FeatureType & ft, Address & addr) const;
+
+private:
+
+  /// Helper class to incapsulate house 2 street table reloading.
+  class HouseTable
+  {
+    unique_ptr<search::v2::HouseToStreetTable> m_table;
+    MwmSet::MwmHandle m_mwmHandle;
+  public:
+    bool Get(Index const & index, FeatureID fId,
+             vector<Street> const & streets, uint32_t & stIndex);
+  };
+
+  bool GetNearbyAddress(HouseTable & table, Building const & bld, Address & addr) const;
 
   /// @return Sorted by distance houses vector with valid house number.
   void GetNearbyBuildings(m2::PointD const & center, vector<Building> & buildings) const;
 
-private:
+  static Building FromFeature(FeatureType & ft, double distMeters);
   static m2::RectD GetLookupRect(m2::PointD const & center, double radiusM);
 };
 
diff --git a/search/v2/geocoder.cpp b/search/v2/geocoder.cpp
index f8fb2048a3..8240d59ef1 100644
--- a/search/v2/geocoder.cpp
+++ b/search/v2/geocoder.cpp
@@ -1247,7 +1247,7 @@ void Geocoder::MatchUnclassified(size_t curToken)
   auto emitUnclassified = [&](uint32_t featureId)
   {
     if (GetSearchTypeInGeocoding(featureId) == SearchModel::SEARCH_TYPE_UNCLASSIFIED)
-      m_results->emplace_back(m_context->m_id, featureId);
+      m_results->emplace_back(m_context->GetId(), featureId);
   };
   coding::CompressedBitVectorEnumerator::ForEach(*allFeatures, emitUnclassified);
 }