Merge lp:~stolowski/unity-scope-click/two-columns-layout into lp:unity-scope-click

[Paweł Stołowski]

Support two column layouts in the previews.

Support two column layouts in the previews.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:409
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/694/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/98/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/94/console

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

review: Needs Fixing (continuous-integration)

[lp:~stolowski/unity-scope-click/two-columns-layout]

410. By Paweł Stołowski on 2016-01-26

Fixed InstallingPreview

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:410
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/695/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/99/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/95/console

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

review: Needs Fixing (continuous-integration)

[dobey (dobey)]

I'm a bit worried that we have multiple new methods being added here, and some changes to some previews, but there are no changes to the tests. Can we get some tests added for the new methods, and tests that ensure the correct version of the pushPackagePreviewWidgets() method is called for the different preview types, perhaps?

review: Needs Fixing

[lp:~stolowski/unity-scope-click/two-columns-layout]

411. By Paweł Stołowski on 2016-01-27

pushWidgets method not needed after all

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:411
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/696/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/100/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/96/console

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

review: Needs Fixing (continuous-integration)

[lp:~stolowski/unity-scope-click/two-columns-layout]

412. By Paweł Stołowski on 2016-01-28

Check that register_layout is called

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:412
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/698/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/102/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/98/console

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

review: Needs Fixing (continuous-integration)

[lp:~stolowski/unity-scope-click/two-columns-layout]

413. By Paweł Stołowski on 2016-01-28

Basic check for widget caching

414. By Paweł Stołowski on 2016-01-28

Real test for column layout and cache

415. By Paweł Stołowski on 2016-01-28

Match preview widget ids too

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:414
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/699/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/103/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/99/console

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

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

> I'm a bit worried that we have multiple new methods being added here, and some
> changes to some previews, but there are no changes to the tests. Can we get
> some tests added for the new methods, and tests that ensure the correct
> version of the pushPackagePreviewWidgets() method is called for the different
> preview types, perhaps?

Ok, I added some tests, mostly for the new pushPackagePreviewWidgets to confirm it does the right thing, plus also added simple checks for calls to register_layout in other two existing tests.

The best test would be to just set call expectations on register_layout() and push() of the reply objects, but that's not possible without spendings tons of time on enhancing current tests. The problem is it's not currently easily possible with InstalledPreview test, because of the entire "run with qt" story in InstalledPreview::run (multiple calls to run various Qt tasks there).

[dobey (dobey)]

Looks ok.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:415
http://jenkins.qa.ubuntu.com/job/unity-scope-click-ci/700/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-amd64-ci/104/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scope-click-wily-armhf-ci/100/console

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

review: Needs Fixing (continuous-integration)

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2015-11-24 18:23:23 +0000
+++ libclickscope/click/preview.cpp	2016-01-28 14:37:47 +0000
@@ -45,6 +45,7 @@
 #include <unity/scopes/PreviewReply.h>
 #include <unity/scopes/Variant.h>
 #include <unity/scopes/VariantBuilder.h>
+#include <unity/scopes/ColumnLayout.h>
 
 #include <QDebug>
 
@@ -56,6 +57,70 @@
 
 namespace click {
 
+void CachedPreviewWidgets::push(unity::scopes::PreviewWidget const &widget)
+{
+    widgets.push_back(widget);
+    widgets_lookup.insert(widget.id());
+}
+
+void CachedPreviewWidgets::push(unity::scopes::PreviewWidgetList const &widgetList)
+{
+    for (auto const& widget: widgetList)
+    {
+        push(widget);
+    }
+}
+
+void CachedPreviewWidgets::flush(unity::scopes::PreviewReplyProxy const& reply)
+{
+    // A safeguard: if a new widget gets added but missing in the layout, we will get a warning
+    // in the logs and layouts will not be registered (single column with all widgets will be used).
+    if (widgets.size() != layout.singleColumn.column1.size() ||
+            widgets.size() != layout.twoColumns.column1.size() + layout.twoColumns.column2.size())
+    {
+        qWarning() << "Number of column layouts doesn't match the number of widgets";
+    }
+    else
+    {
+        layout.registerLayouts(reply);
+    }
+    reply->push(widgets);
+    widgets.clear();
+    widgets_lookup.clear();
+}
+
+bool CachedPreviewWidgets::has(std::string const& widget) const
+{
+    return widgets_lookup.find(widget) != widgets_lookup.end();
+}
+
+void WidgetsInColumns::registerLayouts(unity::scopes::PreviewReplyProxy const& reply)
+{
+    unity::scopes::ColumnLayout layout1col(1);
+    layout1col.add_column(singleColumn.column1);
+
+    unity::scopes::ColumnLayout layout2col(2);
+    layout2col.add_column(twoColumns.column1);
+    layout2col.add_column(twoColumns.column2);
+
+    try
+    {
+        reply->register_layout({layout1col, layout2col});
+    }
+    catch (unity::LogicException const& e)
+    {
+        qWarning() << "Failed to register layout:" << QString::fromStdString(e.what());
+    }
+}
+
+void WidgetsInColumns::appendToColumn(std::vector<std::string>& column, unity::scopes::PreviewWidgetList const& widgets)
+{
+    for (auto const& widget: widgets)
+    {
+        column.push_back(widget.id());
+    }
+}
+
 DepartmentUpdater::DepartmentUpdater(const std::shared_ptr<click::DepartmentsDb>& depts)
     : depts(depts)
 {
@@ -237,6 +302,39 @@
     reply->push(descriptionWidgets(details));
 }
 
+void PreviewStrategy::pushPackagePreviewWidgets(CachedPreviewWidgets &cache,
+                                const PackageDetails& details,
+                                const scopes::PreviewWidgetList& button_area_widgets)
+{
+    cache.push(headerWidgets(details));
+    cache.layout.singleColumn.column1.push_back("hdr");
+    cache.layout.twoColumns.column1.push_back("hdr");
+
+    cache.push(button_area_widgets);
+    cache.layout.appendToColumn(cache.layout.singleColumn.column1, button_area_widgets);
+    cache.layout.appendToColumn(cache.layout.twoColumns.column1, button_area_widgets);
+
+    auto const screenshots = screenshotsWidgets(details);
+    cache.push(screenshots);
+    cache.layout.appendToColumn(cache.layout.singleColumn.column1, screenshots);
+    cache.layout.appendToColumn(cache.layout.twoColumns.column1, screenshots);
+
+    auto descr = descriptionWidgets(details);
+    if (!descr.empty())
+    {
+        cache.push(descr);
+        cache.layout.appendToColumn(cache.layout.singleColumn.column1, descr);
+
+        // for two-columns we need to split the widgets, summary goes to 1st column, everything else to 2nd
+        if (descr.front().id() == "summary")
+        {
+            descr.pop_front();
+            cache.layout.twoColumns.column1.push_back("summary");
+        }
+        cache.layout.appendToColumn(cache.layout.twoColumns.column2, descr);
+    }
+}
+
 PreviewStrategy::~PreviewStrategy()
 {
 }
@@ -463,7 +561,7 @@
 {
     scopes::PreviewWidgetList widgets;
 
-    scopes::PreviewWidget rating("summary", "reviews");
+    scopes::PreviewWidget rating("reviews", "reviews");
     scopes::VariantBuilder builder;
 
     if (reviewlist.size() > 0) {
@@ -647,7 +745,7 @@
                           if (login_error) {
                               reply->push(loginErrorWidgets(details));
                           } else {
-                              pushPackagePreviewWidgets(reply, details, progressBarWidget(object_path));
+                              pushPackagePreviewWidgets(cachedWidgets, details, progressBarWidget(object_path));
                               startLauncherAnimation(details);
                           }
                       },
@@ -655,11 +753,15 @@
                                     click::Reviews::Error error) {
                           if (!login_error) {
                               if (error == click::Reviews::Error::NoError) {
-                                  reply->push(reviewsWidgets(reviewlist));
+                                  auto const revs = reviewsWidgets(reviewlist);
+                                  cachedWidgets.push(revs);
+                                  cachedWidgets.layout.appendToColumn(cachedWidgets.layout.singleColumn.column1, revs);
+                                  cachedWidgets.layout.appendToColumn(cachedWidgets.layout.twoColumns.column1, revs);
                               } else {
                                   qDebug() << "There was an error getting reviews for:" << result["name"].get_string().c_str();
                               }
                           }
+                          cachedWidgets.flush(reply);
                           reply->finished();
                       });
                   break;
@@ -732,7 +834,7 @@
     review.rating = 0;
     std::string widget_id;
     // We use a try/catch here, as scope_data() can be a dict, but not have
-    // the values we need, which will result in an exception thrown. 
+    // the values we need, which will result in an exception thrown.
     try {
         auto metadict = metadata.scope_data().get_dict();
         review.rating = metadict["rating"].get_int();
@@ -801,7 +903,7 @@
     getApplicationUri(manifest, [this, reply, manifest, app_name, &review, userid](const std::string& uri) {
             populateDetails([this, reply, uri, manifest, app_name](const PackageDetails &details){
                 store_department(details);
-                pushPackagePreviewWidgets(reply, details, createButtons(uri, manifest));
+                pushPackagePreviewWidgets(cachedWidgets, details, createButtons(uri, manifest));
             },
             [this, reply, &review, manifest, userid](const ReviewList& reviewlist,
                           click::Reviews::Error error) {
@@ -828,13 +930,19 @@
                         review_input.push_back(rating);
                     }
                 }
-                reply->push(review_input);
+                cachedWidgets.push(review_input);
+                cachedWidgets.layout.appendToColumn(cachedWidgets.layout.singleColumn.column1, review_input);
+                cachedWidgets.layout.appendToColumn(cachedWidgets.layout.twoColumns.column1, review_input);
 
                 if (error == click::Reviews::Error::NoError) {
-                    reply->push(reviewsWidgets(reviews));
+                    auto const revs = reviewsWidgets(reviews);
+                    cachedWidgets.push(revs);
+                    cachedWidgets.layout.appendToColumn(cachedWidgets.layout.singleColumn.column1, revs);
+                    cachedWidgets.layout.appendToColumn(cachedWidgets.layout.twoColumns.column1, revs);
                 } else {
                     qDebug() << "There was an error getting reviews for:" << result["name"].get_string().c_str();
                 }
+                cachedWidgets.flush(reply);
                 reply->finished();
         });
     });
@@ -972,7 +1080,6 @@
         });
 }
 
-
 scopes::PreviewWidgetList PurchasingPreview::purchasingWidgets(const PackageDetails &/*details*/)
 {
     scopes::PreviewWidgetList widgets;
@@ -1130,14 +1237,18 @@
                     button_widgets = progressBarWidget(found_object_path);
                 }
                 qDebug() << "Pushed button action widgets.";
-                pushPackagePreviewWidgets(reply, found_details, button_widgets);
+                pushPackagePreviewWidgets(cachedWidgets, found_details, button_widgets);
                 qDebug() << "Pushed package details widgets.";
                 if (reviewserror == click::Reviews::Error::NoError) {
                     qDebug() << "Pushing reviews widgets.";
-                    reply->push(reviewsWidgets(reviewlist));
+                    auto const revs = reviewsWidgets(reviewlist);
+                    cachedWidgets.push(revs);
+                    cachedWidgets.layout.appendToColumn(cachedWidgets.layout.singleColumn.column1, revs);
+                    cachedWidgets.layout.appendToColumn(cachedWidgets.layout.twoColumns.column1, revs);
                 } else {
                     qDebug() << "There was an error getting reviews for:" << result["name"].get_string().c_str();
                 }
+                cachedWidgets.flush(reply);
                 reply->finished();
                 qDebug() << "---------- Finished reply for:" << result["name"].get_string().c_str();
             });

=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h	2015-11-24 18:23:23 +0000
+++ libclickscope/click/preview.h	2016-01-28 14:37:47 +0000
@@ -45,6 +45,7 @@
 #include <unity/scopes/Result.h>
 #include <unity/scopes/ScopeBase.h>
 #include <unity/util/DefinesPtrs.h>
+#include <set>
 
 namespace scopes = unity::scopes;
 
@@ -54,6 +55,35 @@
 class PreviewStrategy;
 class DepartmentsDb;
 
