Merge lp:~michihenning/storage-framework/api2 into lp:storage-framework/devel

[Michi Henning]

First batch of fixes from code review:
Removed CONSTANT from Q_PROPERTY definitions of Item and Account.
Moved qHash() into correct namespace.
Adjusted hash() and qHash() of Item to also combine with the account hash.
Adjusted operator==() and operator<() of Item to compare equal or less than only if the accounts also compare equal or less than.
make_test_account() trailing arguments are now defaulted, so we don't need an overload.
Chained the RuntimeImpl constructors.

First batch of fixes from code review:
Removed CONSTANT from Q_PROPERTY definitions of Item and Account.
Moved qHash() into correct namespace.
Adjusted hash() and qHash() of Item to also combine with the account hash.
Adjusted operator==() and operator<() of Item to compare equal or less than only if the accounts also compare equal or less than.
make_test_account() trailing arguments are now defaulted, so we don't need an overload.
Chained the RuntimeImpl constructors.

[unity-api-1-bot (unity-api-1-bot)]

PASSED: Continuous integration, rev:84
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/126/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/729
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/735
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/543/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/543
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/543/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-storage-framework-ci/126/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

Looks good.

review: Approve

=== modified file 'include/unity/storage/qt/Account.h'
--- include/unity/storage/qt/Account.h	2016-09-20 04:53:49 +0000
+++ include/unity/storage/qt/Account.h	2016-09-22 12:00:14 +0000
@@ -42,10 +42,10 @@
 class Q_DECL_EXPORT Account final
 {
     Q_GADGET
-    Q_PROPERTY(bool READ isValid CONSTANT FINAL)
-    Q_PROPERTY(QString READ owner CONSTANT FINAL)
-    Q_PROPERTY(QString READ ownerId CONSTANT FINAL)
-    Q_PROPERTY(QString READ description CONSTANT FINAL)
+    Q_PROPERTY(bool READ isValid FINAL)
+    Q_PROPERTY(QString READ owner FINAL)
+    Q_PROPERTY(QString READ ownerId FINAL)
+    Q_PROPERTY(QString READ description FINAL)
 
 public:
     Account();
@@ -81,6 +81,8 @@
     friend class internal::ItemImpl;
 };
 
+uint Q_DECL_EXPORT qHash(Account const& acc);
+
 }  // namespace qt
 }  // namespace storage
 }  // namespace unity
@@ -97,7 +99,3 @@
 };
 
 }  // namespace std
-
-// Note: qHash(Account) does *not* return the same hash value is std::hash<Account> because
-//       std:hash() returns size_t (typically 64 bits), but qHash() returns uint (typically 32 bits).
-uint Q_DECL_EXPORT qHash(unity::storage::qt::Account const& acc);

=== modified file 'include/unity/storage/qt/Item.h'
--- include/unity/storage/qt/Item.h	2016-09-21 01:02:46 +0000
+++ include/unity/storage/qt/Item.h	2016-09-22 12:00:14 +0000
@@ -53,14 +53,14 @@
 class Q_DECL_EXPORT Item final
 {
     Q_GADGET
-    Q_PROPERTY(QString itemId READ itemId CONSTANT FINAL)
-    Q_PROPERTY(QString name READ name CONSTANT FINAL)
-    Q_PROPERTY(unity::storage::qt::Account account READ account CONSTANT FINAL)
-    Q_PROPERTY(QString etag READ etag CONSTANT FINAL)
-    Q_PROPERTY(unity::storage::qt::Item::Type type READ type CONSTANT FINAL)
-    Q_PROPERTY(QVariantMap metadata READ metadata CONSTANT FINAL)
-    Q_PROPERTY(QDateTime lastModifiedTime READ lastModifiedTime CONSTANT FINAL)
-    Q_PROPERTY(QVector<QString> parentIds READ parentIds CONSTANT FINAL)
+    Q_PROPERTY(QString itemId READ itemId FINAL)
+    Q_PROPERTY(QString name READ name FINAL)
+    Q_PROPERTY(unity::storage::qt::Account account READ account FINAL)
+    Q_PROPERTY(QString etag READ etag FINAL)
+    Q_PROPERTY(unity::storage::qt::Item::Type type READ type FINAL)
+    Q_PROPERTY(QVariantMap metadata READ metadata FINAL)
+    Q_PROPERTY(QDateTime lastModifiedTime READ lastModifiedTime FINAL)
+    Q_PROPERTY(QVector<QString> parentIds READ parentIds FINAL)
 
 public:
     Item();
@@ -116,6 +116,10 @@
     friend class internal::ItemImpl;
 };
 
