Merge lp:~stolowski/unity-scope-home/preview-on-lmb into lp:unity-scope-home

[Paweł Stołowski]

Display preview for More Suggestions also on left-mouse-click.

Display preview for More Suggestions also on left-mouse-click.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:96
http://jenkins.qa.ubuntu.com/job/unity-scope-home-ci/19/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-home-raring-amd64-ci/21

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-home-ci/19/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

9 + internal async void handle_metrics (uint action_type, string scope_id, string session_id, string server_sid)
10 {
11 + if (server_sid == null)
12 + return;

You don't allow server_sid to be null ("string server_sid", no "string? server_sid"), so the check is useless.

55 + (activation.action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT ||
56 + activation.action_type == Unity.Protocol.ActionType.PREVIEW_RESULT))

Shouldn't this check if the scope is really remote? Or is that check done earlier?

review: Needs Fixing

[lp:~stolowski/unity-scope-home/preview-on-lmb]

97. By Paweł Stołowski on 2013-04-23

Fixed server_sid check.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:97
http://jenkins.qa.ubuntu.com/job/unity-scope-home-ci/20/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-home-raring-amd64-ci/22

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-home-ci/20/rebuild

review: Approve (continuous-integration)

[lp:~stolowski/unity-scope-home/preview-on-lmb]

98. By Paweł Stołowski on 2013-04-23

Make sure more_suggestions is a remote scope when handling preview for it.

[Paweł Stołowski (stolowski)]

> 9 + internal async void handle_metrics (uint action_type, string
> scope_id, string session_id, string server_sid)
> 10 {
> 11 + if (server_sid == null)
> 12 + return;
>
> You don't allow server_sid to be null ("string server_sid", no "string?
> server_sid"), so the check is useless.

Right. Fixed.

>
> 55 + (activation.action_type ==
> Unity.Protocol.ActionType.ACTIVATE_RESULT ||
> 56 + activation.action_type ==
> Unity.Protocol.ActionType.PREVIEW_RESULT))
>
> Shouldn't this check if the scope is really remote? Or is that check done
> earlier?
We don't really have local "more_suggestions-*" scopes but you're right. Added an extra check just to be safe.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:98
http://jenkins.qa.ubuntu.com/job/unity-scope-home-ci/21/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-home-raring-amd64-ci/23

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-scope-home-ci/21/rebuild

review: Approve (continuous-integration)

[Michal Hruby (mhr3)]

Looks reasonable, I don't think the "yield handle_metrics ();" should be right after "yield handle_preview ()", but as we discussed this will be fixed in another branch.

review: Approve

=== modified file 'src/scope.vala'
--- src/scope.vala	2013-04-22 12:28:51 +0000
+++ src/scope.vala	2013-04-23 17:15:30 +0000
@@ -381,15 +381,20 @@
     return true;
   }
 
-  internal async void handle_metrics (Unity.AggregatorActivation activation, string scope_id, string session_id, string server_sid)
+  internal async void handle_metrics (uint action_type, string scope_id, string session_id, string? server_sid)
   {
+    // server_sid may be null if we didn't query smart scopes service
+    if (server_sid == null)
+      return;
+
+    debug ("Adding activation metrics record for scope %s, action_type=%u", scope_id, action_type);
     var timestamp = new DateTime.now_utc ();
 
-    if (activation.action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT)
+    if (action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT)
     {
       sss_client.add_click_event (session_id, server_sid, scope_id, timestamp);
     }
-    else if (activation.action_type == Unity.Protocol.ActionType.PREVIEW_RESULT)
+    else if (action_type == Unity.Protocol.ActionType.PREVIEW_RESULT)
     {
       sss_client.add_preview_event (session_id, server_sid, scope_id, timestamp);
     }
@@ -471,7 +476,9 @@
   {
     debug ("Activation request for scope %s, action_type=%u", activation.scope_id, activation.action_type);
 
-    if (!scope_mgr.remote_content_search || sss_client == null)
+    if (!scope_mgr.remote_content_search || sss_client == null ||
+        (activation.action_type != Unity.Protocol.ActionType.ACTIVATE_RESULT &&
+         activation.action_type != Unity.Protocol.ActionType.PREVIEW_RESULT))
       return null; //nothing to do
  
     string? server_sid = channel_id_map.server_sid_for_channel (activation.channel_id);
@@ -480,24 +487,28 @@
     var scope_id = activation.scope_id; //this is an id of master (or a standalone scope such as apps)
     var scope_id_var = activation.scope_result.metadata.lookup ("scope-id");
 
-    // server_sid may be null if we didn't query smart scopes service
-    if (server_sid != null)
+    if (scope_id_var != null)
+      scope_id = scope_id_var.get_string ();
+    else
+      debug ("No specific scope_id in the result from master '%s'", scope_id);
+
+    bool is_local_scope = scope_mgr.is_client_scope (scope_id) || ScopeRegistry.instance ().is_master (scope_id);
+
+    // special case for more suggestions in home lens: both lmb & rmb should display a preview
+    if (scope_id.has_prefix ("more_suggestions-") &&
+        !is_local_scope &&
+        (activation.action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT ||
+         activation.action_type == Unity.Protocol.ActionType.PREVIEW_RESULT))
     {
-      if (scope_id_var != null)
-      {
-        scope_id = scope_id_var.get_string ();
-        debug ("Adding activation metrics record for scope %s", scope_id);
-      }
-      else
-      {
-        debug ("No specific scope_id in the result from master '%s'", scope_id);
-      }
-  
-      yield handle_metrics (activation, scope_id, session_id, server_sid);
+      var preview = yield handle_preview (activation, session_id, server_sid);
+      yield handle_metrics (Unity.Protocol.ActionType.PREVIEW_RESULT, scope_id, session_id, server_sid);
+      return preview;
     }
 
+    yield handle_metrics (activation.action_type, scope_id, session_id, server_sid);
+
     // do nothing for local scopes - activation will be handled by actual scope
-    if (scope_mgr.is_client_scope (scope_id) || ScopeRegistry.instance ().is_master (scope_id))
+    if (is_local_scope)
     {
       debug ("Scope %s is a local scope, passing request to it", scope_id);
       return null;