Merge lp:~dobey/unity-scope-click/without-crashing into lp:unity-scope-click

[dobey]

Refactor the code to process Qt events while waiting on future into a template.
Wait for Qt callbacks from downloadmanager before continuing.

[lp:~dobey/unity-scope-click/without-crashing]

443. By dobey on 2016-04-04

Use const references in the error callback args.

444. By dobey on 2016-04-04

Keep the cancellable around.

445. By dobey on 2016-04-05

Block in the preview on getting the progress object path.

446. By dobey on 2016-04-05

Call web::Client::call with full argument list.

447. By dobey on 2016-04-05

Revert the last change.

448. By dobey on 2016-04-05

Fix lambda arg declarations.

449. By dobey on 2016-04-06

Revert the addition of promise/future in preview.

450. By dobey on 2016-04-14

Oops, missed changing one callback to setting the promise.

451. By dobey on 2016-04-14

Use toStdString to convert from QString.

452. By dobey on 2016-04-14

Need to convert for qdebug now.

[Marcus Tomlinson (marcustomlinson)]

Rejecting in favour of https://code.launchpad.net/~marcustomlinson/unity-scope-click/without-crashing/+merge/291987

review: Disapprove

Unmerged revisions

452. By dobey on 2016-04-14

Need to convert for qdebug now.

451. By dobey on 2016-04-14

Use toStdString to convert from QString.

450. By dobey on 2016-04-14

Oops, missed changing one callback to setting the promise.

449. By dobey on 2016-04-06

Revert the addition of promise/future in preview.

448. By dobey on 2016-04-05

Fix lambda arg declarations.

447. By dobey on 2016-04-05

Revert the last change.

446. By dobey on 2016-04-05

Call web::Client::call with full argument list.

445. By dobey on 2016-04-05

Block in the preview on getting the progress object path.

444. By dobey on 2016-04-04

Keep the cancellable around.

443. By dobey on 2016-04-04

Use const references in the error callback args.

=== modified file 'libclickscope/click/download-manager.cpp'
--- libclickscope/click/download-manager.cpp	2016-04-14 18:09:31 +0000
+++ libclickscope/click/download-manager.cpp	2016-04-14 18:09:31 +0000
@@ -28,6 +28,7 @@
  */
 
 #include "download-manager.h"
+#include "qtbridge.h"
 
 #include <QDebug>
 #include <QObject>
@@ -75,11 +76,14 @@
 }
 
 void DownloadManager::get_progress(const std::string& package_name,
-                                   const std::function<void (std::string)>& callback)
+                                   const std::function<void (const std::string&)>& callback)
 {
+    std::promise<std::string> promise;
+    auto future = promise.get_future();
+
     dm->getAllDownloadsWithMetadata(DOWNLOAD_APP_ID_KEY,
                                     QString::fromStdString(package_name),
-                                    [callback, package_name](const QString& /*key*/, const QString& /*value*/, DownloadsList* downloads_list){
+                                    [callback, package_name, &promise](const QString& /*key*/, const QString& /*value*/, DownloadsList* downloads_list){
         // got downloads matching metadata
         std::string object_path;
         auto downloads = downloads_list->downloads();
@@ -92,18 +96,21 @@
         if (downloads.size() > 1) {
             qWarning() << "More than one download with the same object path";
         }
-        callback(object_path);
-    }, [callback, package_name](const QString& /*key*/, const QString& /*value*/, DownloadsList* /*downloads_list*/){
+        promise.set_value(object_path);
+        }, [callback, package_name, &promise](const QString& /*key*/, const QString& /*value*/, DownloadsList* /*downloads_list*/){
         // no downloads found
         qDebug() << "No object path found for package" << QString::fromStdString(package_name);
-        callback("");
+        promise.set_value("");
     });
+    qt::core::world::wait_for_future_ready<std::string>(future);
+    auto result = future.get();
+    callback(result);
 }
 
 click::web::Cancellable DownloadManager::start(const std::string& url,
                                                const std::string& download_sha512,
                                                const std::string& package_name,
-                                               const std::function<void (std::string, Error)>& callback)
+                                               const std::function<void (const std::string&, const Error&)>& callback)
 {
     QSharedPointer<click::web::Response> response = client->call
         (url, "HEAD", true);
@@ -131,21 +138,26 @@
                                                            metadata,
                                                            headers);
 
+                        std::promise<std::pair<std::string, Error>> promise;
+                        auto future = promise.get_future();
                         dm->createDownload(downloadStruct,
-                                           [callback](Download* download) {
+                                           [callback, &promise](Download* download) {
                                                if (download->isError()) {
-                                                   auto error = download->error()->errorString().toUtf8().data();
-                                                   qDebug() << "Received error from ubuntu-download-manager:" << error;
-                                                   callback(error, Error::DownloadInstallError);
+                                                   auto error = download->error()->errorString().toStdString();
+                                                   qDebug() << "Received error from ubuntu-download-manager:" << error.c_str();
+                                                   promise.set_value(std::pair<std::string, Error>{error, Error::DownloadInstallError});
                                                } else {
                                                    download->start();
-                                                   callback(download->id().toUtf8().data(), Error::NoError);
+                                                   promise.set_value(std::pair<std::string, Error>{download->id().toStdString(), Error::NoError});
                                                }
                                            },
-                                           [callback](Download* download) {
-                                               callback(download->error()->errorString().toUtf8().data(),
-                                                        Error::DownloadInstallError);
+                                           [callback, &promise](Download* download) {
+                                               promise.set_value(std::pair<std::string, Error>{download->error()->errorString().toStdString(),
+                                                           Error::DownloadInstallError});
                                            });
+                        qt::core::world::wait_for_future_ready<std::pair<std::string, Error>>(future);
+                        auto result = future.get();
+                        callback(result.first, result.second);
                     } else {
                         std::string error{"Unhandled HTTP response code: "};
                         error += status;
@@ -159,10 +171,11 @@
                          case 401:
                          case 403:
                              client->invalidateCredentials();
-                             callback(error.toUtf8().data(), Error::CredentialsError);
+                             callback(error.toStdString(), Error::CredentialsError);
                              break;
                          default:
-                             callback(error.toUtf8().data(), Error::DownloadInstallError);
+                             callback(error.toStdString(), Error::DownloadInstallError);
+                             break;
                          }
                      });
 

=== modified file 'libclickscope/click/download-manager.h'
--- libclickscope/click/download-manager.h	2016-02-26 18:51:22 +0000
+++ libclickscope/click/download-manager.h	2016-04-14 18:09:31 +0000
@@ -67,12 +67,12 @@
     virtual ~DownloadManager();
 
     virtual void get_progress(const std::string& package_name,
-                              const std::function<void (std::string)>& callback);
+                              const std::function<void (const std::string&)>& callback);
     virtual click::web::Cancellable start(const std::string& url,
                                           const std::string& download_sha512,
                                           const std::string& package_name,
-                                          const std::function<void (std::string,
-                                                                    Error)>& callback);
+                                          const std::function<void (const std::string&,
+                                                                    const Error&)>& callback);
 
     virtual void setCredentialsService(const QSharedPointer<click::CredentialsService>& credentialsService);
 

=== modified file 'libclickscope/click/preview.cpp'
--- libclickscope/click/preview.cpp	2016-04-14 18:09:31 +0000
+++ libclickscope/click/preview.cpp	2016-04-14 18:09:31 +0000
@@ -346,6 +346,7 @@
     reviews_operation.cancel();
     submit_operation.cancel();
     purchase_operation.cancel();
+    download_operation.cancel();
 }
 
 scopes::PreviewWidget PreviewStrategy::build_other_metadata(const PackageDetails &details)
@@ -734,8 +735,8 @@
     std::promise<bool> promise;
     auto future = promise.get_future();
     run_under_qt([this, reply, &promise]() {
-            dm->start(download_url, download_sha512, result["name"].get_string(),
-                      [this, reply, &promise] (std::string msg, DownloadManager::Error dmerr){
+            download_operation = dm->start(download_url, download_sha512, result["name"].get_string(),
+                      [this, reply, &promise] (const std::string& msg, const DownloadManager::Error& dmerr){
                           switch (dmerr)
                           {
                           case DownloadManager::Error::DownloadInstallError:
@@ -1233,7 +1234,7 @@
                       click::Reviews::Error reviewserror) {
             std::string app_name = result["name"].get_string();
             dm->get_progress(app_name,
-                             [this, reply, reviewlist, reviewserror](std::string object_path){
+                             [this, reply, reviewlist, reviewserror](const std::string& object_path){
                 found_object_path = object_path;
                 scopes::PreviewWidgetList button_widgets;
                 if(found_object_path.empty()) {

=== modified file 'libclickscope/click/preview.h'
--- libclickscope/click/preview.h	2016-03-07 14:10:27 +0000
+++ libclickscope/click/preview.h	2016-04-14 18:09:31 +0000
@@ -209,6 +209,7 @@
     scopes::OnlineAccountClient oa_client;
     QSharedPointer<pay::Package> pay_package;
     click::web::Cancellable purchase_operation;
+    click::web::Cancellable download_operation;
 };
 
 class DownloadErrorPreview : public PreviewStrategy

=== modified file 'libclickscope/click/qtbridge.h'
--- libclickscope/click/qtbridge.h	2014-05-26 14:02:45 +0000
+++ libclickscope/click/qtbridge.h	2016-04-14 18:09:31 +0000
@@ -20,6 +20,7 @@
 #ifndef QT_CORE_WORLD_BRIDGE_H_
 #define QT_CORE_WORLD_BRIDGE_H_
 
+#include<QCoreApplication>
 #include <QObject>
 
 #include <functional>
@@ -80,6 +81,21 @@
 
     return future;
 }
+
+/**
+ * @brief Waits for a promise to be fulfilled, without blocking Qt event loop;
+ * @param future The future to wait on for ready state.
+ */
+template<typename T>
+inline void wait_for_future_ready(const std::future<T>& future)
+{
+    std::future_status status = future.wait_for(std::chrono::milliseconds(0));
+    while (status != std::future_status::ready) {
+        QCoreApplication::processEvents();
+        status = future.wait_for(std::chrono::milliseconds(100));
+    }
+}
+
 }
 }
 }

=== modified file 'libclickscope/click/ubuntuone_credentials.cpp'
--- libclickscope/click/ubuntuone_credentials.cpp	2016-04-14 18:09:31 +0000
+++ libclickscope/click/ubuntuone_credentials.cpp	2016-04-14 18:09:31 +0000
@@ -28,9 +28,10 @@
  */
 
 #include "ubuntuone_credentials.h"
+#include "qtbridge.h"
 
 #include <future>
-#include <QCoreApplication>
+
 
 namespace u1 = UbuntuOne;
 
@@ -72,12 +73,7 @@
 
         getCredentials();
 
-        std::future_status status = future.wait_for(std::chrono::milliseconds(0));
-        while (status != std::future_status::ready) {
-            QCoreApplication::processEvents();
-            qDebug() << "Processed some events, waiting to process again.";
-            status = future.wait_for(std::chrono::milliseconds(100));
-        }
+        qt::core::world::wait_for_future_ready<UbuntuOne::Token>(future);
 
         _token = future.get();
         QObject::disconnect(success);