Merge lp:~zhangew401/scope-aggregator/fix-lp-1524597 into lp:scope-aggregator

Proposed by Zhang Enwei
Status: Merged
Approved by: Kyle Nitzsche
Approved revision: 171
Merged at revision: 170
Proposed branch: lp:~zhangew401/scope-aggregator/fix-lp-1524597
Merge into: lp:scope-aggregator
Diff against target: 137 lines (+89/-1)
4 files modified
CMakeLists.txt (+1/-1)
README.md (+57/-0)
include/query.h (+1/-0)
src/utils.cpp (+30/-0)
To merge this branch: bzr merge lp:~zhangew401/scope-aggregator/fix-lp-1524597
Reviewer Review Type Date Requested Status
Gary.Wang Needs Fixing
Kyle Nitzsche (community) Approve
Penk Chen Pending
Review via email: mp+301766@code.launchpad.net

Description of the change

In order to fix lp:1524597, I introduced key "sources" for "scope" key.

This key could only be set for "com.canonical.scopes.clickstore" scope. It is used to inform the user which scopes can be downloaded from the store for this aggregator in case there is little child scopes preinstalled. Example:
        [...]
        "scope":
        {
            "_category_title": "Install More Sources:",
            "id": "com.canonical.scopes.clickstore",
            "link_to_child": "true",
            "local_id": "com.canonical.scopes.clickstore",
            "department": "dept-movies",
            "cardinality": 10,
            "sources":[
                {
                    "id": "com.canonical.scopes.eros_eros"
                },
                {
                    "id": "com.canonical.scopes.hotstar_hotstar"
                },
                {
                    "id": "com.canonical.scopes.timesofindia_timesofindia"
                },
                {
                    "id": "com.canonical.scopes.shemaroo_shemaroo"
                },
                {
                    "id": "com.canonical.scopes.zeetv_zeetv"
                }
            ]
        }
        [...]

This is only useful for aggregators that only use "keyword" to aggregate child scopes. For declared scopes, if some declared scope is not installed, you can easily use above example without "sources" key to inform the user to download missing declared scopes from the store.

To post a comment you must log in.
Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

Hi Gary,

The README.md now says this applies to scopes aggregated by keyword (because it previously worked for scopes aggregated by scope ID).

Question: How does it work for an aggregator scope that includes both:
* scopes aggregated by scope ID
* scopes aggregated by keyword

Does the child_scopes.json dev *only* need to add scope ID for keyword scopes to the new "sources" list because scopes aggregated by scope ID will already display?

