Merge lp:~charlesk/keeper/upate-status-properties-pt-1 into lp:keeper

[Charles Kerr]

Update what properties are provided by the com.canonical.keeper.User methods.

Update what properties are provided by the com.canonical.keeper.User methods.

The low-hanging fruit from https://trello.com/c/dCHzIxCG/58-com-canonical-keeper-user-should-support-design-s-specs.

We don't have anything implementing the state property maps in the service itself, so the card can't be completed yet. But now I can at least test the dbusmock template for an error state when the helper errors out :)

[Xavi Garcia (xavi-garcia-mena)]

I'm not sure if after landing this I will have conflicts with my branch:
https://code.launchpad.net/~xavi-garcia-mena/keeper/using-cwd-and-user-startbackup/+merge/301496

It looks good and your branch was first, so... let's merge it. :)

review: Approve

=== modified file 'src/qdbus-stubs/com.canonical.keeper.User.xml'
--- src/qdbus-stubs/com.canonical.keeper.User.xml	2016-07-07 21:42:19 +0000
+++ src/qdbus-stubs/com.canonical.keeper.User.xml	2016-07-28 19:50:41 +0000
@@ -14,8 +14,7 @@
                   as a map from opaque backup keys to key/value pairs
                   of the choice’s properties,
                   including a ‘display-name’ string,
-                  ‘type’ string (‘application’, ‘system-data’, or ‘folder’)
-                  and, when possible, an ‘icon’ symbolic icon string.</doc:para>
+                  ‘type’ string (‘application’, ‘system-data’, or ‘folder’).</doc:para>
         <doc:para>The choices should be presented to the user,
                   and the keys of those selected should be passed
                   to StartBackup().</doc:para>
@@ -71,19 +70,18 @@
         <doc:description>
           <doc:para>Provides state information so the user interface can show
                     the progress of backup or restore tasks.</doc:para>
-          <doc:para>State is a map of opaque backup keys to property maps,
-                    which will contain a 'display-name' string and 'action' int32
-                    whose possible values are queued(0), saving(1),
-                    restoring(2), complete(3), stopped(4)</doc:para>
-          <doc:para>Some property maps may also have an 'item' string
-                    and a 'percent-done' double [0..1.0].
-                    For example if these are set to (1, “Photos”, 0.36),
-                    the user interface could show “Backing up Photos (36%)”.
-                    Clients should gracefully handle missing properties;
-                    eg a missing percent-done could instead show
-                    “Backing up Photos”.</doc:para>
-          <doc:para>A failed task's property map may also contain an 'error'
-                    string if set by the backup helpers.</doc:para>
+          <doc:para>State is a map of opaque backup keys to property maps.
+                    The property maps include:
+                    * 'action' (string): tells what the task is doing right now.
+                       Possible values are 'queued', 'saving', 'restoring',
+                       'cancelled', 'failed', and 'complete'
+                    * 'display-name' (string): human-readable task name, e.g. "Pictures"
+                    * 'percent-done' (double): how much of this task is complete
+                    * 'speed' (int32): bytes per second
+          </doc:para>
+          <doc:para>If a task's 'action' state is 'failed' the property map also includes:
+                    * 'error' (string): a human-readable error message
+        </doc:para>
         </doc:description>
       </doc:doc>
     </property>
 
=== modified file 'src/service/backup-choices.cpp'
--- src/service/backup-choices.cpp	2016-07-07 14:04:38 +0000
+++ src/service/backup-choices.cpp	2016-07-28 19:50:41 +0000
@@ -59,12 +59,10 @@
     //
 
     const auto type_key = QStringLiteral("type");
-    const auto icon_key = QStringLiteral("icon");
     const auto system_data_str = QStringLiteral("system-data");
     {
         Metadata m(generate_new_uuid(), "System Data"); // FIXME: how to i18n in a Qt DBus service?
         m.set_property(type_key, system_data_str);
-        m.set_property(icon_key, QStringLiteral("folder-system"));
         ret.push_back(m);
     }
 
@@ -129,10 +127,6 @@
                 if (version != QJsonValue::Undefined)
                     m.set_property(version_key, version.toString());
 
-                const auto icon = o[icon_key];
-                if (icon != QJsonValue::Undefined)
-                    m.set_property(icon_key, icon.toString());
-
                 ret.push_back(m);
             }
         }
@@ -142,22 +136,19 @@
     //  XDG User Directories
     //
 
-    const struct {
-        QStandardPaths::StandardLocation location;
-        QString icon;
-    } standard_locations[] = {
-        { QStandardPaths::DocumentsLocation, QStringLiteral("folder-documents") },
-        { QStandardPaths::MoviesLocation,    QStringLiteral("folder-movies")    },
-        { QStandardPaths::PicturesLocation,  QStringLiteral("folder-pictures")  },
-        { QStandardPaths::MusicLocation,     QStringLiteral("folder-music")     }
+    const std::array<QStandardPaths::StandardLocation,4> standard_locations = {
+        QStandardPaths::DocumentsLocation,
+        QStandardPaths::MoviesLocation,
+        QStandardPaths::PicturesLocation,
+        QStandardPaths::MusicLocation
     };
 
     const auto path_key = QStringLiteral("path");
     const auto user_folder_str = QStringLiteral("folder");
-    for (const auto& sl : standard_locations)
+    for (const auto& location : standard_locations)
     {
-        const auto name = QStandardPaths::displayName(sl.location);
-        const auto locations = QStandardPaths::standardLocations(sl.location);
+        const auto name = QStandardPaths::displayName(location);
+        const auto locations = QStandardPaths::standardLocations(location);
         if (locations.empty())
         {
             qWarning() << "unable to find path for"  << name;
@@ -168,7 +159,6 @@
             Metadata m(keystr, name);
             m.set_property(path_key, locations.front());
             m.set_property(type_key, user_folder_str);
-            m.set_property(icon_key, sl.icon);
             ret.push_back(m);
         }
     }
 
=== modified file 'tests/com_canonical_keeper.py'
--- tests/com_canonical_keeper.py	2016-07-21 04:21:47 +0000
+++ tests/com_canonical_keeper.py	2016-07-28 19:50:41 +0000
@@ -5,6 +5,7 @@
 import socket
 import subprocess
 import sys
+import time
 from dbusmock import OBJECT_MANAGER_IFACE, mockobject
 from gi.repository import GLib
 
@@ -36,17 +37,19 @@
 MAIN_OBJ = SERVICE_PATH
 SYSTEM_BUS = False
 
-ACTION_QUEUED = 0
-ACTION_SAVING = 1
-ACTION_RESTORING = 2
-ACTION_COMPLETE = 3
-ACTION_STOPPED = 4
+ACTION_QUEUED = 'queued'
+ACTION_SAVING = 'saving'
+ACTION_RESTORING = 'restoring'
+ACTION_CANCELLED = 'cancelled'
+ACTION_FAILED = 'failed'
+ACTION_COMPLETE = 'complete'
 
+KEY_ACTION = 'action'
 KEY_CTIME = 'ctime'
 KEY_BLOB = 'blob-data'
 KEY_HELPER = 'helper-exec'
-KEY_ICON = 'icon'
 KEY_NAME = 'display-name'
+KEY_SPEED = 'speed'
 KEY_SIZE = 'size'
 KEY_SUBTYPE = 'subtype'
 KEY_TYPE = 'type'
@@ -179,21 +182,9 @@
 def user_build_state(user):
     """Returns a generated state dictionary.
 
-    State is a map of opaque backup keys to property maps,
-    which will contain a 'display-name' string and 'action' int
-    whose possible values are queued(0), saving(1),
-    restoring(2), complete(3), and stopped(4).
-
-    Some property maps may also have an 'item' string
-    and a 'percent-done' double [0..1.0].
-    For example if these are set to (1, "Photos", 0.36),
-    the user interface could show "Backing up Photos (36%)".
-    Clients should gracefully handle missing properties;
-    eg a missing percent-done could instead show
-    "Backing up Photos".
-
-    A failed task's property map may also contain an 'error'
-    string if set by the backup helpers.
+    State is a map of opaque backup keys to property maps.
+    See the documentation for com.canonical.keeper.User's
+    State() method for more information.
     """
 
     tasks_states = {}
@@ -208,9 +199,9 @@
                 action = ACTION_RESTORING
         elif uuid in user.remaining_tasks:
             action = ACTION_QUEUED
-        else:  # FIXME: 'ACTION_STOPPED' not handled yet
+        else:  # fixme: handle ACTION_CANCELLED, ACTION_FAILED
             action = ACTION_COMPLETE
-        task_state['action'] = dbus.Int32(action)
+        task_state[KEY_ACTION] = dbus.String(action)
 
         # get the task's display-name
         choice = user.backup_choices.get(uuid, None)
@@ -221,7 +212,22 @@
         display_name = choice.get(KEY_NAME, None)
         task_state[KEY_NAME] = dbus.String(display_name)
 
+        # speed
+        helper = mockobject.objects[HELPER_PATH]
+        if (uuid == user.current_task) and helper.work:
+            n_secs = 2
+            n_bytes = 0
+            too_old = time.time() - n_secs
+            for key in helper.work.bytes_per_second:
+                if key > too_old:
+                    n_bytes += helper.work.bytes_per_second[key]
+            bytes_per_second = n_bytes / n_secs
+        else:
+            bytes_per_second = 0
+        task_state[KEY_SPEED] = dbus.Int32(bytes_per_second)
+
         # FIXME: use a real percentage here
+        # FIXME: handle ACTION_CANCELLED, ACTION_FAILED
         if action == ACTION_COMPLETE:
             percent_done = dbus.Double(1.0)
         elif action == ACTION_SAVING or action == ACTION_RESTORING:
@@ -256,6 +262,7 @@
     n_left = None
     sock = None
     uuid = None
+    bytes_per_second = None
 
 
 def helper_periodic_func(helper):
@@ -265,10 +272,15 @@
 
     # try to read a bit
     chunk = helper.work.sock.recv(4096*2)
-    if len(chunk):
+    chunk_len = len(chunk)
+    if chunk_len:
         helper.work.chunks.append(chunk)
-        helper.work.n_left -= len(chunk)
-        helper.log('got %s bytes; %s left' % (len(chunk), helper.work.n_left))
+        helper.work.n_left -= chunk_len
+        key = int(time.time())
+        old_n_bytes = helper.work.bytes_per_second.get(key, 0)
+        new_n_bytes = old_n_bytes + chunk_len
+        helper.work.bytes_per_second[key] = new_n_bytes
+        helper.log('got %s bytes; %s left' % (chunk_len, helper.work.n_left))
 
     # cleanup if done
     done = helper.work.n_left <= 0
@@ -284,7 +296,7 @@
         user.start_next_task(user)
         helper.work = None
 
-    if len(chunk) or done:
+    if chunk_len or done:
         user = mockobject.objects[USER_PATH]
         user.update_state_property(user)
 
@@ -306,6 +318,7 @@
     work.n_left = n_bytes
     work.sock = parent
     work.uuid = mockobject.objects[USER_PATH].current_task
+    work.bytes_per_second = {}
     helper.work = work
 
     # start checking periodically
@@ -325,7 +338,7 @@
 
 def mock_add_backup_choice(mock, uuid, props):
 
-    keys = [KEY_NAME, KEY_TYPE, KEY_SUBTYPE, KEY_ICON, KEY_HELPER]
+    keys = [KEY_NAME, KEY_TYPE, KEY_SUBTYPE, KEY_HELPER]
     if set(keys) != set(props.keys()):
         badarg('need props: %s got %s' % (keys, props.keys()))
 
@@ -339,7 +352,7 @@
 
 def mock_add_restore_choice(mock, uuid, props):
 
-    keys = [KEY_NAME, KEY_TYPE, KEY_SUBTYPE, KEY_ICON, KEY_HELPER,
+    keys = [KEY_NAME, KEY_TYPE, KEY_SUBTYPE, KEY_HELPER,
             KEY_SIZE, KEY_CTIME, KEY_BLOB]
     if set(keys) != set(props.keys()):
         badarg('need props: %s got %s' % (keys, props.keys()))
 
=== modified file 'tests/dbusmock/keeper-template-test.cpp'
--- tests/dbusmock/keeper-template-test.cpp	2016-07-27 10:13:02 +0000
+++ tests/dbusmock/keeper-template-test.cpp	2016-07-28 19:50:41 +0000
@@ -51,7 +51,6 @@
         { KEY_NAME, QStringLiteral("some-name") },
         { KEY_TYPE, QStringLiteral("some-type") },
         { KEY_SUBTYPE, QStringLiteral("some-subtype") },
-        { KEY_ICON, QStringLiteral("some-icon") },
         { KEY_HELPER, QString::fromUtf8(FAKE_BACKUP_HELPER_EXEC) }
     };
 
@@ -90,7 +89,6 @@
         { KEY_NAME, QStringLiteral("some-name") },
         { KEY_TYPE, QStringLiteral("some-type") },
         { KEY_SUBTYPE, QStringLiteral("some-subtype") },
-        { KEY_ICON, QStringLiteral("some-icon") },
         { KEY_HELPER, QString::fromUtf8("/dev/null") },
         { KEY_SIZE, quint64(blob.size()) },
         { KEY_CTIME, quint64(time(nullptr)) },
@@ -143,7 +141,6 @@
         { KEY_NAME, QStringLiteral("Music") },
         { KEY_TYPE, QStringLiteral("folder") },
         { KEY_SUBTYPE, sandbox.path() },
-        { KEY_ICON, QStringLiteral("music-icon") },
         { KEY_HELPER, QString::fromUtf8(FAKE_BACKUP_HELPER_EXEC) }
     };
 
 
