Unity Home Scope

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

  1. preview-on-lmb
  2. Merge into trunk
Proposed by Paweł Stołowski
Status: Merged
Approved by: Michal Hruby
Approved revision: 98
Merged at revision: 97
Proposed branch: lp://qastaging/~stolowski/unity-scope-home/preview-on-lmb
Merge into: lp://qastaging/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://qastaging/~stolowski/unity-scope-home/preview-on-lmb
Reviewer Review Type Date Requested Status
Michal Hruby (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+160329@code.qastaging.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.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
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
lp://qastaging/~stolowski/unity-scope-home/preview-on-lmb updated
97. By Paweł Stołowski

Fixed server_sid check.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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://qastaging/~stolowski/unity-scope-home/preview-on-lmb updated
98. By Paweł Stołowski

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

Revision history for this message
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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: