Merge lp:~alecu/unity-scope-click/fix-empty-variant into lp:unity-scope-click

[Alejandro J. Cura]

Check Variant for null value

[dobey (dobey)]

Looks ok.

review: Approve

[Michi Henning (michihenning)]

Thanks for the fix, thanks!

I'm wondering about a more general issue here though. Basically, what happens is that, during destruction of an object, an exception is thrown, which brings down the process. I take it that this is simply the destruction of an object that is used by the scope internally?

As best as I know, any methods that are called by the scopes run time are exception-safe so, if scope throws an exception from anything that is called by the run time, things should be safe. If this is not the case, and the object is being destroyed within the context of a thread that is owned by the run time, I need to know so I can fix it :-)

In general, whenever I have a non-trivial destructor, I have a try-catch block around the code that, if an exception is thrown, logs the exception. If the program is no longer in a sane state after that, I call abort(). Something like:

SomeObject::~SomeObject()
{
    try
    {
        something_->func();
    }
    catch (std::exception const& e)
    {
        LOG(string("~SomeObject, func() threw ") + e.what();
        // call abort() here if it's not possible to recover.
    }
    catch (...)
    {
        LOG("~SomeObject, func() unknown exception");
        // call abort() here if it's not possible to recover.
    }
}

review: Needs Information

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:304
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/561/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-vivid/389
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-vivid-amd64-ci/17
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scope-click-vivid-armhf-ci/17
        deb: http://jenkins.qa.ubuntu.com/job/unity-scope-click-vivid-armhf-ci/17/artifact/work/output/*zip*/output.zip
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-vivid/319
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/397
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-amd64/397/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Michi Henning (michihenning)]

Looks right.

review: Approve

=== modified file 'libclickscope/click/index.h'
--- libclickscope/click/index.h	2014-10-01 03:37:34 +0000
+++ libclickscope/click/index.h	2015-01-13 22:19:09 +0000
@@ -74,6 +74,7 @@
 
 public:
     enum class Error {NoError, CredentialsError, NetworkError};
+    Index() {}
     Index(const QSharedPointer<click::web::Client>& client,
           const QSharedPointer<Configuration> configuration=QSharedPointer<Configuration>(new Configuration()));
     virtual std::pair<Packages, Packages> package_lists_from_json(const std::string& json);

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2014-10-22 19:32:37 +0000
+++ libclickscope/click/preview.cpp	2015-01-13 22:19:09 +0000
@@ -254,6 +254,22 @@
     return b.str();
 }
 
+void PreviewStrategy::run_under_qt(const std::function<void ()> &task)
+{
+    qt::core::world::enter_with_task([task]() {
+        task();
+    });
+}
+
+std::string get_string_maybe_null(scopes::Variant variant)
+{
+    if (variant.is_null()) {
+        return "";
+    } else {
+        return variant.get_string();
+    }
+}
+
 // TODO: error handling - once get_details provides errors, we can
 // return them from populateDetails and check them in the calling code
 // to decide whether to show error widgets. see bug LP: #1289541
@@ -262,15 +278,15 @@
                                                     click::Reviews::Error)> reviews_callback)
 {
 
-    std::string app_name = result["name"].get_string();
+    std::string app_name = get_string_maybe_null(result["name"]);
 
     if (app_name.empty()) {
         click::PackageDetails details;
         qDebug() << "in populateDetails(), app_name is empty";
         details.package.title = result.title();
         details.package.icon_url = result.art();
-        details.description = result["description"].get_string();
-        details.main_screenshot_url = result["main_screenshot"].get_string();
+        details.description = get_string_maybe_null(result["description"]);
+        details.main_screenshot_url = get_string_maybe_null(result["main_screenshot"]);
         details_callback(details);
         reviews_callback(click::ReviewList(), click::Reviews::Error::NoError);
     } else {
@@ -278,7 +294,7 @@
         // I think this should not be required when we switch the click::Index over
         // to using the Qt bridge. With that, the qt dependency becomes an implementation detail
         // and code using it does not need to worry about threading/event loop topics.
-        qt::core::world::enter_with_task([this, details_callback, reviews_callback, app_name]()
+        run_under_qt([this, details_callback, reviews_callback, app_name]()
             {
                 index_operation = index->get_details(app_name, [this, app_name, details_callback, reviews_callback](PackageDetails details, click::Index::Error error){
                     if(error == click::Index::Error::NoError) {
@@ -289,8 +305,8 @@
                         click::PackageDetails details;
                         details.package.title = result.title();
                         details.package.icon_url = result.art();
-                        details.description = result["description"].get_string();
-                        details.main_screenshot_url = result["main_screenshot"].get_string();
+                        details.description = get_string_maybe_null(result["description"]);
+                        details.main_screenshot_url = get_string_maybe_null(result["main_screenshot"]);
                         details_callback(details);
                     }
                     reviews_operation = reviews->fetch_reviews(app_name,

=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h	2014-10-16 21:25:16 +0000
+++ libclickscope/click/preview.h	2015-01-13 22:19:09 +0000
@@ -149,6 +149,7 @@
     virtual std::string build_other_metadata(const PackageDetails& details);
     virtual std::string build_updates_table(const PackageDetails& details);
     virtual std::string build_whats_new(const PackageDetails& details);
+    virtual void run_under_qt(const std::function<void ()> &task);
 
     scopes::Result result;
     QSharedPointer<click::web::Client> client;

=== modified file 'libclickscope/click/reviews.h'
--- libclickscope/click/reviews.h	2014-05-26 14:02:45 +0000
+++ libclickscope/click/reviews.h	2015-01-13 22:19:09 +0000
@@ -72,10 +72,11 @@
     QSharedPointer<web::Client> client;
 public:
     enum class Error {NoError, CredentialsError, NetworkError};
+    Reviews() {}
     Reviews(const QSharedPointer<click::web::Client>& client);
     virtual ~Reviews();
 
-    click::web::Cancellable fetch_reviews (const std::string& package_name,
+    virtual click::web::Cancellable fetch_reviews (const std::string& package_name,
                                            std::function<void(ReviewList, Error)> callback);
     click::web::Cancellable submit_review (const Review& review,
                                            std::function<void(Error)> callback);

=== modified file 'libclickscope/tests/test_preview.cpp'
--- libclickscope/tests/test_preview.cpp	2014-10-17 19:55:56 +0000
+++ libclickscope/tests/test_preview.cpp	2015-01-13 22:19:09 +0000
@@ -33,6 +33,8 @@
 #include <gtest/gtest.h>
 #include <click/preview.h>
 #include <fake_json.h>
+#include <click/index.h>
+#include <click/reviews.h>
 
 using namespace ::testing;
 using namespace unity::scopes;
@@ -46,23 +48,57 @@
 
 };
 
+class FakeIndex : public click::Index
+{
+public:
+    FakeIndex() {
+
+    }
+    click::web::Cancellable get_details(const std::string& /*package_name*/, std::function<void(click::PackageDetails, Error)> callback) override {
+        callback(click::PackageDetails(), Error::NetworkError);
+        return click::web::Cancellable();
+    }
+
+};
+
+class FakeReviews : public click::Reviews
+{
+public:
+    FakeReviews() {
+
+    }
+
+    click::web::Cancellable fetch_reviews(const std::string &/*package_name*/, std::function<void (click::ReviewList, Error)> callback) override {
+        callback(click::ReviewList(), Error::NoError);
+        return click::web::Cancellable();
+    }
+};
+
 class FakePreview : public click::PreviewStrategy
 {
 public:
     FakePreview(Result& result) : click::PreviewStrategy::PreviewStrategy(result)
     {
-
+        index.reset(new FakeIndex());
+        reviews.reset(new FakeReviews());
     }
 
     void run(const PreviewReplyProxy &/*reply*/)
     {
 
     }
+
+    void run_under_qt(const std::function<void()> &task) {
+        // when testing, do not actually run under qt
+        task();
+    }
+
     using click::PreviewStrategy::screenshotsWidgets;
     using click::PreviewStrategy::descriptionWidgets;
     using click::PreviewStrategy::build_other_metadata;
     using click::PreviewStrategy::build_updates_table;
     using click::PreviewStrategy::build_whats_new;
+    using click::PreviewStrategy::populateDetails;
 };
 
 class PreviewsBaseTest : public Test
@@ -103,6 +139,19 @@
     ASSERT_EQ(sources[2].get_string(), "sshot3");
 }
 
+TEST_F(PreviewStrategyTest, testEmptyResults)
+{
+    FakeResult result{vm};
+    result["name"] = "fake name";
+    FakePreview preview{result};
+    preview.populateDetails(
+                [](const click::PackageDetails &){
+                },
+                [](const click::ReviewList&, click::Reviews::Error){
+                });
+
+}
+
 class PreviewStrategyDescriptionTest : public PreviewStrategyTest
 {
 public: