Merge lp:~jamesh/unity-scope-scopes/remove-duplicate-results into lp:unity-scope-scopes

[James Henstridge]

Fix result de-duplication in the slow path, and update the presentation of results a bit to match the design better.

The scopes scope was not correctly removing duplicate results in the slow path for pushing results. This branch fixes this.

In addition, it makes a few other changes:

1. Don't query for online results when surfacing.

2. When presenting search results, use a different rendering template and present results under a single category.

3. Use the actual author metadata in results.

4. Copy over the department value from online results and use it when activating the result.

[Michal Hruby (mhr3)]

The de-duplication works fine now and search results look more like in the design.

Do we plan to add also the ability to list the same scope multiple times (with different departments)? Is that going to be in this branch, or should we just land this and fix that in another one?

review: Needs Information

[lp:~jamesh/unity-scope-scopes/remove-duplicate-results]

34. By James Henstridge on 2014-04-01

Handle the case where the online scope provides multiple recommendations
for a particular scope.

[James Henstridge (jamesh)]

Okay. It now presents multiple recommendations for the same scope.

It's not perfect though: if the online scope is slow in returning its results and we match a particular scope locally, we won't provide multiple results for it when we do the second query to the online scope.

[lp:~jamesh/unity-scope-scopes/remove-duplicate-results]

35. By James Henstridge on 2014-04-01

Remove unneeded include.

[Michal Hruby (mhr3)]

64 + // don't display ourselves
65 + if (item.scope_id() == "scopes") continue;

Could we drop this? Now that we have the invisible flag it shouldn't be needed.

[lp:~jamesh/unity-scope-scopes/remove-duplicate-results]

36. By James Henstridge on 2014-04-01

There is no need to special case removal of the scopes scope from
results, since it is marked invisible.

[Michal Hruby (mhr3)]

Thanks, +1.

review: Approve

=== modified file 'src/scopes-scope.cpp'
--- src/scopes-scope.cpp	2014-03-19 13:57:54 +0000
+++ src/scopes-scope.cpp	2014-04-01 13:33:22 +0000
@@ -54,6 +54,21 @@
 }
 )";
 // unconfuse emacs: "
+static const char SEARCH_CATEGORY_DEFINITION[] = R"(
+{
+  "schema-version": 1,
+  "template": {
+    "category-layout": "grid",
+    "card-size": "small",
+    "card-layout": "horizontal"
+  },
+  "components": {
+    "title": "title",
+    "subtitle": "author",
+    "mascot":  "icon"
+  }
+}
+)";
 
 int ScopesScope::start(std::string const&, RegistryProxy const &registry) {
     this->registry = registry;
@@ -176,6 +191,14 @@
 }
 
 void ScopesQuery::run(SearchReplyProxy const &reply) {
+    if (query.query_string().empty()) {
+        surfacing_query(reply);
+    } else {
+        search_query(reply);
+    }
+}
+
+void ScopesQuery::surfacing_query(SearchReplyProxy const &reply) {
     CategoryRenderer renderer(SCOPES_CATEGORY_DEFINITION);
     Category::SCPtr categories[N_CATEGORIES];
     categories[CAT_FEATURED] = reply->register_category(
@@ -185,12 +208,41 @@
     categories[CAT_OTHER] = reply->register_category(
         "scopes", "Other", "", renderer);
 
+    MetadataMap all_scopes = scope.registry->list();
+    std::vector<ScopeMetadata> scopes;
+    for (const auto &pair : all_scopes) {
+        const auto &item = pair.second;
+        if (item.invisible())
+            continue;
+        scopes.push_back(item);
+    }
+    std::sort(scopes.begin(), scopes.end(), compareMetadata);
+
+    for (const auto &item : scopes) {
+        // TODO: categorisation of scopes should come from the
+        // metadata rather than being hard coded.
+        Category::SCPtr category;
+        try {
+            ScopeCategory cat_id = category_mapping.at(item.scope_id());
+            category = categories[cat_id];
+        } catch (std::out_of_range &e) {
+            category = categories[CAT_OTHER];
+        }
+        push_scope_result(reply, item, category, nullptr);
+    }
+}
+
+void ScopesQuery::search_query(SearchReplyProxy const &reply) {
+    CategoryRenderer renderer(SEARCH_CATEGORY_DEFINITION);
+    Category::SCPtr category = reply->register_category(
+        "scopes", "Scopes", "", renderer);
+
     std::string term = lowercase(query.query_string());
 
     std::shared_ptr<ResultCollector> online_query;
-    std::map<std::string, std::string> recommended_scopes;
+    std::map<std::string, std::vector<Result>> recommended_scopes;
     std::list<CategorisedResult> online_results;
-    
+
     if (scope.online_scope && !query.query_string().empty()) {
         online_query.reset(new ResultCollector);
         try {
@@ -209,11 +261,7 @@
         if (result.category()->id() == "recommendations") {
             if (result.contains("scope_id")) {
                 auto scope_id = result["scope_id"].get_string();
-                std::string search_string;
-                if (result.contains("search_string")) {
-                    search_string = result["search_string"].get_string();
-                }
-                recommended_scopes[scope_id] = search_string;
+                recommended_scopes[scope_id].push_back(result);
             }
         } else {
             if (!reply->lookup_category(result.category()->id())) {
@@ -252,19 +300,15 @@
         // don't display ourselves
         if (item.scope_id() == "scopes") continue;
 
-        // TODO: categorisation of scopes should come from the
-        // metadata rather than being hard coded.
-        Category::SCPtr category;
-        try {
-            ScopeCategory cat_id = category_mapping.at(item.scope_id());
-            category = categories[cat_id];
-        } catch (std::out_of_range &e) {
-            category = categories[CAT_OTHER];
-        }
+        pushed_scope_ids.insert(item.scope_id());
         auto it = recommended_scopes.find(item.scope_id());
-        pushed_scope_ids.insert(item.scope_id());
-        std::string search_string = it != recommended_scopes.end() ? it->second : "";
-        push_scope_result(reply, item, category, search_string);
+        if (it == recommended_scopes.end()) {
+            push_scope_result(reply, item, category, nullptr);
+        } else {
+            for (auto &online_result : it->second) {
+                push_scope_result(reply, item, category, &online_result);
+            }
+        }
     }
 
     // second round, wait for the server results without any time limits
@@ -278,17 +322,14 @@
                     auto scope_id = result["scope_id"].get_string();
                     if (pushed_scope_ids.find(scope_id) != pushed_scope_ids.end())
                         continue;
-                    std::string search_string;
-                    if (result.contains("search_string")) {
-                        search_string = result["search_string"].get_string();
-                    }
-                    recommended_scopes[scope_id] = search_string;
-                }
-            }
-            if (!reply->lookup_category(result.category()->id())) {
-                reply->register_category(result.category());
-            }
-            reply->push(result);
+                    recommended_scopes[scope_id].push_back(result);
+                }
+            } else {
+                if (!reply->lookup_category(result.category()->id())) {
+                    reply->register_category(result.category());
+                }
+                reply->push(result);
+            }
         }
 
         for (auto const& pair : recommended_scopes) {
@@ -298,22 +339,15 @@
 
             auto const& item = meta_it->second;
             // push more results
-            Category::SCPtr category;
-            try {
-                ScopeCategory cat_id = category_mapping.at(item.scope_id());
-                category = categories[cat_id];
-            } catch (std::out_of_range &e) {
-                category = categories[CAT_OTHER];
-            }
-            auto it = recommended_scopes.find(item.scope_id());
             pushed_scope_ids.insert(item.scope_id());
-            std::string search_string = it != recommended_scopes.end() ? it->second : "";
-            push_scope_result(reply, item, category, search_string);
+            for (auto &online_result : pair.second) {
+                push_scope_result(reply, item, category, &online_result);
+            }
         }
     }
 }
 
-void ScopesQuery::push_scope_result(SearchReplyProxy const &reply, ScopeMetadata const& item, Category::SCPtr const& category, std::string const& search_string)
+void ScopesQuery::push_scope_result(SearchReplyProxy const &reply, ScopeMetadata const& item, Category::SCPtr const& category, Result const *online_result)
 {
     std::string uri = "scope://" + item.scope_id();
     CategorisedResult result(category);
@@ -330,11 +364,23 @@
     } catch (NotFoundException &e) {
     }
     result.set_dnd_uri(uri);
-    result["scope_name"] = item.scope_id();
-    // FIXME: this should be provided by the scope metadata
-    result["author"] = "Canonical Ltd";
+    result["scope_id"] = item.scope_id();
+    result["author"] = item.author();
     result["description"] = item.description();
-    result["search_string"] = search_string;
+    if (online_result != nullptr) {
+        if (online_result->contains("subtitle")) {
+            const auto subtitle = (*online_result)["subtitle"].get_string();
+            if (!subtitle.empty()) {
+                result["author"] = (*online_result)["subtitle"];
+            }
+        }
+        if (online_result->contains("search_string")) {
+            result["search_string"] = (*online_result)["search_string"];
+        }
+        if (online_result->contains("scope_department")) {
+            result["scope_department"] = (*online_result)["scope_department"];
+        }
+    }
     std::string icon;
     try {
         icon = item.icon();
@@ -400,7 +446,17 @@
 }
 
 ActivationResponse ScopesActivation::activate() {
-    CannedQuery query(result["scope_name"].get_string(), result["search_string"].get_string(), "");
+    std::string scope_id = result["scope_id"].get_string();
+    std::string search_string;
+    std::string department_id;
+    if (result.contains("search_string")) {
+        search_string = result["search_string"].get_string();
+    }
+    if (result.contains("scope_department")) {
+        department_id = result["scope_department"].get_string();
+    }
+
+    CannedQuery query(scope_id, search_string, department_id);
     return ActivationResponse(query);
 }
 

=== modified file 'src/scopes-scope.h'
--- src/scopes-scope.h	2014-03-05 10:13:09 +0000
+++ src/scopes-scope.h	2014-04-01 13:33:22 +0000
@@ -51,7 +51,10 @@
     virtual void run(unity::scopes::SearchReplyProxy const&reply) override;
 
 private:
-    void push_scope_result(unity::scopes::SearchReplyProxy const &reply, unity::scopes::ScopeMetadata const& item, unity::scopes::Category::SCPtr const& category, std::string const& search_string);
+    void surfacing_query(unity::scopes::SearchReplyProxy const &reply);
+    void search_query(unity::scopes::SearchReplyProxy const &reply);
+
+    void push_scope_result(unity::scopes::SearchReplyProxy const &reply, unity::scopes::ScopeMetadata const& item, unity::scopes::Category::SCPtr const& category, unity::scopes::Result const *online_result);
 
     const ScopesScope &scope;
     const unity::scopes::CannedQuery query;