Merge lp:~abreu-alexandre/unity-js-scopes/department into lp:unity-js-scopes

[Alexandre Abreu]

Add department

Add department

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:85
http://jenkins.qa.ubuntu.com/job/unity-js-scopes-ci/12/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-amd64-ci/12
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/12
        deb: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/12/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-i386-ci/12

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-js-scopes-ci/12/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Left some inline comments

review: Needs Fixing

[lp:~abreu-alexandre/unity-js-scopes/department]

86. By Alexandre Abreu on 2015-10-19

fixes

[Alexandre Abreu (abreu-alexandre)]

updated

[Marcus Tomlinson (marcustomlinson)]

Looks like your working hard to get around converting Department::SPtr to Department::SCPtr in a few places.

E.g.
1) Your subdepartments() method doesn't match the base implementation (you're returning a list of Department::SPtr instead of the intended Department::SCPtr), and
2) your register_departments() method on search-reply also doesn't match the original, it should be taking a Department::SCPtr argument.

I think you'd have a lot less headache if you changed: "unity::scopes::Department::SPtr department()" to "unity::scopes::Department::SCPtr department()".

Then you wouldn't have to do this either:

153: deps.push_back(std::make_shared<unity::scopes::Department>(*d->department()));

Which should really just be: deps.push_back(d->department());

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:86
http://jenkins.qa.ubuntu.com/job/unity-js-scopes-ci/13/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-amd64-ci/13
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/13
        deb: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/13/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-i386-ci/13

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-js-scopes-ci/13/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

> Looks like your working hard to get around converting Department::SPtr to
> Department::SCPtr in a few places.
>
> E.g.
> 1) Your subdepartments() method doesn't match the base implementation (you're
> returning a list of Department::SPtr instead of the intended
> Department::SCPtr), and
> 2) your register_departments() method on search-reply also doesn't match the
> original, it should be taking a Department::SCPtr argument.
>
>
> I think you'd have a lot less headache if you changed:
> "unity::scopes::Department::SPtr department()" to
> "unity::scopes::Department::SCPtr department()".
>
> Then you wouldn't have to do this either:
>
> 153:
> deps.push_back(std::make_shared<unity::scopes::Department>(*d->department()));
>
> Which should really just be: deps.push_back(d->department());

Perhaps to clarify a bit further. This shouldn't have to be so complicated:

std::vector<std::shared_ptr<Department>>
Department::subdepartments() const {
  std::vector<std::shared_ptr<Department>> deps;
  for (auto & d: department_->subdepartments()) {
    deps.push_back(std::shared_ptr<Department>(new Department(*d)));
  }
  return deps;
}

Why not just:

unity::scopes::DepartmentList Department::subdepartments() const {
  return department_->subdepartments();
}

The closer we match the base implementations the easier we make it on ourselves to maintain the bindings.

[lp:~abreu-alexandre/unity-js-scopes/department]

87. By Alexandre Abreu on 2015-10-20

Simplify

[Alexandre Abreu (abreu-alexandre)]

I simplified things a bit, but I cannot really have unity::scopes::Department & DepartmentList play along w/ plain wrapped Department, I see your point here obviously but it maked things more confusing when you have at some point wrapped Department being created and handed back to JS, and also unity::scopes::Department existing at the same level just to simplify handing them off to things like SearchReply ...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:87
http://jenkins.qa.ubuntu.com/job/unity-js-scopes-ci/14/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-amd64-ci/14
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/14
        deb: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-armhf-ci/14/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-js-scopes-wily-i386-ci/14

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-js-scopes-ci/14/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

> I simplified things a bit, but I cannot really have unity::scopes::Department
> & DepartmentList play along w/ plain wrapped Department, I see your point here
> obviously but it maked things more confusing when you have at some point
> wrapped Department being created and handed back to JS, and also
> unity::scopes::Department existing at the same level just to simplify handing
> them off to things like SearchReply ...

Sorry yes, I did get a little confused with where you were dealing with the wrapped and base versions of the class, but looks much cleaner now thanks!

review: Approve

=== modified file 'src/bindings/src/addon.cc'
--- src/bindings/src/addon.cc	2015-10-16 13:39:16 +0000
+++ src/bindings/src/addon.cc	2015-10-20 14:40:24 +0000
@@ -35,6 +35,7 @@
 #include "action-metadata.h"
 #include "activation-query.h"
 #include "categorised-result.h"
+#include "department.h"
 #include "online-account-client.h"
 #include "preview-query.h"
 #include "preview-reply.h"
@@ -208,6 +209,43 @@
       .add_method("number_of_columns", &unity::scopes::ColumnLayout::number_of_columns)
       .add_method("column", &unity::scopes::ColumnLayout::column);
 
+    v8cpp::Class<Department> department(isolate);
+    department
+      .set_constructor<v8::FunctionCallbackInfo<v8::Value>>()
+      .add_method("set_subdepartments", &Department::set_subdepartments)
+      .add_method("add_subdepartment", &Department::add_subdepartment)
+      .add_method("set_alternate_label", &Department::set_alternate_label)
+      .add_method("set_has_subdepartments", &Department::set_has_subdepartments)
+      .add_method("id", &Department::id)
+      .add_method("label", &Department::label)
+      .add_method("alternate_label", &Department::alternate_label)
+      .add_method("query", &Department::query)
+      .add_method("subdepartments", &Department::subdepartments)
+      .add_method("has_subdepartments", &Department::has_subdepartments);
+
+    v8cpp::Class<unity::scopes::Location> location(isolate);
+    location
+      // unity::scopes::Location
+      .add_method("altitude", &unity::scopes::Location::altitude)
+      .add_method("has_altitude", &unity::scopes::Location::has_altitude)
+      .add_method("area_code", &unity::scopes::Location::area_code)
+      .add_method("has_area_code", &unity::scopes::Location::has_area_code)
+      .add_method("city", &unity::scopes::Location::city)
+      .add_method("has_city", &unity::scopes::Location::has_city)
+      .add_method("has_country_code", &unity::scopes::Location::has_country_code)
+      .add_method("country_code", &unity::scopes::Location::country_code)
+      .add_method("has_country_name", &unity::scopes::Location::has_country_name)
+      .add_method("country_name", &unity::scopes::Location::country_name)
+      .add_method("has_horizontal_accuracy", &unity::scopes::Location::has_horizontal_accuracy)
+      .add_method("horizontal_accuracy", &unity::scopes::Location::horizontal_accuracy)
+      .add_method("latitude", &unity::scopes::Location::latitude)
+      .add_method("longitude", &unity::scopes::Location::longitude)
+      .add_method("has_region_code", &unity::scopes::Location::has_region_code)
+      .add_method("region_code", &unity::scopes::Location::region_code)
+      .add_method("has_region_name", &unity::scopes::Location::has_region_name)
+      .add_method("region_name", &unity::scopes::Location::region_name)
+      .add_method("has_vertical_accuracy", &unity::scopes::Location::has_vertical_accuracy);
+
     v8cpp::Class<PreviewWidget> preview_widget(isolate);
     preview_widget
       .set_constructor<v8::Local<v8::Value>, v8::Local<v8::Value>>()
@@ -277,29 +315,6 @@
       .add_method("onrun", &SearchQuery::onrun)
       .add_method("oncancelled", &SearchQuery::oncancelled);
 
-    v8cpp::Class<unity::scopes::Location> location(isolate);
-    location
-      // unity::scopes::Location
-      .add_method("altitude", &unity::scopes::Location::altitude)
-      .add_method("has_altitude", &unity::scopes::Location::has_altitude)
-      .add_method("area_code", &unity::scopes::Location::area_code)
-      .add_method("has_area_code", &unity::scopes::Location::has_area_code)
-      .add_method("city", &unity::scopes::Location::city)
-      .add_method("has_city", &unity::scopes::Location::has_city)
-      .add_method("has_country_code", &unity::scopes::Location::has_country_code)
-      .add_method("country_code", &unity::scopes::Location::country_code)
-      .add_method("has_country_name", &unity::scopes::Location::has_country_name)
-      .add_method("country_name", &unity::scopes::Location::country_name)
-      .add_method("has_horizontal_accuracy", &unity::scopes::Location::has_horizontal_accuracy)
-      .add_method("horizontal_accuracy", &unity::scopes::Location::horizontal_accuracy)
-      .add_method("latitude", &unity::scopes::Location::latitude)
-      .add_method("longitude", &unity::scopes::Location::longitude)
-      .add_method("has_region_code", &unity::scopes::Location::has_region_code)
-      .add_method("region_code", &unity::scopes::Location::region_code)
-      .add_method("has_region_name", &unity::scopes::Location::has_region_name)
-      .add_method("region_name", &unity::scopes::Location::region_name)
-      .add_method("has_vertical_accuracy", &unity::scopes::Location::has_vertical_accuracy);
-
     v8cpp::Class<SearchMetaData> search_metadata(isolate);
     search_metadata
       .add_inheritance<unity::scopes::SearchMetadata>()
@@ -352,6 +367,7 @@
     module.add_class("categorised_result", categorised_result);
     module.add_class("category_renderer", category_renderer);
     module.add_class("column_layout", column_layout);
+    module.add_class("department", department);
     module.add_class("location", location);
     module.add_class("online_account_client", online_account_client);
     module.add_class("preview_widget", preview_widget);

