[search] Fixed possible bug when canceling request.

Signed-off-by: Viktor Govako <viktor.govako@gmail.com>

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();

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 */);

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);

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<size_t>(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)
     {

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();
 

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)

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);

search/search_tests/results_tests.cpp

@@ -2,18 +2,24 @@
 
 #include "search/result.hpp"
 
+#include "indexer/data_source.hpp"
+
 #include <iterator>
 
-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