Merge lp:~gary-wzl77/scope-aggregator/fix_1521528 into lp:scope-aggregator

[Gary.Wang]

Add display_order field to enable developer to set child scopes display order for keyword scope.

Merge branch that fixes bug lp:1521528 in scope-agg.

Add display_order field to enable developer to set child scopes display order for keyword scope.

[Kyle Nitzsche (knitzsche)]

General Comment

This is an interesting feature. One of the advantages of keywords is that you do not know what scopes may be installed. Yet this feature allows you to set the order inside a keyword by scope ID. Clearly, this allows order control with keyword aggregation. Nice.

It raises the question: why not just declare them as "scope" json objects and thereby set the order absolutely. And the answer is: because keywords move the logic of what results to provide to the child scope and simplifies the aggregation. So I like it!

[Kyle Nitzsche (knitzsche)]

Line 31: I am unclear from this text what the display order will be when there are installed scopes listed in display_order and there are other installed scopes included through the keyword. My expectation is that scopes in display_order will always come first (if installed) and then other scopes in random order. Is that correct?

[Kyle Nitzsche (knitzsche)]

A question: should we also implement this for "category" > "keywords"? Note I just had to add a feature for keyword that was working in category > keywords, so maybe we should try to keep the keyword support unified in both cases.

[Kyle Nitzsche (knitzsche)]

So, while I love this feature and will approve, for now I'll mark as Needs Fixing to get your answers and the typo fix mentioned below.

review: Needs Fixing

[Gary.Wang (gary-wzl77)]

1...the answer is: because keywords move the logic of what results to provide to the child scope and simplifies the aggregation. So I like it!
A: Hah, That's also my general idea to implement this MR. :)

2.My expectation is that scopes in display_order will always come first (if installed) and then other scopes in random order. Is that correct?
A: Yes, that's correct. The testing aggreagtor can be found here. Please have a try

   bzr branch lp:~hanloon-team/hanloon/test-display-order

   Note: The "display_order" rule is only applied to children scopes in "keyword", which doesn't impact "category" or "scope" field.
   E.g. In testing bollywood aggregator, I put cinema scope at first declaration, so scope aggregator will display all children scopes of "keyword" according to display_order *under* cinema scope. If I put cinema scope declaration under "keyword", all children scopes of "keyword" will come first, then cinema scope display below "keyword".

3. A question: should we also implement this for "category" > "keywords" to keep the keyword support unified in both cases.
A: Yes, we should. Can we do it in another MR?

[lp:~gary-wzl77/scope-aggregator/fix_1521528]

157. By Gary.Wang on 2015-12-02

Typo fixed.

[Kyle Nitzsche (knitzsche)]

looks good. Except for one doc comment. line 31 below says this:

"display_order allows developer to set child scopes display order of keyword scope. You can specify multiple scope id in the display_order and scope aggregator will display child scopes according to the order you defined in display_order. If supported child scopes are not defined in display_order, scope aggregator will display them randomly"

That ^ does not quite state clearly that: Installed keyword child scopes that are not listed in display_order are displayed after installed display_order scopes and in a indeterminate order.

Two key points: 1) state clearly that they are AFTER. 2) Please use "indeterminate" instead of any variant of "random". You can use my exact sentence if you want.

Great feature!

review: Approve

[Gary.Wang (gary-wzl77)]

Oh,Yes.
That's more clear. I use your sentence to explain the usage.
Thanks.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-11-30 17:08:54 +0000
+++ CMakeLists.txt	2015-12-02 03:32:24 +0000
@@ -1,4 +1,4 @@
-set(VERSION "4.1")
+set(VERSION "4.2")
 
 # Supress qDebug() output
 ADD_DEFINITIONS( -DQT_NO_DEBUG_OUTPUT )

=== modified file 'README.md'
--- README.md	2015-09-14 02:37:51 +0000
+++ README.md	2015-12-02 03:32:24 +0000
@@ -765,7 +765,7 @@
 
 Value:  json list of objects.
 
-Suppose you want some scopes which supports one certain keywords not to display results, you can specify scope id in the exclude_scopes and scope aggregator will not displays results of specified scopes. 
+Suppose you want some scopes which supports one certain keywords not to display results, you can specify scope id in the exclude_scopes and scope aggregator will not display results of specified scopes. 
 
 Note:This key is only supposed to be declared in a `keywords` json object, which doesn't not use a shared category.
 e.g. amazon supports 'books' keyword, but in most cases it returns something unrelated to 'books' keyword. we can get rid of its results in the way as following.
@@ -786,3 +786,35 @@
      ]
 
 
+#### `display_order` key (optional)
+
+Value:  json list of objects.
+
+display_order allows developer to set child scopes display order of keyword scope. You can specify multiple scope id in the display_order and scope aggregator will display child scopes according to the order you defined in display_order. If supported child scopes are not defined in display_order, scope aggregator will display them randomly.
+
+Note: `display_order` must be a direct child of a `keyword`.
+e.g. the following scopes support 'bollywood.movie' keyword, 
+    1.Shemaroo
+    2.Eros
+    3.ZeeTV
+    4.Star-plus
+     
+if we define display_order as following, shemaroo scope will be shown in the first place, then eros at 2nd place.And zeeTV and star-plus will be shown randomly
+     "order":
+      [
+        {
+            "keyword":
+            {
+                "keyword": "bollywood.movie",
+                "link_to_child" : "true",
+                "display_order" :[
+                    {
+                        "id": "com.canonical.scopes.shemaroo_shemaroo"
+                    },
+                    {
+                        "id": "com.canonical.scopes.eros_eros"
+                    }
+                ] 
+            }
+        }
+     ]

=== modified file 'include/query.h'
--- include/query.h	2015-09-14 02:37:51 +0000
+++ include/query.h	2015-12-02 03:32:24 +0000
@@ -155,6 +155,7 @@
         std::string dept_title_msgid;
         bool link_to_child = false;
         std::vector<std::string> exclude_scopes; // id of exclude scopes
+        std::vector<std::string> display_order;  // id of display order scopes
     };
     std::vector<std::shared_ptr<keyword>> keywords;// pulled from child_sccopes.json
     std::map<std::string, std::shared_ptr<keyword>> id_keyword_map;//keyword id (the keyword string) to keyword instance

=== modified file 'src/utils.cpp'
--- src/utils.cpp	2015-11-20 22:22:35 +0000
+++ src/utils.cpp	2015-12-02 03:32:24 +0000
@@ -664,6 +664,17 @@
                       keyword_.exclude_scopes.emplace_back(id);
                   }
             }
+            if (keyword_o.contains("display_order"))
+            {
+                  QJsonArray scope_a = keyword_o["display_order"].toArray();
+
+                  for (const auto item : scope_a)
+                  {
+                      QJsonObject scope_o = item.toObject();
+                      std::string id = scope_o["id"].toString().toStdString();
+                      keyword_.display_order.emplace_back(id);
+                  }
+            }
             
             std::shared_ptr<keyword> kw_ptr = std::make_shared<keyword>(keyword_);
             keywords.emplace_back(kw_ptr);
@@ -683,6 +694,60 @@
         {
             if (pr.first == id)
             {
+                bool foundKeywod = false;
+                for (auto kwv : keywords)
+                {
+                    if (id == kwv->id)
+                    {
+                        foundKeywod = true;
+
+                        int lastDisplayOrder = -1;
+                        std::vector<std::string>::iterator orderIter;
+                        std::vector<std::string> order_keyscopes;
+                        std::vector<std::string> unorder_keyscopes;
+                        for (std::string scope_id : pr.second)
+                        {
+                            bool foundScope = false;
+                            for (int i = 0; i < kwv->display_order.size() ; i++)
+                            {
+                                if ((kwv->display_order[i] +":"+ kwv->id) == scope_id)
+                                {
+                                    if ( lastDisplayOrder < 0) 
+                                    {
+                                        orderIter = order_keyscopes.insert(order_keyscopes.begin(), scope_id);
+                                    }
+                                    else if (i < lastDisplayOrder)  
+                                    {
+                                        if (i < (orderIter - order_keyscopes.begin()))
+                                        {
+                                            orderIter = order_keyscopes.insert(order_keyscopes.begin(), scope_id);
+                                        } 
+                                        else 
+                                        {
+                                            orderIter = order_keyscopes.insert(orderIter, scope_id);
+                                        }
+                                    }
+                                    else 
+				    {
+                                        orderIter = order_keyscopes.insert(orderIter + 1, scope_id);
+                                    }
+
+                                    foundScope = true;
+                                    lastDisplayOrder = i;
+                                    break;
+                                }
+                            }
+
+                            if (!foundScope) {
+                                unorder_keyscopes.emplace_back(scope_id);
+                            }
+                        }
+
+                        scopes_ordered.insert(scopes_ordered.end(), order_keyscopes.begin(), order_keyscopes.end());
+                        scopes_ordered.insert(scopes_ordered.end(), unorder_keyscopes.begin(), unorder_keyscopes.end());
+                    }
+                }
+
                 for (std::string id_ : pr.second)
                 {
                     scopes_ordered.emplace_back(id_);