+// Note: qHash(Item) does *not* return the same hash value is std::hash<Item> because
+//       std:hash() returns size_t (typically 64 bits), but qHash() returns uint (typically 32 bits).
+uint Q_DECL_EXPORT qHash(unity::storage::qt::Item const& i);
+
 }  // namespace qt
 }  // namespace storage
 }  // namespace unity
@@ -136,7 +140,3 @@
 };
 
 }  // namespace std
-
-// Note: qHash(Item) does *not* return the same hash value is std::hash<Item> because
-//       std:hash() returns size_t (typically 64 bits), but qHash() returns uint (typically 32 bits).
-uint Q_DECL_EXPORT qHash(unity::storage::qt::Item const& i);

=== modified file 'include/unity/storage/qt/Runtime.h'
--- include/unity/storage/qt/Runtime.h	2016-09-21 00:29:49 +0000
+++ include/unity/storage/qt/Runtime.h	2016-09-22 12:00:14 +0000
@@ -62,14 +62,11 @@
     StorageError shutdown();
     Q_INVOKABLE AccountsJob* accounts() const;
 
-    // TODO: Get rid of two-argument version and default trailing params.
-    // TODO: can be const methods.
-    Account make_test_account(QString const& bus_name, QString const& object_path);
     Account make_test_account(QString const& bus_name,
                               QString const& object_path,
-                              QString const& owner_id,
-                              QString const& owner,
-                              QString const& description);
+                              QString const& owner_id = "",
+                              QString const& owner = "",
+                              QString const& description = "") const;
 
 private:
     std::shared_ptr<internal::RuntimeImpl> p_;

=== modified file 'include/unity/storage/qt/internal/RuntimeImpl.h'
--- include/unity/storage/qt/internal/RuntimeImpl.h	2016-09-21 00:29:49 +0000
+++ include/unity/storage/qt/internal/RuntimeImpl.h	2016-09-22 12:00:14 +0000
@@ -21,11 +21,10 @@
 #include <unity/storage/qt/Account.h>
 #include <unity/storage/qt/StorageError.h>
 
-#include <OnlineAccounts/Manager>
-
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wcast-align"
 #pragma GCC diagnostic ignored "-Wctor-dtor-privacy"
+#include <OnlineAccounts/Manager>
 #include <QDBusConnection>
 #pragma GCC diagnostic pop
 
@@ -62,9 +61,6 @@
     std::shared_ptr<OnlineAccounts::Manager> accounts_manager() const;
 
     Account make_test_account(QString const& bus_name,
-                              QString const& object_path);
-
-    Account make_test_account(QString const& bus_name,
                               QString const& object_path,
                               QString const& owner_id,
                               QString const& owner,

=== modified file 'src/qt/Account.cpp'
--- src/qt/Account.cpp	2016-09-20 02:24:36 +0000
+++ src/qt/Account.cpp	2016-09-22 12:00:14 +0000
@@ -138,11 +138,13 @@
     return p_->hash();
 }
 
+// Due to potentially different size of size_t and uint, hash() and qhash() may not return the same value.
+
+uint qHash(Account const& acc)
+{
+    return acc.hash();
+}
+
 }  // namespace qt
 }  // namespace storage
 }  // namespace unity
-
-uint qHash(unity::storage::qt::Account const& acc)
-{
-    return acc.hash();
-}

=== modified file 'src/qt/Item.cpp'
--- src/qt/Item.cpp	2016-09-20 02:24:36 +0000
+++ src/qt/Item.cpp	2016-09-22 12:00:14 +0000
@@ -220,11 +220,13 @@
     return p_->hash();
 }
 
+// Due to potentially different size of size_t and uint, hash() and qhash() may not return the same value.
+
+uint qHash(Item const& i)
+{
+    return i.hash();
+}
+
 }  // namespace qt
 }  // namespace storage
 }  // namespace unity
-
-uint qHash(unity::storage::qt::Item const& i)
-{
-    return i.hash();
-}

=== modified file 'src/qt/Runtime.cpp'
--- src/qt/Runtime.cpp	2016-09-16 06:25:08 +0000
+++ src/qt/Runtime.cpp	2016-09-22 12:00:14 +0000
@@ -68,16 +68,11 @@
     return p_->accounts();
 }
 
-Account Runtime::make_test_account(QString const& bus_name, QString const& object_path)
-{
-    return p_->make_test_account(bus_name, object_path);
-}
-
 Account Runtime::make_test_account(QString const& bus_name,
                                    QString const& object_path,
                                    QString const& owner_id,
                                    QString const& owner,
-                                   QString const& description)
+                                   QString const& description) const
 {
     return p_->make_test_account(bus_name, object_path, owner_id, owner, description);
 }

