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

Proposed by Paweł Stołowski on 2013-04-23
Status: Merged
Approved by: Michal Hruby on 2013-04-24
Approved revision: 98
Merged at revision: 97
Proposed branch: lp:~stolowski/unity-scope-home/preview-on-lmb
Merge into: lp:unity-scope-home
Diff against target: 81 lines (+29/-18)
1 file modified
src/scope.vala (+29/-18)
To merge this branch: bzr merge lp:~stolowski/unity-scope-home/preview-on-lmb
Reviewer Review Type Date Requested Status
Michal Hruby (community) 2013-04-23 Approve on 2013-04-24
PS Jenkins bot (community) continuous-integration Approve on 2013-04-23
Review via email: mp+160329@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Michal Hruby (mhr3) wrote :

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
97. By Paweł Stołowski on 2013-04-23

Fixed server_sid check.

PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
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) wrote :

> 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) wrote :
review: Approve (continuous-integration)
Michal Hruby (mhr3) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/scope.vala'
2--- src/scope.vala 2013-04-22 12:28:51 +0000
3+++ src/scope.vala 2013-04-23 17:15:30 +0000
4@@ -381,15 +381,20 @@
5 return true;
6 }
7
8- internal async void handle_metrics (Unity.AggregatorActivation activation, string scope_id, string session_id, string server_sid)
9+ internal async void handle_metrics (uint action_type, string scope_id, string session_id, string? server_sid)
10 {
11+ // server_sid may be null if we didn't query smart scopes service
12+ if (server_sid == null)
13+ return;
14+
15+ debug ("Adding activation metrics record for scope %s, action_type=%u", scope_id, action_type);
16 var timestamp = new DateTime.now_utc ();
17
18- if (activation.action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT)
19+ if (action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT)
20 {
21 sss_client.add_click_event (session_id, server_sid, scope_id, timestamp);
22 }
23- else if (activation.action_type == Unity.Protocol.ActionType.PREVIEW_RESULT)
24+ else if (action_type == Unity.Protocol.ActionType.PREVIEW_RESULT)
25 {
26 sss_client.add_preview_event (session_id, server_sid, scope_id, timestamp);
27 }
28@@ -471,7 +476,9 @@
29 {
30 debug ("Activation request for scope %s, action_type=%u", activation.scope_id, activation.action_type);
31
32- if (!scope_mgr.remote_content_search || sss_client == null)
33+ if (!scope_mgr.remote_content_search || sss_client == null ||
34+ (activation.action_type != Unity.Protocol.ActionType.ACTIVATE_RESULT &&
35+ activation.action_type != Unity.Protocol.ActionType.PREVIEW_RESULT))
36 return null; //nothing to do
37
38 string? server_sid = channel_id_map.server_sid_for_channel (activation.channel_id);
39@@ -480,24 +487,28 @@
40 var scope_id = activation.scope_id; //this is an id of master (or a standalone scope such as apps)
41 var scope_id_var = activation.scope_result.metadata.lookup ("scope-id");
42
43- // server_sid may be null if we didn't query smart scopes service
44- if (server_sid != null)
45+ if (scope_id_var != null)
46+ scope_id = scope_id_var.get_string ();
47+ else
48+ debug ("No specific scope_id in the result from master '%s'", scope_id);
49+
50+ bool is_local_scope = scope_mgr.is_client_scope (scope_id) || ScopeRegistry.instance ().is_master (scope_id);
51+
52+ // special case for more suggestions in home lens: both lmb & rmb should display a preview
53+ if (scope_id.has_prefix ("more_suggestions-") &&
54+ !is_local_scope &&
55+ (activation.action_type == Unity.Protocol.ActionType.ACTIVATE_RESULT ||
56+ activation.action_type == Unity.Protocol.ActionType.PREVIEW_RESULT))
57 {
58- if (scope_id_var != null)
59- {
60- scope_id = scope_id_var.get_string ();
61- debug ("Adding activation metrics record for scope %s", scope_id);
62- }
63- else
64- {
65- debug ("No specific scope_id in the result from master '%s'", scope_id);
66- }
67-
68- yield handle_metrics (activation, scope_id, session_id, server_sid);
69+ var preview = yield handle_preview (activation, session_id, server_sid);
70+ yield handle_metrics (Unity.Protocol.ActionType.PREVIEW_RESULT, scope_id, session_id, server_sid);
71+ return preview;
72 }
73
74+ yield handle_metrics (activation.action_type, scope_id, session_id, server_sid);
75+
76 // do nothing for local scopes - activation will be handled by actual scope
77- if (scope_mgr.is_client_scope (scope_id) || ScopeRegistry.instance ().is_master (scope_id))
78+ if (is_local_scope)
79 {
80 debug ("Scope %s is a local scope, passing request to it", scope_id);
81 return null;

Subscribers

People subscribed via source and target branches

to all changes: