Merge lp:~marcustomlinson/unity-scopes-shell/oa_loading_bar into lp:unity-scopes-shell

[Marcus Tomlinson]

Animate the loading bar on the scope while OA retrieves a token.

[Marcus Tomlinson (marcustomlinson)]

In the case where either:
   1. A new account is added from settings, the scope service is enabled under that account, then the log-in button is pressed in a scope.
   2. The token for a scope expires (bringing the log-in button back), then the log-in button is pressed in the scope.

The call to OA to refresh the token takes some time to return with no UI indication.

This branch uses setSearchInProgress() in order to animate the scope's loading bar while we wait for the token to be returned.

NOTE: The reason I need to use ExcludeUserInputEvents for the event loop here is because we can't allow the user move to another scope / open a preview while this method is busy. This is because the post log-in action needs to be performed from the state the scope was at the time the log-in button was pressed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:178
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/269/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/11
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/11
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/11/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/11

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/269/rebuild

review: Approve (continuous-integration)

[Paweł Stołowski (stolowski)]

+ QFuture<void> future = QtConcurrent::run([&]{ service_statuses = oa_client.get_service_statuses(); });

I think oa_client could be created inside the lambda?

Also, please make the lambda return std::vector<scopes::OnlineAccountClient::ServiceStatus>, then the future will be of QFuture<std::vector<...>> type and you can get the result by future's T result() getter instead of modifying the vector by reference (the lambda needs no outer variables then I think).

review: Needs Fixing

[lp:~marcustomlinson/unity-scopes-shell/oa_loading_bar]

179. By Marcus Tomlinson on 2014-12-12

Addressed review comment

[Marcus Tomlinson (marcustomlinson)]

> + QFuture<void> future = QtConcurrent::run([&]{ service_statuses =
> oa_client.get_service_statuses(); });
>
> I think oa_client could be created inside the lambda?
>
> Also, please make the lambda return
> std::vector<scopes::OnlineAccountClient::ServiceStatus>, then the future will
> be of QFuture<std::vector<...>> type and you can get the result by future's T
> result() getter instead of modifying the vector by reference (the lambda needs
> no outer variables then I think).

Done

[Paweł Stołowski (stolowski)]

36 if (status.service_enabled)
37 {
38 - service_enabled = true;
39 - break;
40 + return true;
41 }

Just "return status.service_enabled" ?

review: Needs Fixing

[Marcus Tomlinson (marcustomlinson)]

> 36 if (status.service_enabled)
> 37 {
> 38 - service_enabled = true;
> 39 - break;
> 40 + return true;
> 41 }
>
> Just "return status.service_enabled" ?

That if statement is within a loop over all accounts. So default is false, the if one is enabled return true

[Paweł Stołowski (stolowski)]

> > 36 if (status.service_enabled)
> > 37 {
> > 38 - service_enabled = true;
> > 39 - break;
> > 40 + return true;
> > 41 }
> >
> > Just "return status.service_enabled" ?
>
> That if statement is within a loop over all accounts. So default is false, the
> if one is enabled return true

Ah, ok, that wasn't obvious from the diff. Looks good now, thanks!

[Paweł Stołowski (stolowski)]

review: Approve

=== modified file 'src/Unity/CMakeLists.txt'
--- src/Unity/CMakeLists.txt	2014-12-02 12:21:57 +0000
+++ src/Unity/CMakeLists.txt	2014-12-12 11:29:06 +0000
@@ -61,7 +61,7 @@
     ${ONLINE_ACCOUNTS_CLIENT_LDFLAGS}
     )
 
-qt5_use_modules(Unity-qml Qml Gui DBus)
+qt5_use_modules(Unity-qml Qml Gui DBus Concurrent)
 
 # export the qmldir qmltypes and plugin files
 export_qmlfiles(Unity Unity)

=== modified file 'src/Unity/scope.cpp'
--- src/Unity/scope.cpp	2014-12-10 11:19:27 +0000
+++ src/Unity/scope.cpp	2014-12-12 11:29:06 +0000
@@ -43,6 +43,7 @@
 #include <QFileInfo>
 #include <QDir>
 #include <QLocale>
+#include <QtConcurrent>
 
 #include <libintl.h>
 
@@ -1247,7 +1248,7 @@
     // calling it from the shell (hence it will use the default UI policy when talking to libsignon).
     setenv("UNITY_SCOPES_OA_UI_POLICY", "1", 0);
 
-    bool service_enabled = false;
+    QFuture<bool> service_enabled_future = QtConcurrent::run([&]
     {
         // Check if at least one account has the specified service enabled
         scopes::OnlineAccountClient oa_client(service_name.toStdString(), service_type.toStdString(), provider_name.toStdString());
@@ -1256,11 +1257,25 @@
         {
             if (status.service_enabled)
             {
-                service_enabled = true;
-                break;
+                return true;
             }
         }
-    }
+        return false;
+    });
+    QFutureWatcher<bool> future_watcher;
+    future_watcher.setFuture(service_enabled_future);
+
+    // Set SearchInProgress so that the loading bar animates while we waiting for the token to be issued.
+    setSearchInProgress(true);
+
+    QEventLoop loop;
+    connect(&future_watcher, &QFutureWatcher<void>::finished, &loop, &QEventLoop::quit);
+    loop.exec(QEventLoop::ProcessEventsFlag::ExcludeUserInputEvents);
+
+    // Unset SearchInProgress to stop the loading bar animation.
+    setSearchInProgress(false);
+
+    bool service_enabled = service_enabled_future.result();
 
     // Start the signon UI if no enabled services were found
     if (!service_enabled)