=== modified file 'src/qt/internal/AccountImpl.cpp'
--- src/qt/internal/AccountImpl.cpp	2016-09-20 04:53:49 +0000
+++ src/qt/internal/AccountImpl.cpp	2016-09-22 12:00:14 +0000
@@ -169,11 +169,7 @@
     {
         return false;
     }
-    if (description_ < other.description_)
-    {
-        return true;
-    }
-    return false;
+    return description_ < other.description_;
 }
 
 bool AccountImpl::operator<=(AccountImpl const& other) const

=== modified file 'src/qt/internal/ItemImpl.cpp'
--- src/qt/internal/ItemImpl.cpp	2016-09-20 02:24:36 +0000
+++ src/qt/internal/ItemImpl.cpp	2016-09-22 12:00:14 +0000
@@ -25,6 +25,8 @@
 #include <unity/storage/qt/internal/RuntimeImpl.h>
 #include <unity/storage/qt/internal/validate.h>
 
+#include <boost/functional/hash.hpp>
+
 #include <cassert>
 
 using namespace std;
@@ -176,7 +178,9 @@
 {
     if (is_valid_)
     {
-        return other.is_valid_ && md_.item_id == other.md_.item_id;
+        return other.is_valid_
+               && *account_ == *other.account_
+               && md_.item_id == other.md_.item_id;
     }
     return !other.is_valid_;
 }
@@ -188,11 +192,24 @@
 
 bool ItemImpl::operator<(ItemImpl const& other) const
 {
-    if (is_valid_)
-    {
-        return other.is_valid_ && md_.item_id < other.md_.item_id;
-    }
-    return other.is_valid_;
+    if (!is_valid_)
+    {
+        return other.is_valid_;
+    }
+    if (is_valid_ && !other.is_valid_)
+    {
+        return false;
+    }
+    assert(is_valid_ && other.is_valid_);
+    if (*account_ < *other.account_)
+    {
+        return true;
+    }
+    if (*account_ > *other.account_)
+    {
+        return false;
+    }
+    return md_.item_id < other.md_.item_id;
 }
 
 bool ItemImpl::operator<=(ItemImpl const& other) const
@@ -216,7 +233,10 @@
     {
         return 0;
     }
-    return qHash(md_.item_id);
+    size_t hash = 0;
+    boost::hash_combine(hash, account_->hash());
+    boost::hash_combine(hash, qHash(md_.item_id));
+    return hash;
 }
 
 Item ItemImpl::make_item(QString const& method,

=== modified file 'src/qt/internal/RuntimeImpl.cpp'
--- src/qt/internal/RuntimeImpl.cpp	2016-09-21 00:53:09 +0000
+++ src/qt/internal/RuntimeImpl.cpp	2016-09-22 12:00:14 +0000
@@ -57,24 +57,9 @@
 
 }
 
-// TODO: chain the two constructors.
 RuntimeImpl::RuntimeImpl()
-    : is_valid_(true)
-    , conn_(QDBusConnection::sessionBus())
-    , accounts_manager_(new OnlineAccounts::Manager("", conn_))
+    : RuntimeImpl(QDBusConnection::sessionBus())
 {
-    register_meta_types();
-
-#if 0
-    if (!conn_.isConnected())
-    {
-        // LCOV_EXCL_START
-        is_valid_ = false;
-        QString msg = "Runtime(): cannot connect to session bus: " + conn_.lastError().message();
-        error_ = StorageErrorImpl::local_comms_error(msg);
-        // LCOV_EXCL_STOP
-    }
-#endif
 }
 
 RuntimeImpl::RuntimeImpl(QDBusConnection const& bus)
@@ -83,15 +68,6 @@
     , accounts_manager_(new OnlineAccounts::Manager("", conn_))
 {
     register_meta_types();
-
-#if 0
-    if (!conn_.isConnected())
-    {
-        is_valid_ = false;
-        QString msg = "Runtime(): DBus connection is not connected";
-        error_ = StorageErrorImpl::local_comms_error(msg);
-    }
-#endif
 }
 
 RuntimeImpl::~RuntimeImpl()
@@ -143,12 +119,6 @@
 }
 
 Account RuntimeImpl::make_test_account(QString const& bus_name,
-                                       QString const& object_path)
-{
-    return make_test_account(bus_name, object_path, "", "", "");
-}
-
-Account RuntimeImpl::make_test_account(QString const& bus_name,
                                        QString const& object_path,
                                        QString const& owner_id,
                                        QString const& owner,