Or when this "sources" key is present, does it completed supersede the previous behavior (which would mean the dev would need to add *both* scopes agg'd by scope id and by keyword?

Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

Please change this text in README.md:
This key could only be set for "com.canonical.scopes.clickstore" scope.

To this:
This key only applies to the "com.canonical.scopes.clickstore" scope.

review: Needs Fixing
Revision history for this message
Zhang Enwei (zhangew401) wrote :

Thanks Kyle.
Sorry for the confusion.
1. If both two kinds of scopes exist in child_scopes.json, it will collect missing scopes in both kinds and show to the end user.
2. For you question, "Does the child_scopes.json dev *only* need to add scope ID for keyword scopes to the new "sources" list because scopes aggregated by scope ID will already display?"
   Yes.

171. By Zhang Enwei

Update explanation in README.md

Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

Hi Enwei,

Just for clarity, can you please use this in the README instead of the propsed text?

This key is only used on the special clickstore scope declared with "id": "com.canonical.scopes.clickstore".

This clickstore scope displays scopes that are not installed and allows the user to install them.

By default, the clickstore scope (when declared) displays all child scopes declared in the child scopes.jon file by scope ID that are not currently installed.

This key allows the clickstore scope to also display scopes that are aggregated by keyword and that are not installed (in addition to scopes declared by ID and not installed, if any).

Example:

(show your USUAGE EXAMPLE json)

Example with no keyword_sources:

        [...]
        "scope":
        {
            "_category_title": "Install More Sources:",
            "id": "com.canonical.scopes.clickstore",
            "link_to_child": "true",
            "local_id": "com.canonical.scopes.clickstore",
            "department": "dept-movies",
            "cardinality": 10,
        }
        [...]

Revision history for this message
Kyle Nitzsche (knitzsche) wrote :

I think it would be even more clear if the key name is changed (in source and README.md) to "keyword_scope_ids".

Please make these two changes and go ahead and merge.

Thanks!

review: Approve
Revision history for this message
Zhang Enwei (zhangew401) wrote :

Thanks Kyle so much.

172. By Zhang Enwei

Upddate naming and README.md

Revision history for this message
Gary.Wang (gary-wzl77) wrote :

Hi Enwei
   Sorry for the late reply.
   Today I added this feature in bollywood aggregator to verify it, here is the child_json.file
   https://bazaar.launchpad.net/~hanloon-team/hanloon/bollywood_opt_keyword/revision/24

   However, I can see some scope installation recommendation in scope that I don't list in keyword_scope_ids. Please have a check the snapshot below
   https://drive.google.com/open?id=0B2H9ECPSSfqIS2UxX3FzYU9peW8

   Also please see my inline comment below. I will merge my branch into trunk later after this problem is fixed as this one is prerequisite branch.
  Thanks.

   Here is my image info
  current build number: 34
device name: krillin
channel: ubuntu-touch/stable/bq-aquaris.en
last update: 2016-08-01 15:23:14
version version: 34
version ubuntu: 20160708
version tag: OTA-12
version device: 20160606-ab415b2
version custom: 20160701-981-38-14

review: Needs Fixing
Revision history for this message
Zhang Enwei (zhangew401) wrote :

Thanks Gary, for your screen shot. Have you updated the .so file for bollywood? If you didn't do this, your result is expected.

Revision history for this message
Gary.Wang (gary-wzl77) wrote :

Yes, I updated the .so file for bollywood in my testing and didn't push the
.so file into trunk as you've updated it

On Thu, Aug 4, 2016 at 11:13 AM, Zhang Enwei <email address hidden>
wrote:

> Thanks Gary, for your screen shot. Have you updated the .so file for
> bollywood? If you didn't do this, your result is expected.
> --
>
> https://code.launchpad.net/~zhangew401/scope-aggregator/fix-lp-1524597/+merge/301766
> You are reviewing the proposed merge of
> lp:~zhangew401/scope-aggregator/fix-lp-1524597 into lp:scope-aggregator.
>

--
Br
Gary.Wzl

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'CMakeLists.txt'
2--- CMakeLists.txt 2016-07-26 15:14:40 +0000
3+++ CMakeLists.txt 2016-08-04 01:18:33 +0000
4@@ -1,4 +1,4 @@
5-set(VERSION "4.11")
6+set(VERSION "4.12")
7
8 # Supress qDebug() output
9 ADD_DEFINITIONS( -DQT_NO_DEBUG_OUTPUT )
10
11=== modified file 'README.md'
12--- README.md 2016-04-22 23:29:09 +0000
13+++ README.md 2016-08-04 01:18:33 +0000
14@@ -473,6 +473,63 @@
15 ]
16 [...]
17
18+#### `keyword_scope_ids` key (optional)
19+
20+Value: json list of objects.
21+
22+This key is only used on the special clickstore scope declared with "id": "com.canonical.scopes.clickstore".
23+
24+This clickstore scope displays scopes that are not installed and allows the user to install them.
25+
26+By default, the clickstore scope (when declared) displays all child scopes declared in the child_scopes.json file by scope ID that are not currently installed.
27+
28+This key allows the clickstore scope to also display scopes that are aggregated by keyword and that are not installed (in addition to scopes declared by ID and not installed, if any).
29+
30+Example:
31+
32+ [...]
33+ "scope":
34+ {
35+ "_category_title": "Install More Sources:",
36+ "id": "com.canonical.scopes.clickstore",
37+ "link_to_child": "true",
38+ "local_id": "com.canonical.scopes.clickstore",
39+ "department": "dept-movies",
40+ "cardinality": 10,
41+ "sources":[
42+ {
43+ "id": "com.canonical.scopes.eros_eros"
44+ },
45+ {
46+ "id": "com.canonical.scopes.hotstar_hotstar"
47+ },
48+ {
49+ "id": "com.canonical.scopes.timesofindia_timesofindia"
50+ },
51+ {
52+ "id": "com.canonical.scopes.shemaroo_shemaroo"
53+ },
54+ {
55+ "id": "com.canonical.scopes.zeetv_zeetv"
56+ }
57+ ]
58+ }
59+ [...]
60+
61+Example with no `keyword_scope_ids`:
62+
63+ [...]
64+ "scope":
65+ {
66+ "_category_title": "Install More Sources:",
67+ "id": "com.canonical.scopes.clickstore",
68+ "link_to_child": "true",
69+ "local_id": "com.canonical.scopes.clickstore",
70+ "department": "dept-movies",
71+ "cardinality": 10,
72+ }
73+ [...]
74+
75 ### `keyword` (optional)
76
77 Declare a specific keyword for aggregation.
78
79=== modified file 'include/query.h'
80--- include/query.h 2016-06-29 10:29:37 +0000
81+++ include/query.h 2016-08-04 01:18:33 +0000
82@@ -210,6 +210,7 @@
83 std::string scope_dir_;
84 std::string cache_dir_;
85 std::string query_store_; //for searching the store for a given a scope
86+ std::list<std::string> sources_for_clickstore;
87 unity::scopes::RegistryProxy registry_;
88 AggScope *agg_scope_;
89
90
91=== modified file 'src/utils.cpp'
92--- src/utils.cpp 2016-07-26 15:14:40 +0000
93+++ src/utils.cpp 2016-08-04 01:18:33 +0000
94@@ -521,6 +521,18 @@
95 cs->overriding_search_template = true;
96 }
97
98+ if (cs->id == "com.canonical.scopes.clickstore" && scope_o.contains(QStringLiteral("keyword_scope_ids")))
99+ {
100+ QJsonArray scope_a = scope_o[QStringLiteral("keyword_scope_ids")].toArray();
101+
102+ for (const auto & item : scope_a)
103+ {
104+ QJsonObject scope_o = item.toObject();
105+ std::string id = scope_o[QStringLiteral("id")].toString().toStdString();
106+ sources_for_clickstore.emplace_back(id);
107+ }
108+ }
109+
110 child_scopes.emplace_back(cs);
111 child_scopes_m[cs->local_id] = cs;
112 type_ids_m[cs->local_id].emplace_back(cs->local_id);
113@@ -1155,6 +1167,24 @@
114 has_one_source = true;
115 }
116 }
117+ for (auto source : sources_for_clickstore)
118+ {
119+ AggChildScope scope(source);
120+ scope.set_proxy(registry_);
121+ if (!scope.exists()) {
122+ auto myID = QString::fromStdString(source);
123+ myID = myID.left(myID.indexOf("_"));
124+ if (query_store_ == "")
125+ {
126+ query_store_.append("name:");
127+ }
128+ else
129+ {
130+ query_store_.append("+");
131+ }
132+ query_store_.append(myID.toStdString());
133+ }
134+ }
135 }
136 catch (unity::Exception const& e)
137 {

Subscribers

People subscribed via source and target branches

to all changes: