Merge lp:~larryprice/libertine-scope/libertine-store-status-updates into lp:~larryprice/libertine-scope/libertine-store-search

[Larry Price]

Update state of application once install/remove is complete.

Update state of application once install/remove is complete.

[lp:~larryprice/libertine-scope/libertine-store-status-updates]

119. By Larry Price on 2016-08-26

Updating dbus name

120. By Larry Price on 2016-08-31

Constantly emit progress signal so it's picked up by scope

121. By Larry Price on 2016-08-31

merge

122. By Larry Price on 2016-08-31

fixing test

123. By Larry Price on 2016-09-02

merge

124. By Larry Price on 2016-09-15

Merge with parent

125. By Larry Price on 2016-09-16

merge

126. By Larry Price on 2016-09-16

Remove stupid Timer from Progress object and instead rely on the C++ Preview to ask us to emit the current progress as necessary

127. By Larry Price on 2016-09-19

Use package name to get progress update

128. By Larry Price on 2016-09-19

Test that update_progress was called on service

[Christopher Townsend (townsend)]

Per the conversation on IRC, I'm not a fan of how the progress meter increments, the main problem being that it jumps to 75% quite quickly and then can sit there for some time while packages are downloaded and installed.

This will need to be addressed, but as agreed, this can be done later.

Approving this so we can get it merged.

review: Approve

[lp:~larryprice/libertine-scope/libertine-store-status-updates]

129. By Larry Price on 2016-09-20

Merge with parent

[Christopher Townsend (townsend)]

Just a couple of quick formatting comments inline.

[lp:~larryprice/libertine-scope/libertine-store-status-updates]

130. By Larry Price on 2016-09-21

Fixing indentation of tuple def

=== modified file 'scope/store/package.cpp'
--- scope/store/package.cpp	2016-08-25 20:41:33 +0000
+++ scope/store/package.cpp	2016-09-21 14:34:34 +0000
@@ -30,6 +30,7 @@
   pkg.id = pkgMap["id"].toString().toStdString();
   pkg.package = pkgMap["package"].toString().toStdString();
   pkg.status = pkgMap["status"].toString().toStdString();
+  pkg.progress = pkgMap["progress_id"].toString().toStdString();
 
   for (auto const& screenshot: pkgMap["screenshots"].toStringList())
   {

=== modified file 'scope/store/package.h'
--- scope/store/package.h	2016-08-25 20:41:33 +0000
+++ scope/store/package.h	2016-09-21 14:34:34 +0000
@@ -48,6 +48,7 @@
   std::string publisher;
   std::string website;
   std::string license;
+  std::string progress;
 
   std::vector<std::string> screenshots;
 };

=== modified file 'scope/store/preview.cpp'
--- scope/store/preview.cpp	2016-08-25 20:41:33 +0000
+++ scope/store/preview.cpp	2016-09-21 14:34:34 +0000
@@ -18,6 +18,8 @@
 
 #include "scope/store/i18n.h"
 #include "scope/store/service_manager.h"
+#include <future>
+#include <QDebug>
 #include <unity/UnityExceptions.h>
 #include <unity/scopes/ColumnLayout.h>
 #include <unity/scopes/PreviewReply.h>
@@ -25,7 +27,6 @@
 #include <unity/scopes/VariantBuilder.h>
 #include <unity/scopes/ActionMetadata.h>
 #include <unity/scopes/Result.h>
-#include <QDebug>
 
 
 namespace usc = unity::scopes;
