Merge lp:~stolowski/unity-scopes-api/update-push-filters into lp:unity-scopes-api/devel

[Paweł Stołowski]

Marked the push(Filters const&, FilterState const&) method of SearchReply and SearchListenerBase
as deprecated and provided push methods which take Filters argument only.

Marked the push(Filters const&, FilterState const&) method of SearchReply and SearchListenerBase
as deprecated and provided push methods which take Filters argument only.

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:681
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/27/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/78/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/85/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/27/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Looking good. Just a few inline comments, mostly grammatical.

review: Needs Fixing

[lp:~stolowski/unity-scopes-api/update-push-filters]

682. By Paweł Stołowski on 2016-06-28

Review comments

[unity-api-1-bot (unity-api-1-bot)]

FAILED: Continuous integration, rev:682
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/31/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build/82/console
    FAILURE: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/89/console

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/31/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

> Looking good. Just a few inline comments, mostly grammatical.

Ok, done. Thanks for the corrections... will I ever get this stuff right ;)...

[Marcus Tomlinson (marcustomlinson)]

> > Looking good. Just a few inline comments, mostly grammatical.
>
> Ok, done. Thanks for the corrections... will I ever get this stuff right ;)...

;) It's really not that bad! Thanks Pawel. Although, now I see there are 2 conflicts with devel

review: Needs Fixing

[lp:~stolowski/unity-scopes-api/update-push-filters]

683. By Paweł Stołowski on 2016-06-28

Merged devel

[Paweł Stołowski (stolowski)]

> > > Looking good. Just a few inline comments, mostly grammatical.
> >
> > Ok, done. Thanks for the corrections... will I ever get this stuff right
> ;)...
>
> ;) It's really not that bad! Thanks Pawel. Although, now I see there are 2
> conflicts with devel

Ok, fixed + bumped to 1.0.7.

[Marcus Tomlinson (marcustomlinson)]

> > > > Looking good. Just a few inline comments, mostly grammatical.
> > >
> > > Ok, done. Thanks for the corrections... will I ever get this stuff right
> > ;)...
> >
> > ;) It's really not that bad! Thanks Pawel. Although, now I see there are 2
> > conflicts with devel
>
> Ok, fixed + bumped to 1.0.7.

Thanks +1

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'RELEASE_NOTES.md'
--- RELEASE_NOTES.md	2016-05-23 07:55:39 +0000
+++ RELEASE_NOTES.md	2016-06-28 08:18:23 +0000
@@ -1,6 +1,11 @@
 Release notes
 =============
 
+Changes in version 1.0.7
+========================
+  - Marked the push(Filters const&, FilterState const&) method of SearchReply and SearchListenerBase
+    as deprecated and provided push methods which take the Filters argument only.
+
 Changes in version 1.0.6
 ========================
   - Got rid of category header background, as per design (Bug #1446216).
 
=== modified file 'STRUCTS'
--- STRUCTS	2016-02-25 16:26:09 +0000
+++ STRUCTS	2016-06-28 08:18:23 +0000
@@ -142,10 +142,9 @@
  'central_label'       : string
  'filter_group' : string, optional
 
-Filters + FilterState
-=====================
+Filters
+=======
  'filter'       : array of filters (implementations of FilterBase)
- 'filter_state' : FilterState
 
 PreviewResult (returned by serialize())
 =======================================
 
=== modified file 'debian/VERSION'
--- debian/VERSION	2016-05-23 07:56:41 +0000
+++ debian/VERSION	2016-06-28 08:18:23 +0000
@@ -1,1 +1,1 @@
-1.0.6
+1.0.7
 
=== modified file 'debian/changelog'
--- debian/changelog	2016-06-17 07:27:25 +0000
+++ debian/changelog	2016-06-28 08:18:23 +0000
@@ -1,3 +1,11 @@
+unity-scopes-api (1.0.7-0ubuntu1) UNRELEASED; urgency=medium
+
+  * Marked the push(Filters const&, FilterState const&) method of SearchReply
+    and SearchListenerBase as deprecated and provided push methods which take
+    the Filters argument only.
+
+ -- Pawel Stolowski <[e-mail]>  Tue, 28 Jun 2016 10:11:22 +0200
+
 unity-scopes-api (1.0.6+16.10.20160617-0ubuntu1) yakkety; urgency=medium
 
   [ Marcus Tomlinson ]
 
=== modified file 'include/unity/scopes/SearchListenerBase.h'
--- include/unity/scopes/SearchListenerBase.h	2015-09-01 22:19:45 +0000
+++ include/unity/scopes/SearchListenerBase.h	2016-06-28 08:18:23 +0000
@@ -96,10 +96,19 @@
     /**
     \brief Called once by the scopes to send all the filters and their state.
 
+    \deprecated Please override the push(Filters& const) method instead. This method will be removed from future releases.
+
     The default implementation does nothing.
     */
     virtual void push(Filters const& filters, FilterState const& filter_state);
 
+    /**
+    \brief Called once by the scope to send all filters and their states.
+
+    The default implementation does nothing.
+    */
+    virtual void push(Filters const& filters);
+
 protected:
     /// @cond
     SearchListenerBase();
 
=== modified file 'include/unity/scopes/SearchReply.h'
--- include/unity/scopes/SearchReply.h	2015-10-29 02:35:50 +0000
+++ include/unity/scopes/SearchReply.h	2016-06-28 08:18:23 +0000
@@ -139,7 +139,8 @@
     virtual bool push(experimental::Annotation const& annotation) = 0;
 
     /**
-    \brief Sends all filters and their state to the source of a query.
+    \brief Sends all filters and their states to the source of a query.
+    \deprecated Sending filter state back to the UI is deprecated and will be removed from future releases. Please use the push(Filters const&) method instead.
     \return True if the filters were accepted, false otherwise.
     */
     virtual bool push(Filters const& filters, FilterState const& filter_state) = 0;
@@ -190,6 +191,12 @@
     */
     virtual void push_surfacing_results_from_cache() = 0;
 
+    /**
+    \brief Sends all filter definitions to the source of a query.
+    \return True if the filters were accepted, false otherwise.
+    */
+    virtual bool push(Filters const& filters) = 0;
+
 protected:
     /// @cond
     SearchReply();
 
=== modified file 'include/unity/scopes/internal/SearchReplyImpl.h'
--- include/unity/scopes/internal/SearchReplyImpl.h	2015-02-27 05:28:09 +0000
+++ include/unity/scopes/internal/SearchReplyImpl.h	2016-06-28 08:18:23 +0000
@@ -80,6 +80,8 @@
 
     virtual void push_surfacing_results_from_cache() noexcept override;
 
+    virtual bool push(unity::scopes::Filters const& filters) override;
+
 private:
     bool push(Category::SCPtr category);
     void write_cached_results() noexcept;
@@ -94,7 +96,6 @@
 
     Department::SCPtr cached_departments_;
     unity::scopes::Filters cached_filters_;
-    unity::scopes::FilterState cached_filter_state_;
     std::vector<unity::scopes::CategorisedResult> cached_results_;
     std::mutex mutex_;
 };
 
=== modified file 'include/unity/scopes/testing/MockSearchReply.h'
--- include/unity/scopes/testing/MockSearchReply.h	2015-04-10 10:10:06 +0000
+++ include/unity/scopes/testing/MockSearchReply.h	2016-06-28 08:18:23 +0000
@@ -66,6 +66,7 @@
     MOCK_METHOD1(lookup_category, Category::SCPtr(std::string const&));
     MOCK_METHOD1(push, bool(CategorisedResult const&));
     MOCK_METHOD2(push, bool(Filters const&, FilterState const&));
+    MOCK_METHOD1(push, bool(Filters const&));
     MOCK_METHOD1(push, bool(experimental::Annotation const& annotation));
     MOCK_METHOD0(push_surfacing_results_from_cache, void());
 
 
=== modified file 'include/unity/scopes/utility/internal/BufferedSearchReplyImpl.h'
--- include/unity/scopes/utility/internal/BufferedSearchReplyImpl.h	2015-02-27 05:28:09 +0000
+++ include/unity/scopes/utility/internal/BufferedSearchReplyImpl.h	2016-06-28 08:18:23 +0000
@@ -78,6 +78,7 @@
     std::string to_string() override;
 
     void flush();
+    bool push(unity::scopes::Filters const& filters) override;
 
 private:
     std::mutex mutex_;
 
=== modified file 'src/scopes/SearchListenerBase.cpp'
--- src/scopes/SearchListenerBase.cpp	2014-06-17 15:07:13 +0000
+++ src/scopes/SearchListenerBase.cpp	2016-06-28 08:18:23 +0000
@@ -44,6 +44,11 @@
     // Intentionally empty: "do nothing" default implementation.
 }
 
+void SearchListenerBase::push(Filters const& /* filters */)
+{
+    // Intentionally empty: "do nothing" default implementation.
+}
+
 void SearchListenerBase::push(Category::SCPtr const& /* category */)
 {
     // Intentionally empty: "do nothing" default implementation.
 
=== modified file 'src/scopes/internal/ResultReplyObject.cpp'
--- src/scopes/internal/ResultReplyObject.cpp	2016-02-24 16:49:34 +0000
+++ src/scopes/internal/ResultReplyObject.cpp	2016-06-28 08:18:23 +0000
@@ -74,16 +74,11 @@
         }
 
         Filters const filters = FilterBaseImpl::deserialize_filters(it->second.get_array(), groups);
-        it = data.find("filter_state");
-        if (it != data.end())
-        {
-            auto filter_state = FilterStateImpl::deserialize(it->second.get_dict());
-            receiver_->push(filters, filter_state);
-        }
-        else
-        {
-            throw InvalidArgumentException("ResultReplyObject::push(): filters present but missing filter_state data");
-        }
+
+        // temporarily support both variants of push until the one supporting filter state gets dropped.
+        FilterState filter_state;
+        receiver_->push(filters, filter_state);
+        receiver_->push(filters);
     }
 
     it = data.find("category");
 
=== modified file 'src/scopes/internal/SearchReplyImpl.cpp'
--- src/scopes/internal/SearchReplyImpl.cpp	2016-02-25 16:08:01 +0000
+++ src/scopes/internal/SearchReplyImpl.cpp	2016-06-28 08:18:23 +0000
@@ -170,7 +170,12 @@
     return true;
 }
 
-bool SearchReplyImpl::push(unity::scopes::Filters const& filters, unity::scopes::FilterState const& filter_state)
+bool SearchReplyImpl::push(unity::scopes::Filters const& filters, unity::scopes::FilterState const&)
+{
+    return push(filters);
+}
+
+bool SearchReplyImpl::push(unity::scopes::Filters const& filters)
 {
     // basic consistency check
     try
@@ -186,7 +191,6 @@
     {
         lock_guard<mutex> lock(mutex_);
         cached_filters_ = filters;
-        cached_filter_state_ = filter_state;
     }
 
     VariantMap var;
@@ -196,7 +200,6 @@
         var["filter_groups"] = filter_groups;
     }
     var["filters"] = internal::FilterBaseImpl::serialize_filters(filters);
-    var["filter_state"] = filter_state.serialize();
     return ReplyImpl::push(var);
 }
 
@@ -260,7 +263,6 @@
             departments = cached_departments_->serialize();
         }
         auto filters = internal::FilterBaseImpl::serialize_filters(cached_filters_);
-        auto filter_state = cached_filter_state_.serialize();
         VariantArray categories = cat_registry_->serialize();
         VariantArray results;
         for (auto const& r : cached_results_)
@@ -279,7 +281,6 @@
         }
 
         vm["filters"] = move(filters);
-        vm["filter_state"] = move(filter_state);
 
         vm["results"] = move(results);
         string const json = Variant(move(vm)).serialize_json();
@@ -371,13 +372,6 @@
         }
         auto filter_array = it->second.get_array();
 
-        it = vm.find("filter_state");
-        if (it == vm.end())
-        {
-            throw unity::scopes::NotFoundException("malformed cache file", "filter_state");
-        }
-        auto filter_state_dict = it->second.get_dict();
-
         it = vm.find("results");
         if (it == vm.end())
         {
@@ -408,8 +402,7 @@
         }
 
         auto filters = FilterBaseImpl::deserialize_filters(move(filter_array), groups);
-        auto filter_state = FilterStateImpl::deserialize(move(filter_state_dict));
-        push(filters, filter_state);
+        push(filters);
 
         for (auto const& r : result_array)
         {
 
=== modified file 'src/scopes/testing/InProcessBenchmark.cpp'
--- src/scopes/testing/InProcessBenchmark.cpp	2015-01-09 07:10:07 +0000
+++ src/scopes/testing/InProcessBenchmark.cpp	2016-06-28 08:18:23 +0000
@@ -204,6 +204,11 @@
         return true;
     }
 
+    bool push(unity::scopes::Filters const&) override
+    {
+        return true;
+    }
+
     bool push(unity::scopes::experimental::Annotation const&) override
     {
         return true;
 
=== modified file 'src/scopes/utility/internal/BufferedSearchReplyImpl.cpp'
--- src/scopes/utility/internal/BufferedSearchReplyImpl.cpp	2015-02-27 05:28:09 +0000
+++ src/scopes/utility/internal/BufferedSearchReplyImpl.cpp	2016-06-28 08:18:23 +0000
@@ -92,6 +92,11 @@
     return upstream_->push(filters, filter_state);
 }
 
+bool BufferedSearchReplyImpl::push(unity::scopes::Filters const& filters)
+{
+    return upstream_->push(filters);
+}
+
 void BufferedSearchReplyImpl::push_surfacing_results_from_cache() noexcept
 {
     upstream_->push_surfacing_results_from_cache();
 
=== modified file 'test/gtest/scopes/Filters/Filters_test.cpp'
--- test/gtest/scopes/Filters/Filters_test.cpp	2016-03-03 11:46:25 +0000
+++ test/gtest/scopes/Filters/Filters_test.cpp	2016-06-28 08:18:23 +0000
@@ -66,12 +66,19 @@
 class SearchReceiver : public SearchListenerBase, public WaitUntilFinished
 {
 public:
-    virtual void push(CategorisedResult /* result */) override {}
-
-    virtual void push(Filters const& filters, FilterState const& filter_state) override
+    virtual void push(CategorisedResult result) override
+    {
+        result_uri = result.uri();
+    }
+
+    virtual void push(Filters const& filters, FilterState const&) override
+    {
+        this->filters2 = filters;
+    }
+
+    virtual void push(Filters const& filters) override
     {
         this->filters = filters;
-        this->filter_state = filter_state;
     }
 
     virtual void finished(CompletionDetails const& details) override
@@ -81,7 +88,8 @@
     }
 
     Filters filters;
-    FilterState filter_state;
+    Filters filters2;
+    std::string result_uri;
 };
 
 template <typename ScopeType>
@@ -135,8 +143,13 @@
     auto ctrl = scope->search("test", hints, receiver);
     receiver->wait_until_finished();
 
-    auto filter_state = receiver->filter_state; // copy filter state, it will be sent with 2nd query
+    EXPECT_EQ("no options active", receiver->result_uri);
+
+    FilterState filter_state;
     {
+        // make sure the deprecated API push method for filters still works
+        ASSERT_EQ(1u, receiver->filters2.size());
+
         auto filters = receiver->filters;
         ASSERT_EQ(1u, filters.size());
         EXPECT_EQ("f1", filters.front()->id());
@@ -159,11 +172,9 @@
     receiver->wait_until_finished();
     {
         auto filters = receiver->filters;
-        auto filter_state2 = receiver->filter_state;
+        ASSERT_EQ(1u, filters.size());
         auto selector = std::dynamic_pointer_cast<const OptionSelectorFilter>(filters.front());
-        ASSERT_EQ(1u, selector->active_options(filter_state2).size());
-        auto option1 = *(selector->active_options(filter_state2).begin());
-        EXPECT_EQ("o1", option1->id());
+        EXPECT_EQ("option o1 active", receiver->result_uri);
     }
 }
 
 
=== modified file 'test/gtest/scopes/Filters/TestScope.h'
--- test/gtest/scopes/Filters/TestScope.h	2016-02-25 16:08:01 +0000
+++ test/gtest/scopes/Filters/TestScope.h	2016-06-28 08:18:23 +0000
@@ -21,6 +21,7 @@
 #include <unity/scopes/CategorisedResult.h>
 #include <unity/scopes/Category.h>
 #include <unity/scopes/FilterGroup.h>
+#include <unity/scopes/FilterOption.h>
 #include <unity/scopes/OptionSelectorFilter.h>
 #include <unity/scopes/ScopeBase.h>
 #include <unity/scopes/SearchReply.h>
@@ -49,11 +50,27 @@
         filter->add_option("o2", "Option 2");
         filters.push_back(filter);
         auto active_opts = filter->active_options(query().filter_state());
-        reply->push(filters, query().filter_state()); // send unmodified state back
+
+        if (query().query_string() == "test")
+        {
+            reply->push(filters);
+        }
+        else
+        {
+            // use the deprecated push call
+            reply->push(filters, query().filter_state());
+        }
 
         auto cat = reply->register_category("cat1", "Category 1", "");
         CategorisedResult res(cat);
-        res.set_uri("uri");
+        if (active_opts.size() == 1)
+        {
+            res.set_uri("option " + (*(active_opts.begin()))->id() + " active");
+        }
+        else
+        {
+            res.set_uri("no options active");
+        }
         res.set_dnd_uri("dnd_uri");
         reply->push(res);
     }
 
=== modified file 'test/gtest/scopes/ResultCache/ResultCache_test.cpp'
--- test/gtest/scopes/ResultCache/ResultCache_test.cpp	2016-02-25 17:28:22 +0000
+++ test/gtest/scopes/ResultCache/ResultCache_test.cpp	2016-06-28 08:18:23 +0000
@@ -46,11 +46,10 @@
         dept_ = parent;
     }
 
-    virtual void push(Filters const& filters, FilterState const& filter_state) override
+    virtual void push(Filters const& filters) override
     {
         lock_guard<mutex> lock(mutex_);
         filters_ = filters;
-        filter_state_ = filter_state;
     }
 
     virtual void push(CategorisedResult result) override
@@ -86,12 +85,6 @@
         return filters_;
     }
 
-    FilterState filter_state() const
-    {
-        lock_guard<mutex> lock(mutex_);
-        return filter_state_;
-    }
-
     CategorisedResult::SCPtr result() const
     {
         lock_guard<mutex> lock(mutex_);
@@ -101,7 +94,6 @@
 private:
     Department::SCPtr dept_;
     Filters filters_;
-    FilterState filter_state_;
     CategorisedResult::SCPtr result_;
     bool query_complete_;
     mutable mutex mutex_;
@@ -248,9 +240,6 @@
     auto osf = dynamic_pointer_cast<OptionSelectorFilter const>(f);
     ASSERT_TRUE(osf != nullptr);
     EXPECT_EQ("Choose an option", osf->label());
-    auto fs = receiver->filter_state();
-    EXPECT_TRUE(fs.has_filter("f1"));
-    EXPECT_TRUE(osf->has_active_option(fs));
 
     // Cache file must still be there.
     boost::system::error_code ec;
@@ -290,9 +279,6 @@
     auto osf = dynamic_pointer_cast<OptionSelectorFilter const>(f);
     ASSERT_TRUE(osf != nullptr);
     EXPECT_EQ("Choose an option", osf->label());
-    auto fs = receiver->filter_state();
-    EXPECT_TRUE(fs.has_filter("f1"));
-    EXPECT_TRUE(osf->has_active_option(fs));
 
     // Cache file must still be there.
     boost::system::error_code ec;
@@ -344,9 +330,6 @@
     auto osf = dynamic_pointer_cast<OptionSelectorFilter const>(f);
     ASSERT_TRUE(osf != nullptr);
     EXPECT_EQ("Choose an option", osf->label());
-    auto fs = receiver->filter_state();
-    EXPECT_TRUE(fs.has_filter("f1"));
-    EXPECT_TRUE(osf->has_active_option(fs));
 
     // New cache file must have been created
     boost::system::error_code ec;
@@ -414,20 +397,6 @@
     EXPECT_TRUE(boost::filesystem::exists(TEST_RUNTIME_PATH "/unconfined/CacheScope/.surfacing_cache", ec));
 }
 
-TEST_F(CacheScopeTest, missing_filter_state)
-{
-    // Get coverage on missing filters_state entry
-    system("cp " TEST_SOURCE_PATH "/no_filter_state_cache " TEST_RUNTIME_PATH "/unconfined/CacheScope/.surfacing_cache");
-    auto receiver = make_shared<Receiver>();
-    scope()->search("", SearchMetadata("unused", "unused"), receiver);
-    receiver->wait_until_finished();
-    EXPECT_EQ(receiver->result(), nullptr);
-
-    // Cache file must still be there, but decode will have failed.
-    boost::system::error_code ec;
-    EXPECT_TRUE(boost::filesystem::exists(TEST_RUNTIME_PATH "/unconfined/CacheScope/.surfacing_cache", ec));
-}
-
 TEST_F(CacheScopeTest, missing_results)
 {
     // Get coverage on missing results entry
 
=== removed file 'test/gtest/scopes/ResultCache/no_filter_state_cache'
--- test/gtest/scopes/ResultCache/no_filter_state_cache	2015-02-05 09:35:42 +0000
+++ test/gtest/scopes/ResultCache/no_filter_state_cache	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-{"categories":[{"icon":"","id":"CacheScope","renderer_template":"\n    {\n        \"schema-version\":1,\n        \"template\":\n        {\n            \"category-layout\":\"grid\"\n        },\n        \"components\":\n        {\n            \"title\":\"title\",\n            \"art\":\"art\"\n        }\n    }\n    ","title":""}],"departments":{"departments":[{"label":"Sub","query":{"department_id":"sub","filter_state":{},"query_string":"","scope":"CacheScope"}}],"label":"Top","query":{"department_id":"","filter_state":{},"query_string":"","scope":"CacheScope"}},"filters":[{"filter_type":"option_selector","id":"f1","label":"Choose an option","multi_select":false,"options":[{"id":"o1","label":"Option 1"},{"id":"o2","label":"Option 2"}]}],"results":[{"attrs":{"title":"","uri":"uri"},"internal":{"cat_id":"CacheScope"}}]}