Merge lp:~mhr3/unity/fix-1067327 into lp:unity

[Michal Hruby]

Ensure categories with non-personal results can show up before empty categories with personal results

Due to a bug in the CategorySorter, lenses with "personal-content" flag set were sorted before lenses without the flag even if there were 0 results. This then caused lenses without the persoanl-content flag set to be displayed below shopping lens, simple example of how it looked after sort:

1) apps.lens - 0 results
2) files.lens - 0 results
3) shopping.lens - 5 results
4) video.lens - 14 (non-personal) results
...

Included unit tests to ensure this doesn't regress.

[Paweł Stołowski (stolowski)]

Looks good, just one suggestion - since you added temporary a_results & b_results:

10 + unsigned a_results = results_per_category_[cat_a];
11 + unsigned b_results = results_per_category_[cat_b];

Can you move them up and simplify the return statement:

17 return results_per_category_[cat_a] > results_per_category_[cat_b];

review: Needs Fixing

[Paweł Stołowski (stolowski)]

Couldn't test it because of a unity trunk - nux trunk compilation issue, but code-wise looks good.

review: Approve

=== modified file 'UnityCore/HomeLens.cpp'
--- UnityCore/HomeLens.cpp	2012-10-01 13:30:18 +0000
+++ UnityCore/HomeLens.cpp	2012-10-16 14:31:20 +0000
@@ -307,13 +307,19 @@
         b_has_personal_content = it->second->provides_personal_content();
       }
 
+      unsigned a_results = results_per_category_[cat_a];
+      unsigned b_results = results_per_category_[cat_b];
+
       // prioritize categories that have private content
       if (a_has_personal_content != b_has_personal_content)
       {
-        return a_has_personal_content ? true : false;
+        // personal content results are first unless there are 0 results
+        if (a_has_personal_content && a_results > 0) return true;
+        else if (b_has_personal_content) return !(b_results > 0);
+        return false;
       }
 
-      return results_per_category_[cat_a] > results_per_category_[cat_b];
+      return a_results > b_results;
     }
 
     private:

=== modified file 'tests/test_home_lens.cpp'
--- tests/test_home_lens.cpp	2012-08-31 11:17:42 +0000
+++ tests/test_home_lens.cpp	2012-10-16 14:31:20 +0000
@@ -63,9 +63,11 @@
            description, search_hint, true, "",
            ModelType::LOCAL)
     , num_results_(-1)
+    , provides_personal_results_(true)
   {
     search_in_global(true);
     connected.SetGetterFunction(sigc::mem_fun(this, &StaticTestLens::force_connected));
+    provides_personal_content.SetGetterFunction(sigc::mem_fun(this, &StaticTestLens::get_provides_personal_results));
 
     DeeModel* cats = categories()->model();
     DeeModel* results = global_results()->model();
@@ -96,6 +98,11 @@
     return true;
   }
 
+  bool get_provides_personal_results()
+  {
+    return provides_personal_results_;
+  }
+
   virtual void DoGlobalSearch(string const& search_string)
   {
     DeeModel* model = global_results()->model();
@@ -165,9 +172,15 @@
     num_results_ = count;
   }
 
+  void SetProvidesPersonalResults(bool value)
+  {
+    provides_personal_results_ = value;
+  }
+
 private:
   string results_base_name_;
   int num_results_;
+  bool provides_personal_results_;
 };
 
 static gboolean dispatch_global_search(gpointer userdata)
@@ -643,4 +656,125 @@
   EXPECT_EQ(order_changed, true);
 }
 