=== added file 'src/bindings/src/department.cc'
--- src/bindings/src/department.cc	1970-01-01 00:00:00 +0000
+++ src/bindings/src/department.cc	2015-10-20 14:40:24 +0000
@@ -0,0 +1,104 @@
+/*
+ * Copyright 2015 Canonical Ltd.
+ *
+ * This file is part of unity-js-scopes.
+ *
+ * unity-js-scopes is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * unity-js-scopes is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "department.h"
+
+
+Department::Department(const unity::scopes::Department& d)
+  : department_(new unity::scopes::Department(d)) {
+}
+
+Department::Department(v8::FunctionCallbackInfo<v8::Value> const & args) {
+  if (args.Length() != 2 || args.Length() != 3) {
+    throw std::runtime_error("Invalid number of arguments");
+  }
+
+  if (args[0]->IsString()) {
+    auto department_id =
+      v8cpp::from_v8<std::string>(v8::Isolate::GetCurrent(), args[0]);
+    auto cq =
+      v8cpp::from_v8<std::shared_ptr<unity::scopes::CannedQuery>>(
+          v8::Isolate::GetCurrent(), args[1]);
+    auto label =
+      v8cpp::from_v8<std::string>(v8::Isolate::GetCurrent(), args[2]);
+
+    department_ = unity::scopes::Department::create(department_id, *cq, label);
+  } else {
+    auto cq =
+      v8cpp::from_v8<std::shared_ptr<unity::scopes::CannedQuery>>(
+          v8::Isolate::GetCurrent(), args[0]);
+    auto label =
+      v8cpp::from_v8<std::string>(v8::Isolate::GetCurrent(), args[1]);
+
+    department_ = unity::scopes::Department::create(*cq, label);
+  }
+}
+
+void Department::set_subdepartments(std::vector<std::shared_ptr<Department>> departments) {
+  unity::scopes::DepartmentList deps;
+  for (auto & d: departments) {
+    deps.push_back(d->department());
+  }
+  department_->set_subdepartments(deps);
+}
+
+void Department::add_subdepartment(std::shared_ptr<Department> const &department) {
+  department_->add_subdepartment(department->department());
+}
+
+void Department::set_alternate_label(std::string const &label) {
+  department_->set_alternate_label(label);
+}
+
+void Department::set_has_subdepartments(bool subdepartments) {
+  department_->set_has_subdepartments(subdepartments);
+}
+
+std::string Department::id() const {
+  return department_->id();
+}
+
+std::string Department::label() const {
+  return department_->label();
+}
+
+std::string Department::alternate_label() const {
+  return department_->alternate_label();
+}
+
+unity::scopes::CannedQuery
+Department::query () const {
+  return department_->query();
+}
+
+std::vector<std::shared_ptr<Department>>
+Department::subdepartments() const {
+  std::vector<std::shared_ptr<Department>> deps;
+  for (auto & d: department_->subdepartments()) {
+    deps.push_back(std::make_shared<Department>(*d));
+  }
+  return deps;
+}
+
+bool Department::has_subdepartments() const {
+  return department_->has_subdepartments();
+}
+
+unity::scopes::Department::SCPtr Department::department() {
+  return department_;
+}

=== added file 'src/bindings/src/department.h'
--- src/bindings/src/department.h	1970-01-01 00:00:00 +0000
+++ src/bindings/src/department.h	2015-10-20 14:40:24 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright 2015 Canonical Ltd.
+ *
+ * This file is part of unity-js-scopes.
+ *
+ * unity-js-scopes is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 3.
+ *
+ * unity-js-scopes is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef _UNITY_JS_DEPARTMENT_H_
+#define _UNITY_JS_DEPARTMENT_H_
+
+#include <unity/scopes/Department.h>
+#include <unity/scopes/CannedQuery.h>
+
+#include <v8-cpp.h>
+
+class Department
+{
+ public:
+  Department(const unity::scopes::Department& d);
+  Department(v8::FunctionCallbackInfo<v8::Value> const& args);
+
+  // v8 bindings
+  void set_subdepartments(std::vector<std::shared_ptr<Department>> departments);
+  void add_subdepartment(std::shared_ptr<Department> const &department);
+  void set_alternate_label(std::string const &label);
+  void set_has_subdepartments(bool subdepartments);
+  std::string id() const;
+  std::string label() const;
+  std::string alternate_label() const;
+  unity::scopes::CannedQuery query () const;
+  std::vector<std::shared_ptr<Department>> subdepartments() const;
+  bool has_subdepartments() const;
+
+  unity::scopes::Department::SCPtr department();
+
+ private:
+  std::shared_ptr<unity::scopes::Department> department_;
+};
+
+#endif // _UNITY_JS_DEPARTMENT_H_

=== modified file 'src/bindings/src/search-reply.cc'
--- src/bindings/src/search-reply.cc	2015-10-09 17:00:22 +0000
+++ src/bindings/src/search-reply.cc	2015-10-20 14:40:24 +0000
@@ -56,3 +56,7 @@
 {
   reply_->finished();
 }
+
+void SearchReply::register_departments(std::shared_ptr<Department> department) {
+  reply_->register_departments(department->department());
+}

=== modified file 'src/bindings/src/search-reply.h'
--- src/bindings/src/search-reply.h	2015-10-09 17:00:22 +0000
+++ src/bindings/src/search-reply.h	2015-10-20 14:40:24 +0000
@@ -23,6 +23,7 @@
 #include <unity/scopes/Category.h>
 
 #include "categorised-result.h"
+#include "department.h"
 
 #include <v8-cpp.h>
 
@@ -33,6 +34,8 @@
   SearchReply(unity::scopes::SearchReplyProxy const& reply);
   ~SearchReply();
 
+  void register_departments(std::shared_ptr<Department> department);
+
   unity::scopes::Category::SCPtr register_category(
       const std::string& id,
       const std::string& title,