@@ -92,18 +93,19 @@
 static usc::PreviewWidget
 buttonWidgets(Libertine::Store::Package const& package)
 {
-  usc::PreviewWidget buttons(ACTIONS_ID, "actions");
+  if (package.status == "installing" || package.status == "removing")
+  {
+    usc::PreviewWidget progress(ACTIONS_ID, "progress");
+    usc::VariantMap tuple;
+    tuple["dbus-name"] = "com.canonical.libertine.ContainerManager";
+    tuple["dbus-object"] = std::string("/Progress/") + package.progress;
+    progress.add_attribute_value("source", usc::Variant(tuple));
+
+    return progress;
+  }
+
   usc::VariantBuilder builder;
-
-  if (package.status.empty())
-  {
-    builder.add_tuple(
-        {
-            {"id", usc::Variant("install")},
-            {"label", usc::Variant(ACTION_INSTALL)},
-        });
-  }
-  else if (package.status == "installed")
+  if (package.status == "installed")
   {
     builder.add_tuple(
         {
@@ -120,10 +122,12 @@
   {
     builder.add_tuple(
         {
-            {"id", usc::Variant("noop")},
-            {"label", usc::Variant(package.status == "removing" ? ACTION_NOOP_REMOVING : ACTION_NOOP_INSTALLING)},
+            {"id", usc::Variant("install")},
+            {"label", usc::Variant(ACTION_INSTALL)},
         });
   }
+
+  usc::PreviewWidget buttons(ACTIONS_ID, "actions");
   buttons.add_attribute_value("actions", builder.end());
   return buttons;
 }
@@ -170,13 +174,28 @@
 }
 
 
+std::chrono::milliseconds Libertine::Store::Preview::
+update_progress_after = std::chrono::milliseconds(500);
+
+
 Libertine::Store::Preview::
-Preview(usc::Result const&         result,
-        usc::ActionMetadata const& metadata,
+Preview(usc::Result const&                      result,
+        usc::ActionMetadata const&              metadata,
         std::shared_ptr<ServiceManager>  const& service)
   : PreviewQueryBase(result, metadata)
   , service_(service)
-{
+  , thread_()
+{
+}
+
+
+Libertine::Store::Preview::
+~Preview()
+{
+    if (thread_ != nullptr)
+    {
+      thread_->join();
+    }
 }
 
 
@@ -225,6 +244,15 @@
 
   registerLayouts(reply);
   reply->push(widgets);
+
+  // update the progress bar asynchronously
+  // if an operation is in progress
+  if (app.status == "installing" || app.status == "removing") {
+    thread_.reset(new std::thread([=](){
+      std::this_thread::sleep_for(update_progress_after);
+      service_->update_progress(QString::fromStdString(app.package));
+    }));
+  }
 }
 
 

=== modified file 'scope/store/preview.h'
--- scope/store/preview.h	2016-07-25 20:12:20 +0000
+++ scope/store/preview.h	2016-09-21 14:34:34 +0000
@@ -16,9 +16,16 @@
 #ifndef LIBERTINE_STORE_PREVIEW_H
 #define LIBERTINE_STORE_PREVIEW_H
 
+#include <chrono>
 #include <unity/scopes/PreviewQueryBase.h>
 
 
+namespace std
+{
+class thread;
+}
+
+
 namespace Libertine
 {
 namespace Store
@@ -37,7 +44,7 @@
                    unity::scopes::ActionMetadata const&   metadata,
                    std::shared_ptr<ServiceManager> const& service);
 
-  virtual ~Preview() = default;
+  virtual ~Preview();
 
   virtual void
   cancelled() override;
@@ -45,6 +52,9 @@
   virtual void
   run(unity::scopes::PreviewReplyProxy const& reply) override;
 
+  static
+  std::chrono::milliseconds update_progress_after;
+
 private:
   void
   registerLayouts(unity::scopes::PreviewReplyProxy const& reply);
@@ -65,6 +75,7 @@
   } twoColumns;
 
   std::shared_ptr<ServiceManager> service_;
+  std::unique_ptr<std::thread> thread_;
 };
 
 

=== modified file 'scope/store/scope.cpp'
--- scope/store/scope.cpp	2016-08-25 20:41:33 +0000
+++ scope/store/scope.cpp	2016-09-21 14:34:34 +0000
@@ -60,7 +60,6 @@
 }
 
 
-
 usc::ActivationQueryBase::UPtr Libertine::Store::Scope::
 perform_action(usc::Result const&         result,
                usc::ActionMetadata const& metadata,
@@ -72,4 +71,3 @@
                                                    action_id,
                                                    service_));
 }
-

=== modified file 'scope/store/scope.h'
--- scope/store/scope.h	2016-08-15 20:49:25 +0000
+++ scope/store/scope.h	2016-09-21 14:34:34 +0000
@@ -75,6 +75,3 @@
 } // namespace Libertine
 
 #endif /* LIBERTINE_STORE_SCOPE_H_ */
-
-
-

=== modified file 'scope/store/service_manager.cpp'
--- scope/store/service_manager.cpp	2016-08-31 19:59:05 +0000
+++ scope/store/service_manager.cpp	2016-09-21 14:34:34 +0000
@@ -25,6 +25,7 @@
 constexpr auto APP_INFO_METHOD = "app_info";
 constexpr auto INSTALL_METHOD = "install";
 constexpr auto REMOVE_METHOD = "remove";
+constexpr auto UPDATE_PROGRESS_METHOD = "update_progress";
 constexpr auto LIBERTINE_SERVICE_DESTINATION = "com.canonical.libertine.ContainerManager";
 constexpr auto LIBERTINE_SERVICE_INTERFACE = "com.canonical.libertine.ContainerManager";
 constexpr auto LIBERTINE_SERVICE_OBJECT = "/Manager";
@@ -59,6 +60,13 @@
 }
 
 
+void Libertine::Store::ServiceManager::
+update_progress(QString const& package_name) const
+{
+  call<bool>(UPDATE_PROGRESS_METHOD, QVariant(package_name));
+}
+
+
 QList<Libertine::Store::Package> Libertine::Store::ServiceManager::
 search_cache(const QString &query) const
 {
@@ -93,4 +101,3 @@
 {
   return QDBusConnection::sessionBus().isConnected();
 }
-

=== modified file 'scope/store/service_manager.h'
--- scope/store/service_manager.h	2016-08-16 18:32:43 +0000
+++ scope/store/service_manager.h	2016-09-21 14:34:34 +0000
@@ -38,6 +38,7 @@
   virtual Package app_info(QString const& app_id) const;
   virtual void install(QString const& package_name) const;
   virtual void remove(QString const& package_name) const;
+  virtual void update_progress(QString const& package_name) const;
 
 private:
   bool valid() const;

=== modified file 'service/libertine_service/apt.py'
--- service/libertine_service/apt.py	2016-09-16 18:39:05 +0000
+++ service/libertine_service/apt.py	2016-09-21 14:34:34 +0000
@@ -101,5 +101,6 @@
                 app_data["summary"] = app.versions[0].summary
                 app_data["website"] = app.versions[0].homepage
             app_data["description"] = app.versions[0].description
+            app_data["package"] = app.name
 
         return app_data

=== modified file 'service/libertine_service/container.py'
--- service/libertine_service/container.py	2016-08-18 19:04:10 +0000
+++ service/libertine_service/container.py	2016-09-21 14:34:34 +0000
@@ -36,24 +36,24 @@
         self.libertine_config = SafeContainersConfig()
         self.lock = Lock()
 
-    def install(self, package_name, container_id):
+    def install(self, package_name, container_id, progress):
         self.libertine_config.refresh_database()
         if not self._container_exists(container_id):
             self.log.debug("Container does not exist, queuing create operation")
             create = tasks.CreateTask(container_id, self.libertine_config, self.lock, self.log)
             create.start()
 
-        task = tasks.InstallTask(package_name, container_id, self.libertine_config, self.lock, self.log)
+        task = tasks.InstallTask(package_name, container_id, self.libertine_config, progress, self.lock, self.log)
         return task.start()
 
-    def remove(self, package_name, container_id):
+    def remove(self, package_name, container_id, progress):
         self.libertine_config.refresh_database()
         if not self._container_exists(container_id):
             self.log.debug("Container does not exist, queuing create operation")
             create = tasks.CreateTask(container_id, self.libertine_config, self.lock, self.log)
             create.start()
 
-        task = tasks.RemoveTask(package_name, container_id, self.libertine_config, self.lock, self.log)
+        task = tasks.RemoveTask(package_name, container_id, self.libertine_config, progress, self.lock, self.log)
         return task.start()
 
     def status(self, package_name, container_id):

=== modified file 'service/libertine_service/dbus.py'
--- service/libertine_service/dbus.py	2016-08-31 19:59:05 +0000
+++ service/libertine_service/dbus.py	2016-09-21 14:34:34 +0000
@@ -19,20 +19,53 @@
 from libertine_service import appstream
 from libertine_service import container
 from dbus.mainloop.glib import DBusGMainLoop
+from time import time
 
 
 LIBERTINE_SERVICE_NAME = "com.canonical.libertine.ContainerManager"
 LIBERTINE_STORE_INTERFACE = LIBERTINE_SERVICE_NAME
+DOWNLOAD_INTERFACE = "com.canonical.applications.Download"
 LIBERTINE_STORE_PATH = "/Manager"
 CONTAINER_ID = "palatine"
 
 log = logging.getLogger("Libertine")
 
 
+class Progress(dbus.service.Object):
+    def __init__(self, connection, package):
+        log.debug("creating a Progress object")
+        self.package = package
+        self._id = hex(int(time()*10000000))[2:]
+        self._finished = False
+        self.current_progress = (dbus.UInt64(0), dbus.UInt64(0))
+        dbus.service.Object.__init__(self, conn=connection, object_path=("/Progress/%s" % self._id))
+
+    def emit_progress(self):
+        self.progress(self.current_progress[0], self.current_progress[1])
+
+    @property
+    def id(self):
+        return self._id
+
+    @property
+    def done(self):
+        return self._finished
+
+    @dbus.service.signal(DOWNLOAD_INTERFACE)
+    def finished(self, path):
+        log.debug("emit finished('%s')" % path)
+        self._finished = True
+
+    @dbus.service.signal(DOWNLOAD_INTERFACE)
+    def progress(self, received, total):
+        log.debug("emit progress(%d, %d)" % (received, total))
+        self.current_progress = (received, total)
+
+
 class Service(dbus.service.Object):
 
     def __init__(self, config):
-        log.info("creating service")
+        log.debug("creating service")
         DBusGMainLoop(set_as_default=True)
         try:
             bus_name = dbus.service.BusName(LIBERTINE_SERVICE_NAME,
@@ -49,6 +82,7 @@
             self.cache = apt.AptCache(config)
 
         self.container = container.Container(log)
+        self.progress = {}
 
         super().__init__(bus_name, LIBERTINE_STORE_PATH)
 
@@ -85,6 +119,9 @@
         app = self.cache.app_info(app_id)
         if 'package' in app:
             app['status'] = self.container.status(app['package'], CONTAINER_ID)
+            if app['package'] in self.progress:
+                app['progress_id'] = self.progress[app['package']].id
+
         return app
 
     @dbus.service.signal(LIBERTINE_STORE_INTERFACE,
@@ -92,16 +129,49 @@
     def updating(self, target, percent):
         log.debug("emit updating('{}', {})".format(target, percent))
 
+    # Packaging
+
     @dbus.service.method(LIBERTINE_STORE_INTERFACE,
                          in_signature='s',
                          out_signature='x')
     def install(self, package_name):
         log.debug("install('%s')" % package_name)
-        return self.container.install(package_name, CONTAINER_ID)
+        return self.container.install(package_name, CONTAINER_ID, self.find_or_create_package_progress(package_name))
 
     @dbus.service.method(LIBERTINE_STORE_INTERFACE,
                          in_signature='s',
                          out_signature='x')
     def remove(self, package_name):
         log.debug("remove('%s')" % package_name)
-        return self.container.remove(package_name, CONTAINER_ID)
+        return self.container.remove(package_name, CONTAINER_ID, self.find_or_create_package_progress(package_name))
+
+    @dbus.service.method(LIBERTINE_STORE_INTERFACE,
+                         in_signature='s',
+                         out_signature='b')
+    def update_progress(self, package_name):
+        log.debug("update_progress('%s')" % package_name)
+        self.cleanup_progress()
+        if package_name in self.progress:
+            self.progress[package_name].emit_progress()
+            return True
+        return False
+
+    def find_or_create_package_progress(self, package_name):
+        self.cleanup_progress()
+
+        if package_name in self.progress:
+            progress = self.progress[package_name]
+        else:
+            progress = Progress(self.connection, package_name)
+            self.progress[package_name] = progress
+
+        return progress
+
+    def cleanup_progress(self):
+        updated_progress_list = {}
+        for key in self.progress.keys():
+            if self.progress[key].done:
+                self.progress[key].remove_from_connection()
+            else:
+                updated_progress_list[key] = self.progress[key]
+        self.progress = updated_progress_list

=== modified file 'service/libertine_service/tasks.py'
--- service/libertine_service/tasks.py	2016-09-16 16:38:43 +0000
+++ service/libertine_service/tasks.py	2016-09-21 14:34:34 +0000
@@ -16,6 +16,7 @@
 from threading import Thread
 from abc import ABCMeta, abstractmethod
 from libertine import LibertineContainer, HostInfo, utils
+from dbus import UInt64
 
 
 class LibertineTask(metaclass=ABCMeta):
@@ -34,6 +35,8 @@
             thread = Thread(target=self.run)
             thread.start()
             return thread.ident
+        else:
+            return 0
 
     def run(self):
         with self.lock:
@@ -47,53 +50,75 @@
         return True
 
 
-class InstallTask(LibertineTask):
-    def __init__(self, package_name, container_id, config, lock, log):
+class ProgressTask(LibertineTask):
+    def __init__(self, package_name, container_id, config, progress, lock, log):
         super().__init__(lock, log)
         self.package = package_name
         self.container = container_id
         self.config = config
+        self.progress = progress
+        self.current_progress = 0
+
+    def _progress(self):
+        self.current_progress += 1
+        self.progress.progress(UInt64(self.current_progress), UInt64(self.current_progress+1))
+
+
+class InstallTask(ProgressTask):
+    def __init__(self, package_name, container_id, config, progress, lock, log):
+        super().__init__(package_name, container_id, config, progress, lock, log)
 
     def _run(self):
         self.log.debug("Installing package '%s'" % self.package)
+        self._progress()
         container = LibertineContainer(self.container, self.config)
         if container.install_package(self.package):
+            self._progress()
             self.config.update_package_install_status(self.container, self.package, "installed")
             utils.refresh_libertine_scope()
         else:
+            self.log.warning("Package installation failed for '%s'" % self.package)
             self.config.delete_package(self.container, self.package)
+        self.progress.finished(self.package)
 
     def _before(self):
         self.log.debug("InstallTask::_before")
+        self._progress()
         self.config.add_new_package(self.container, self.package)
         self.config.update_package_install_status(self.container, self.package, "installing")
+        self._progress()
         return True
 
 
-class RemoveTask(LibertineTask):
-    def __init__(self, package_name, container_id, config, lock, log):
-        super().__init__(lock, log)
-        self.package = package_name
-        self.container = container_id
-        self.config = config
+class RemoveTask(ProgressTask):
+    def __init__(self, package_name, container_id, config, progress, lock, log):
+        super().__init__(package_name, container_id, config, progress, lock, log)
         self.fallback = "installed"
 
     def _run(self):
         self.log.debug("Removing package '%s'" % self.package)
+        self._progress()
         container = LibertineContainer(self.container, self.config)
         if container.remove_package(self.package):
+            self._progress()
             self.config.delete_package(self.container, self.package)
             utils.refresh_libertine_scope()
         else:
+            self.log.warning("Package removal failed for '%s'" % self.package)
             self.config.update_package_install_status(self.container, self.package, self.fallback)
+        self.progress.finished(self.package)
 
     def _before(self):
         self.log.debug("RemoveTask::_before")
+        self._progress()
         if self.config.package_exists(self.container, self.package):
             self.fallback = self.config.get_package_install_status(self.container, self.package)
             self.config.update_package_install_status(self.container, self.package, "removing")
+            self._progress()
             return True
-        return False
+        else:
+            self.log.debug("Package '%s' already exists, skipping install" % self.package)
+            return False
 
 
 class CreateTask(LibertineTask):

=== modified file 'tests/scope/store/mock_service_manager.h'
--- tests/scope/store/mock_service_manager.h	2016-08-18 18:17:36 +0000
+++ tests/scope/store/mock_service_manager.h	2016-09-21 14:34:34 +0000
@@ -37,6 +37,7 @@
 
   MOCK_CONST_METHOD1(search_cache, QList<Package>(QString const&));
   MOCK_CONST_METHOD1(app_info, Package(QString const&));
+  MOCK_CONST_METHOD1(update_progress, void(QString const&));
   MOCK_CONST_METHOD1(install, void(QString const&));
   MOCK_CONST_METHOD1(remove, void(QString const&));
 };

=== modified file 'tests/scope/store/test_preview.cpp'
--- tests/scope/store/test_preview.cpp	2016-08-25 20:41:33 +0000
+++ tests/scope/store/test_preview.cpp	2016-09-21 14:34:34 +0000
@@ -52,14 +52,13 @@
 }
 
 static void
-verifyInProgressButtons(unity::scopes::PreviewWidget const& buttons, std::string const& text)
+verifyInProgressButtons(unity::scopes::PreviewWidget const& buttons, std::string const& progress_id)
 {
   EXPECT_EQ("buttons", buttons.id());
-  EXPECT_EQ("actions", buttons.widget_type());
-  auto buttons_actions = buttons.attribute_values()["actions"].get_array();
-  ASSERT_EQ(1, buttons_actions.size());
-  EXPECT_EQ("noop", buttons_actions[0].get_dict()["id"].get_string());
-  EXPECT_EQ(text, buttons_actions[0].get_dict()["label"].get_string());
+  EXPECT_EQ("progress", buttons.widget_type());
+  auto source = buttons.attribute_values()["source"].get_dict();
+  EXPECT_EQ("com.canonical.libertine.ContainerManager", source["dbus-name"].get_string());
+  EXPECT_EQ(std::string("/Progress/") + progress_id, source["dbus-object"].get_string());
 }
 
 
@@ -160,6 +159,7 @@
   SetUp()
   {
     EXPECT_CALL(reply, push(testing::_)).WillOnce(testing::SaveArg<0>(list.get()));
+    Preview::update_progress_after = std::chrono::milliseconds(0);
   }
 
   void usePackage(Package const& package)
@@ -248,17 +248,19 @@
 {
   auto package = basePackage();
   package.status = "removing";
+  package.progress = "something";
   usePackage(package);
+  EXPECT_CALL(*service, update_progress(QString::fromStdString(package.package)));
 
-  Preview preview(result, metadata, service);
-  preview.run(proxy);
+  std::unique_ptr<Preview> preview(new Preview(result, metadata, service));
+  preview->run(proxy);
 
   ASSERT_NE(nullptr, list);
   ASSERT_EQ(4, list->size());
 
   verifyHeader(package, list->front());
   list->pop_front();
-  verifyInProgressButtons(list->front(), "Removing...");
+  verifyInProgressButtons(list->front(), package.progress);
   list->pop_front();
   verifyMetadata(package, list->front());
   list->pop_front();
@@ -270,17 +272,19 @@
 {
   auto package = basePackage();
   package.status = "installing";
+  package.progress = "foobar";
   usePackage(package);
+  EXPECT_CALL(*service, update_progress(QString::fromStdString(package.package)));
 
-  Preview preview(result, metadata, service);
-  preview.run(proxy);
+  std::unique_ptr<Preview> preview(new Preview(result, metadata, service));
+  preview->run(proxy);
 
   ASSERT_NE(nullptr, list);
   ASSERT_EQ(4, list->size());
 
   verifyHeader(package, list->front());
   list->pop_front();
-  verifyInProgressButtons(list->front(), "Installing...");
+  verifyInProgressButtons(list->front(), package.progress);
   list->pop_front();
   verifyMetadata(package, list->front());
   list->pop_front();