Unity Media Scanner Scope

Merge lp://qastaging/~marcustomlinson/unity-scope-mediascanner/aggregate_soundcloud into lp://qastaging/unity-scope-mediascanner

  1. aggregate_soundcloud
  2. Merge into trunk
Proposed by Marcus Tomlinson
Status: Merged
Approved by: Marcus Tomlinson
Approved revision: 211
Merged at revision: 213
Proposed branch: lp://qastaging/~marcustomlinson/unity-scope-mediascanner/aggregate_soundcloud
Merge into: lp://qastaging/unity-scope-mediascanner
Diff against target: 220 lines (+92/-12)
4 files modified
src/musicaggregator/musicaggregator-settings.ini.in (+5/-0)
src/musicaggregator/musicaggregatorquery.cpp (+65/-0)
src/musicaggregator/musicaggregatorscope.cpp (+2/-2)
tests/test-music-aggregator.cpp (+20/-10)
To merge this branch: bzr merge lp://qastaging/~marcustomlinson/unity-scope-mediascanner/aggregate_soundcloud
Reviewer Review Type Date Requested Status
James Henstridge Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+241386@code.qastaging.launchpad.net

Commit message

Aggregate the SoundCloud scope in the musicaggregator scope

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:207
http://jenkins.qa.ubuntu.com/job/unity-scope-mediascanner-ci/159/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-mediascanner-vivid-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-mediascanner-vivid-armhf-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-mediascanner-vivid-i386-ci/2

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scope-mediascanner-ci/159/rebuild

review: Approve (continuous-integration)
Revision history for this message
James Henstridge (jamesh) wrote :

This looks good. I just wonder if it is worth moving some of the special surfacing logic out of the aggregator and down to the underlying scopes similar to how the video aggregator works.

I don't think what we've done there is ideal (using a bogus department ID), but perhaps putting something in the hints dictionary? I'm not suggesting switching the handling of all the child scopes in one go, but could we start with this one?

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> This looks good. I just wonder if it is worth moving some of the special
> surfacing logic out of the aggregator and down to the underlying scopes
> similar to how the video aggregator works.
>
> I don't think what we've done there is ideal (using a bogus department ID),
> but perhaps putting something in the hints dictionary? I'm not suggesting
> switching the handling of all the child scopes in one go, but could we start
> with this one?

Do you mean moving this to the child? :
104 + else if (scopes[i] == soundcloud_scope)

Anything else?

Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

Alright so I agree, we should add a aggregated_by() method to QueryMetadata in order for a scope to find out if it is being aggregated, and to get the id of the aggregating scope.

However, lets just land this change as it is, and I'll add this to my todo list.

Revision history for this message
James Henstridge (jamesh) wrote :

For the songkick integration, I needed to manually filter out a "no location" result. The SoundCloud scope includes a login result at the top when unauthenticated, so does that get shown in the aggregator?

If so, it should be fairly easy to remove by ignoring results under "soundcloud_login_nag".

review: Needs Information
Revision history for this message
Marcus Tomlinson (marcustomlinson) wrote :

> For the songkick integration, I needed to manually filter out a "no location"
> result. The SoundCloud scope includes a login result at the top when
> unauthenticated, so does that get shown in the aggregator?
>
> If so, it should be fairly easy to remove by ignoring results under
> "soundcloud_login_nag".

Yep, you're right. Added the filter.

Revision history for this message
James Henstridge (jamesh) wrote :

Looks good

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: