Merge lp:~alecu/unity-scope-click/force-finished into lp:unity-scope-click/devel

[Alejandro J. Cura]

Force calling .finished() when the last search result is pushed

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:232
http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-ci/34/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-amd64-ci/9
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/8
        deb: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-armhf-ci/8/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-team-unity-scope-click-devel-utopic-i386-ci/8

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-team-unity-scope-click-devel-ci/34/rebuild

review: Approve (continuous-integration)

[Alejandro J. Cura (alecu)]

Just tried the .deb created by jenkins above on image #7 on mako, and the branch fixes the issue that dobey found.

[dobey (dobey)]

review: Approve

[Michal Hruby (mhr3)]

I don't like this, it's just hiding that you're actually leaking the SearchReply + the instance that contains it.

[Alejandro J. Cura (alecu)]

I agree with mhr3's comment, but I intended this as a workaround so we can unblock the landing.
If it's ok with you, we can open a new bug to trace the leak.

=== modified file 'scope/click/query.cpp'
--- scope/click/query.cpp	2014-04-16 21:49:04 +0000
+++ scope/click/query.cpp	2014-05-02 00:03:08 +0000
@@ -160,6 +160,11 @@
     return searchReply->push(res);
 }
 
+void click::Query::finished(const scopes::SearchReplyProxy &searchReply)
+{
+    searchReply->finished();
+}
+
 scopes::Category::SCPtr click::Query::register_category(const scopes::SearchReplyProxy &searchReply,
                                                         const std::string &id,
                                                         const std::string &title,
@@ -215,6 +220,8 @@
                     qDebug() << "no reason to catch";
                 }
             }
+            qDebug() << "search completed";
+            this->finished(searchReply);
         });
 
     });

=== modified file 'scope/click/query.h'
--- scope/click/query.h	2014-04-24 16:03:18 +0000
+++ scope/click/query.h	2014-05-02 00:03:08 +0000
@@ -70,7 +70,7 @@
     };
 
     Query(std::string const& query, click::Index& index, scopes::SearchMetadata const& metadata);
-    ~Query();
+    virtual ~Query();
 
     virtual void cancelled() override;
 
@@ -79,6 +79,7 @@
 protected:
     virtual void add_available_apps(const scopes::SearchReplyProxy &searchReply, const std::set<std::string> &locallyInstalledApps, const std::string &category);
     virtual bool push_result(const scopes::SearchReplyProxy &searchReply, scopes::CategorisedResult const& res);
+    virtual void finished(const scopes::SearchReplyProxy &searchReply);
     virtual void push_local_results(scopes::SearchReplyProxy const &replyProxy,
                                     std::vector<click::Application> const &apps,
                                     std::string& categoryTemplate);

=== modified file 'scope/tests/test_query.cpp'
--- scope/tests/test_query.cpp	2014-04-16 21:49:04 +0000
+++ scope/tests/test_query.cpp	2014-05-02 00:03:08 +0000
@@ -104,6 +104,7 @@
         add_available_apps(searchReply, locallyInstalledApps, categoryTemplate);
     }
     MOCK_METHOD2(push_result, bool(scopes::SearchReplyProxy const&, scopes::CategorisedResult const&));
+    MOCK_METHOD1(finished, void(scopes::SearchReplyProxy const&));
     MOCK_METHOD5(register_category, scopes::Category::SCPtr(const scopes::SearchReplyProxy &searchReply,
                                                             const std::string &id,
                                                             const std::string &title,
@@ -175,6 +176,26 @@
     q.wrap_add_available_apps(reply, no_installed_packages, FAKE_CATEGORY_TEMPLATE);
 }
 
+TEST(QueryTest, testAddAvailableAppsCallsFinished)
+{
+    click::PackageList packages {
+        {"name", "title", 0.0, "", ""}
+    };
+    MockIndex mock_index(packages);
+    scopes::SearchMetadata metadata("en_EN", "phone");
+    std::set<std::string> no_installed_packages;
+    MockQuery q(FAKE_QUERY, mock_index, metadata);
+    EXPECT_CALL(mock_index, do_search(FAKE_QUERY, _));
+
+    scopes::CategoryRenderer renderer("{}");
+    auto ptrCat = std::make_shared<FakeCategory>("id", "", "", renderer);
+    EXPECT_CALL(q, register_category(_, _, _, _, _)).WillOnce(Return(ptrCat));
+
+    scopes::SearchReplyProxy reply;
+    EXPECT_CALL(q, finished(_));
+    q.wrap_add_available_apps(reply, no_installed_packages, FAKE_CATEGORY_TEMPLATE);
+}
+
 TEST(QueryTest, testAddAvailableAppsWithNullCategory)
 {
     click::PackageList packages {