+struct WidgetsInColumns
+{
+    struct {
+        std::vector<std::string> column1;
+    } singleColumn;
+    struct {
+        std::vector<std::string> column1;
+        std::vector<std::string> column2;
+    } twoColumns;
+
+    void registerLayouts(unity::scopes::PreviewReplyProxy const& reply);
+    void appendToColumn(std::vector<std::string>& column, unity::scopes::PreviewWidgetList const& widgets);
+};
+
+class CachedPreviewWidgets
+{
+public:
+    void push(unity::scopes::PreviewWidget const &widget);
+    void push(unity::scopes::PreviewWidgetList const &widgetList);
+    void flush(unity::scopes::PreviewReplyProxy const& reply);
+    bool has(std::string const& widget) const;
+
+    WidgetsInColumns layout;
+
+private:
+    std::list<unity::scopes::PreviewWidget> widgets;
+    std::unordered_set<std::string> widgets_lookup;
+};
+
 class DepartmentUpdater
 {
 protected:
@@ -137,6 +167,10 @@
 
     virtual void cancelled();
     virtual void run(unity::scopes::PreviewReplyProxy const& reply) = 0;
+    virtual void pushPackagePreviewWidgets(CachedPreviewWidgets &reply,
+                                           const PackageDetails& details,
+                                           const scopes::PreviewWidgetList& button_area_widgets);
+
 protected:
     virtual void populateDetails(std::function<void(const PackageDetails &)> details_callback,
                                  std::function<void(const click::ReviewList&,
@@ -164,6 +198,7 @@
     virtual void invalidateScope(const std::string& scope_id);
 
     scopes::Result result;
+
     QSharedPointer<click::web::Client> client;
     QSharedPointer<click::Index> index;
     click::web::Cancellable index_operation;
@@ -205,6 +240,7 @@
     std::string download_sha512;
     QSharedPointer<click::Downloader> downloader;
     std::shared_ptr<click::DepartmentsDb> depts_db;
+    CachedPreviewWidgets cachedWidgets;
     void startLauncherAnimation(const PackageDetails& details);
 };
 
@@ -228,6 +264,7 @@
                                             const click::Manifest& manifest);
 private:
     scopes::ActionMetadata metadata;
+    CachedPreviewWidgets cachedWidgets;
 };
 
 class InstalledScopePreview : public PreviewStrategy
@@ -272,7 +309,6 @@
     virtual ~UninstallConfirmationPreview();
 
     void run(unity::scopes::PreviewReplyProxy const& reply) override;
-
 };
 
 class UninstalledPreview : public PreviewStrategy, public DepartmentUpdater
@@ -290,6 +326,7 @@
     void run(unity::scopes::PreviewReplyProxy const& reply) override;
 protected:
     PackageDetails found_details;
+    CachedPreviewWidgets cachedWidgets;
     std::string found_object_path;
     virtual click::Downloader* get_downloader(const QSharedPointer<click::network::AccessManager>& nam);
     virtual scopes::PreviewWidgetList uninstalledActionButtonWidgets(const PackageDetails &details);

=== modified file 'libclickscope/tests/test_preview.cpp'
--- libclickscope/tests/test_preview.cpp	2015-11-24 18:23:23 +0000
+++ libclickscope/tests/test_preview.cpp	2016-01-28 14:37:47 +0000
@@ -42,6 +42,7 @@
 #include <boost/locale/time_zone.hpp>
 
 using namespace ::testing;
+using ::testing::Matcher;
 using namespace unity::scopes;
 
 class FakeResult : public Result
@@ -158,6 +159,81 @@
 
 }
 
+MATCHER_P(PreviewWidgetsListMatchers, widgets, "") {
+    if (arg.size() == widgets.size()) {
+        auto it1 = widgets.begin();
+        auto it2 = arg.begin();
+        while (*it1 != it2->id()) {
+            *result_listener << "Preview widgets don't match: " << it2->id() << ", expected " << *it1;
+            return false;
+        }
+        return true;
+    }
+    *result_listener << "Preview widgets list don't match: ";
+    for (auto const& widget: arg) {
+        *result_listener << widget.id() << ", ";
+    }
+    *result_listener << "expected: ";
+    for (auto const& widget: widgets) {
+        *result_listener << widget << ", ";
+    }
+    return false;
+}
+
+MATCHER_P(LayoutMatches, layouts, "") {
+    if (arg.size() != layouts.size()) {
+        *result_listener << "Layout lists sizes don't match, " << arg.size() << " vs " << layouts.size();
+        return false;
+    }
+    auto it1 = layouts.begin();
+    auto it2 = arg.begin();
+    while (it1 != layouts.end()) {
+        if (it1->serialize() != it2->serialize()) {
+            *result_listener << "Layouts don't match: " << unity::scopes::Variant(it1->serialize()).serialize_json() << " vs "
+                << unity::scopes::Variant(it2->serialize()).serialize_json();
+            return false;
+        }
+        it1++;
+        it2++;
+    }
+    return true;
+}
+
+TEST_F(PreviewStrategyTest, testPushCachedWidgets)
+{
+    unity::scopes::testing::MockPreviewReply reply;
+    std::shared_ptr<unity::scopes::testing::MockPreviewReply> replyptr{&reply, [](unity::scopes::testing::MockPreviewReply*){}};
+
+    FakeResult result{vm};
+    FakePreview preview{result};
+    click::PackageDetails details;
+    details.main_screenshot_url = "sshot1";
+    details.license = "GPL";
+    details.company_name = "Ubuntu";
+    details.website = "http://ubuntu.com";
+    details.changelog = "Foo";
+    details.version = "0.1";
+    details.download_url = "http://ubuntu.com/";
+    details.description = "Foo";
+
+    click::CachedPreviewWidgets cache;
+    scopes::PreviewWidget buttons("buttons", "actions");
+
+    unity::scopes::ColumnLayout single_column(1);
+    single_column.add_column({"hdr","buttons","screenshots","summary","other_metadata","updates_table","whats_new"});
+    unity::scopes::ColumnLayout two_columns(2);
+    two_columns.add_column({"hdr","buttons","screenshots","summary"});
+    two_columns.add_column({"other_metadata","updates_table","whats_new"});
+    unity::scopes::ColumnLayoutList expected_layout {single_column, two_columns};
+
+    std::vector<std::string> expected_widgets {"hdr", "buttons", "screenshots", "summary", "other_metadata", "updates_table", "whats_new"};
+
+    EXPECT_CALL(*replyptr, register_layout(Matcher<unity::scopes::ColumnLayoutList const&>(LayoutMatches(expected_layout))));
+    EXPECT_CALL(*replyptr, push(Matcher<unity::scopes::PreviewWidgetList const&>(PreviewWidgetsListMatchers(expected_widgets))));
+    preview.pushPackagePreviewWidgets(cache, details, {buttons});
+    cache.flush(replyptr);
+}
+
 class PreviewStrategyDescriptionTest : public PreviewStrategyTest
 {
 public:
@@ -391,6 +467,7 @@
     EXPECT_CALL(preview, progressBarWidget(_))
             .Times(1)
             .WillOnce(Return(response));
+    EXPECT_CALL(*replyptr, register_layout(_));
     preview.run(replyptr);
     preview.fake_downloader->activate_callback();
 }
@@ -404,6 +481,7 @@
     EXPECT_CALL(preview, uninstalledActionButtonWidgets(_))
             .Times(1)
             .WillOnce(Return(response));
+    EXPECT_CALL(*replyptr, register_layout(_));
     preview.run(replyptr);
     preview.fake_downloader->activate_callback();
 }
@@ -483,6 +561,7 @@
         : click::InstalledPreview(result, metadata, client, pay_package, depts) {
 
     }
+
     using click::InstalledPreview::createButtons;
     MOCK_METHOD0(isRefundable, bool());
 };
@@ -520,7 +599,6 @@
     ASSERT_EQ(get_action_from_widgets(widgets, 0, 1), "uninstall_click");
 }
 
-
 class FakeCancelPurchasePreview : public click::CancelPurchasePreview  {
 public:
     FakeCancelPurchasePreview(const unity::scopes::Result& result, bool installed)