scope-aggregator

Merge lp://staging/~knitzsche/scope-aggregator/refactor-handle-methods_add-login_remove-shared-category into lp://staging/scope-aggregator

  1. refactor-handle-methods_add-login_remove-shared-category
  2. Merge into vivid-trunk
Proposed by Kyle Nitzsche
Status: Merged
Approved by: Kyle Nitzsche
Approved revision: 191
Merged at revision: 174
Proposed branch: lp://staging/~knitzsche/scope-aggregator/refactor-handle-methods_add-login_remove-shared-category
Merge into: lp://staging/scope-aggregator
Diff against target: 3725 lines (+1703/-1386)
15 files modified
DELETED_FEATURES.md (+1/-0)
README.md (+270/-286)
gtests/CMakeLists.txt (+4/-0)
gtests/MockRegistry.h (+7/-0)
gtests/scope_directory/login_1.json (+74/-0)
gtests/scope_directory/login_2.json (+73/-0)
gtests/scope_directory/login_3.json (+35/-0)
gtests/scope_test.cpp (+110/-3)
include/aggchildscope.h (+36/-1)
include/query.h (+16/-9)
src/CMakeLists.txt (+1/-0)
src/aggchildscope.cpp (+34/-1)
src/handle_results.cpp (+960/-0)
src/query.cpp (+2/-9)
src/utils.cpp (+80/-1077)
To merge this branch: bzr merge lp://staging/~knitzsche/scope-aggregator/refactor-handle-methods_add-login_remove-shared-category
Reviewer Review Type Date Requested Status
Zhang Enwei (community) Approve
Gary.Wang Pending
Review via email: mp+307352@code.staging.launchpad.net

Description of the change

For a proposed 4.13 release.

* Significant refactor of the three methods that handle results:
handle_category_child(), handle_keyword_child(), handle_declared_child(). These
were simply too hard to debug and understand, and they had redundant code to
lookup and, if needed, to register Categories. Each of the three methods is
refactored as follows: define an enum that states the possible run types for
the method: get state variables, analyze state and set run type, switch on run
type to select code. Category lookup and registration is now moved to new
set_result_category() method, which has two signatures depending on whether a
link to child is needed. The three methods (and related) are moved to a new
file for clarity: handle_results.cpp.

* Added support for a "login_renderer" and
"login_renderer_incoming_category_id" declarations in child_scopes.json. See
the updated README.md. I also added gtests for this.

* Removed "shared category" from "keywords". This feature is not used, I think.
I did not find it in all child_scopes.json files in clicks branch. It is mostly
redundant with a "category". That is, they both confine keywords to a Category
(although category type also can include scopes). We can reimplemented any
features that were supported only in keywords that are not supported in
categories if needed.

* Added Query::is_using_departments() method. And added gtesting for this.
Note: With this MP, an agg scope child_scopes.json file does not need a
"departments" object: it's absence is sufficient to signal no departments.

* Updated/rewrote README.md.

I tested this in Today, News, Nearby and Photos. It would be awesome if the .so
could be tested with our other aggregators!

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

Note: I started doing this refactoring to help debug https://bugs.launchpad.net/canonical-devices-system-image/+bug/1622423. However, after the refactor that issue continues, and I suspect it may be a unity8 rendering issue (I am working with unity api team on this separately).

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

LGTM except some typos.
I verified on food, films, china-dashboard, news-china and baidu aggregator scopes.

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

thanks Enwei.

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