+TEST(TestHomeLens, TestPersonalResultsFirst)
+{
+  HomeLens home_lens_("name", "description", "searchhint",
+                      HomeLens::MergeMode::OWNER_LENS);
+  ThreeStaticTestLenses lenses_;
+  unsigned int lens1_cat = 0;
+  unsigned int lens2_cat = 1;
+  // the lens is added as third, so must have cat == 2
+  unsigned int apps_lens_cat = 2;
+
+  home_lens_.AddLenses(lenses_);
+
+  /*
+   * lens1 -> 1 result
+   * lens2 -> 3 results (no personal content)
+   * lens3 -> 2 results (apps.lens)
+   */
+
+  Lens::Ptr lens = lenses_.GetLensAtIndex(2);
+  auto static_lens = dynamic_pointer_cast<StaticTestLens>(lens);
+  static_lens->SetResultsBaseName("noapes"); // no exact match in apps lens
+  static_lens->SetNumResults(2);
+
+  static_lens = dynamic_pointer_cast<StaticTestLens>(lenses_.GetLensAtIndex(0));
+  static_lens->SetNumResults(1);
+  static_lens = dynamic_pointer_cast<StaticTestLens>(lenses_.GetLensAtIndex(1));
+  static_lens->SetNumResults(3);
+  static_lens->SetProvidesPersonalResults(false);
+
+  bool order_changed = false;
+  home_lens_.categories_reordered.connect([&order_changed] ()
+  {
+    order_changed = true;
+  });
+
+  home_lens_.Search("ape");
+
+  bool finished = false;
+  home_lens_.search_finished.connect([&finished] (Lens::Hints const& hints)
+  {
+    finished = true;
+  });
+  Utils::WaitUntil(finished);
+
+  /* Validate counts */
+  EXPECT_EQ(home_lens_.results()->count(), 6); // 1+3+2 hits
+  EXPECT_EQ(home_lens_.categories()->count(), 3); // 3 cats since we are merging categories by lens
+  EXPECT_EQ(home_lens_.filters()->count(), 0); // We ignore filters deliberately currently
+
+  /* Validate the category order */
+  auto order = home_lens_.GetCategoriesOrder();
+
+  /* The home lens includes applications lens but it doesn't contain exact
+   * match, so can't be the first category */
+  EXPECT_EQ(order.at(0), apps_lens_cat);
+  EXPECT_EQ(order.at(1), lens1_cat);
+  EXPECT_EQ(order.at(2), lens2_cat);
+
+  /* Plus the categories reordered should have been emitted */
+  EXPECT_EQ(order_changed, true);
+}
+
+TEST(TestHomeLens, TestNonPersonalCategoriesBeforeEmpty)
+{
+  HomeLens home_lens_("name", "description", "searchhint",
+                      HomeLens::MergeMode::OWNER_LENS);
+  ThreeStaticTestLenses lenses_;
+  unsigned int lens1_cat = 0;
+  unsigned int lens2_cat = 1;
+  // the lens is added as third, so must have cat == 2
+  unsigned int apps_lens_cat = 2;
+
+  home_lens_.AddLenses(lenses_);
+
+  /*
+   * lens1 -> 1 result
+   * lens2 -> 3 results (no personal content)
+   * lens3 -> 0 results (apps.lens)
+   */
+
+  Lens::Ptr lens = lenses_.GetLensAtIndex(2);
+  auto static_lens = dynamic_pointer_cast<StaticTestLens>(lens);
+  static_lens->SetNumResults(0);
+
+  static_lens = dynamic_pointer_cast<StaticTestLens>(lenses_.GetLensAtIndex(0));
+  static_lens->SetNumResults(1);
+  static_lens = dynamic_pointer_cast<StaticTestLens>(lenses_.GetLensAtIndex(1));
+  static_lens->SetNumResults(3);
+  static_lens->SetProvidesPersonalResults(false);
+
+  bool order_changed = false;
+  home_lens_.categories_reordered.connect([&order_changed] ()
+  {
+    order_changed = true;
+  });
+
+  home_lens_.Search("ape");
+
+  bool finished = false;
+  home_lens_.search_finished.connect([&finished] (Lens::Hints const& hints)
+  {
+    finished = true;
+  });
+  Utils::WaitUntil(finished);
+
+  /* Validate counts */
+  EXPECT_EQ(home_lens_.results()->count(), 4); // 1+3 hits
+  EXPECT_EQ(home_lens_.categories()->count(), 3); // 3 cats since we are merging categories by lens
+  EXPECT_EQ(home_lens_.filters()->count(), 0); // We ignore filters deliberately currently
+
+  /* Validate the category order */
+  auto order = home_lens_.GetCategoriesOrder();
+
+  EXPECT_EQ(order.at(0), lens1_cat);
+  EXPECT_EQ(order.at(1), lens2_cat);
+  EXPECT_EQ(order.at(2), apps_lens_cat);
+
+  /* Plus the categories reordered should have been emitted */
+  EXPECT_EQ(order_changed, true);
+}
+
 }