=== modified file 'tests/unit/metadata-providers/user-dirs-test.cpp'
--- tests/unit/metadata-providers/user-dirs-test.cpp	2016-07-01 15:17:06 +0000
+++ tests/unit/metadata-providers/user-dirs-test.cpp	2016-07-28 19:50:41 +0000
@@ -70,13 +70,11 @@
 
     // confirm that choices has the advertised public properties
     const auto type_str = QStringLiteral("type");
-    const auto icon_str = QStringLiteral("icon");
     for(const auto& choice : choices)
     {
         ASSERT_FALSE(choice.key().isEmpty());
         ASSERT_FALSE(choice.display_name().isEmpty());
         ASSERT_TRUE(choice.has_property(type_str));
-        EXPECT_TRUE(choice.has_property(icon_str));
     }
 
     // confirm that we have a system-data choice
@@ -86,7 +84,7 @@
             break;
     ASSERT_TRUE(i != n);
     auto system_data = choices[i];
-    EXPECT_TRUE(system_data.has_property(icon_str));
+    EXPECT_TRUE(system_data.has_property(type_str));
 
     // confirm that we have user-dir choices
     std::set<QString> expected_user_dir_display_names = {
 
=== modified file 'tests/unit/tar/keeper-tar-create-test.cpp'
--- tests/unit/tar/keeper-tar-create-test.cpp	2016-07-22 13:49:34 +0000
+++ tests/unit/tar/keeper-tar-create-test.cpp	2016-07-28 19:50:41 +0000
@@ -43,7 +43,6 @@
         // tell keeper that's a backup choice
         const auto uuid = add_backup_choice(QMap<QString,QVariant>{
             { KEY_NAME, QDir(in.path()).dirName() },
-            { KEY_ICON, QStringLiteral("folder") },
             { KEY_TYPE, QStringLiteral("folder") },
             { KEY_SUBTYPE, in.path() },
             { KEY_HELPER, QString::fromUtf8(KEEPER_TAR_CREATE_INVOKE) }
 
=== modified file 'tests/utils/keeper-dbusmock-fixture.h'
--- tests/utils/keeper-dbusmock-fixture.h	2016-07-22 13:47:58 +0000
+++ tests/utils/keeper-dbusmock-fixture.h	2016-07-28 19:50:41 +0000
@@ -37,22 +37,21 @@
 #define KEY_CTIME "ctime"
 #define KEY_BLOB "blob-data"
 #define KEY_HELPER "helper-exec"
-#define KEY_ICON "icon"
 #define KEY_NAME "display-name"
 #define KEY_PERCENT "percent-done"
 #define KEY_SIZE "size"
+#define KEY_SPEED "speed"
 #define KEY_SUBTYPE "subtype"
 #define KEY_TYPE "type"
 #define KEY_UUID "uuid"
 
 // FIXME: this should go in a shared header in include/
-enum
-{
-    ACTION_QUEUED = 0,
-    ACTION_SAVING = 1,
-    ACTION_RESTORING = 2,
-    ACTION_IDLE = 3
-};
+static constexpr char const * ACTION_QUEUED = {"queued"};
+static constexpr char const * ACTION_SAVING = {"saving"};
+static constexpr char const * ACTION_RESTORING = {"restoring"};
+static constexpr char const * ACTION_CANCELLED = {"cancelled"};
+static constexpr char const * ACTION_FAILED = {"failed"};
+static constexpr char const * ACTION_COMPLETE = {"complete"};
 
 using namespace QtDBusMock;
 using namespace QtDBusTest;