[DC-150] refactor Account shared pointer (#12311)

* new point

* switch shared pointer to qpointer in connection validater and added nullptr checks

* removed accessor for nam on abstract core job
refactored CheckServerJobFactory to take a raw account pointer. Also removed the parent arg to the factory as it's fairly useless
adjusted the parenting of the one-off nam created in the factory to be sure it is parented by the job, so it is cleaned up along with the job.
minimal update to oauth to deal with the new factory interface

* lots of small cleanups/updates/removing accessors that have no business existing.

* removed the oc10 related folderwizardremotepath and updated deps
also cleaned up some additional accessors in the abstract network job

* updated AccountBasedOAuth and request auth impls to use *account

* comments and fixed typo

* removed dep on account from activity data and updated uses

* small cleanups, removed some accessors, including those to get the account, and got rid of account member for DiscoverySingleDirectoryJob as it was never used

* changed folder wizard and related class dependencies from the account state to the spacesmanager as that was all that was needed
eliminated more accessors to get the account, replaced more accountptr with just account*.
identified cases where the account dep can be removed completely with correct refactoring to make jobs only run in controllers/managers (where it's reasonable).
removed no longer used deprecated function from SpacesManager

* fixed forward declaration

* fixed the long standing issue in folderstatusmodel with the convoluted setAccountStatus function. Now the account status handling is much safer and clearer.

* make gcc happy and added todo's to really fix this mess

* did an interim refactor of the account storage in socketapi. Instead of a qset of accountptr instances I changed it to a hash of uuid -> QPointer<account>
I think we can actually replace this with a qset of defaultSyncRoot since the roots are unique to the account, and that is all the class needs to function, but don't want to get too far off track with this pr so do that another time

* more random conversions from shared to raw or q pointer

* updated the syncEngine account pointer

* AbstractNetworkJob now uses QPointer for the account (as do all subclasses now)

* made account member in AbstractNetworkJob private to ensure it's not used directly by subclasses
updated fetchServerSettings and discovery classes

* updated propagator classes

* finished the bulk of the updates. vfs is still to be updated so currently vfs tests don't pass - I am pushing this so I can pull the vfs changes

* merged the vfs changes in, updated the accountPtr to raw pointer in vfs - still need to turn it into a qpointer and check for nulls but it works now (local tests passed too) so should be no problem.

* removed accountfwd.h and all associated refs to AccountPtr - it's now gone.
also did some basic cleanup on some includes as well as updating to #pragma once where I ran into it

* missed a nullptr check that crashed on quit immediately after adding account

Author: modSpike

src/common/vfs.cpp

@@ -276,7 +276,7 @@ const VfsPluginManager &VfsPluginManager::instance()
     return *_instance;
 }
 
-VfsSetupParams::VfsSetupParams(const AccountPtr &account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine)
+VfsSetupParams::VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine)
     : account(account)
     , _baseUrl(baseUrl)
     , _groupInSidebar(groupInSidebar)

src/common/vfs.h

@@ -13,33 +13,38 @@
  */
 #pragma once
 
+// if this is included, to support a qpointer on the account, all hell breaks loose wrt endless "cannot open include file: 'owncloudlib.h'
+// No such file or directory" which is fundamental to the libsync exports
+// so unfortunately we can't make a qpointer for the account here - do the update described below asap to get rid of the dependency
+// #include "libsync/account.h"
 #include "assert.h"
 #include "csync/csync.h"
-#include "libsync/accountfwd.h"
 #include "ocsynclib.h"
 #include "pinstate.h"
 #include "result.h"
 #include "syncfilestatus.h"
 #include "utility.h"
 
 #include <QObject>
-#include <QSharedPointer>
+#include <QPointer>
 #include <QUrl>
 #include <QVersionNumber>
 
 #include <memory>
 
+
 namespace OCC {
 
 class Account;
+
 class SyncJournalDb;
 class SyncFileItem;
 class SyncEngine;
 
 /** Collection of parameters for initializing a Vfs instance. */
 struct OCSYNC_EXPORT VfsSetupParams
 {
-    explicit VfsSetupParams(const AccountPtr &account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine);
+    explicit VfsSetupParams(Account *account, const QUrl &baseUrl, bool groupInSidebar, SyncEngine *syncEngine);
     /** The full path to the folder on the local filesystem
      *
      * Always ends with /.
@@ -53,7 +58,7 @@ struct OCSYNC_EXPORT VfsSetupParams
     QString remotePath;
 
     /// Account url, credentials etc. for network calls
-    AccountPtr account;
+    Account *account;
 
     /** Access to the sync folder's database.
      *

src/gui/accountmanager.cpp

@@ -109,50 +109,53 @@ bool AccountManager::restore()
         if (auto acc = loadAccountHelper(*settings)) {
             acc->_groupIndex = accountIndex;
             if (auto accState = AccountState::loadFromSettings(acc, *settings)) {
-                addAccountState(std::move(accState));
+                addAccountState(accState);
             }
         }
         settings->endGroup();
     }
 
     return true;
 }
-  
-AccountPtr AccountManager::createAccount(const NewAccountModel &model)
+
+Account *AccountManager::createAccount(const NewAccountModel &model)
 {
-    auto newAccountPtr = Account::create(QUuid::createUuid(), model.davUser(), model.effectiveUserInfoUrl());
+    auto account = new Account(QUuid::createUuid(), model.davUser(), model.effectiveUserInfoUrl());
 
-    newAccountPtr->setDavDisplayName(model.displayName());
+    account->setDavDisplayName(model.displayName());
 
-    Credentials *creds = new Credentials(model.authToken(), model.refreshToken(), newAccountPtr.get());
-    newAccountPtr->setCredentials(creds);
+    Credentials *creds = new Credentials(model.authToken(), model.refreshToken(), account);
+    account->setCredentials(creds);
 
-    newAccountPtr->addApprovedCerts(model.trustedCertificates());
+    account->addApprovedCerts(model.trustedCertificates());
     QString syncRoot = model.defaultSyncRoot();
     if (!syncRoot.isEmpty()) {
-        newAccountPtr->setDefaultSyncRoot(syncRoot);
+        account->setDefaultSyncRoot(syncRoot);
         if (!QFileInfo::exists(syncRoot)) {
             OC_ASSERT(QDir().mkpath(syncRoot));
         }
-        Utility::markDirectoryAsSyncRoot(syncRoot, newAccountPtr->uuid());
+        Utility::markDirectoryAsSyncRoot(syncRoot, account->uuid());
     }
 
-    newAccountPtr->setCapabilities(model.capabilities());
+    account->setCapabilities(model.capabilities());
 
-    return newAccountPtr;
+    return account;
 }
 
 void AccountManager::save()
 {
     for (const auto &acc : std::as_const(_accounts)) {
-        saveAccount(acc->account().data());
+        saveAccount(acc->account());
     }
 
     qCInfo(lcAccountManager) << "Saved all account settings";
 }
 
 void AccountManager::saveAccount(Account *account)
 {
+    if (!account)
+        return;
+
     qCDebug(lcAccountManager) << "Saving account" << account->url().toString();
     auto settings = ConfigFile::settingsWithGroup(accountsC());
     settings->beginGroup(account->groupIndex());
@@ -206,9 +209,9 @@ QStringList AccountManager::accountNames() const
 bool AccountManager::accountForLoginExists(const QUrl &url, const QString &davUser) const
 {
     for (const auto &state : _accounts) {
-        if (state) {
-            AccountPtr account = state->account();
-            if (account && account->url() == url && account->davUser() == davUser) {
+        if (state && state->account()) {
+            Account *account = state->account();
+            if (account->url() == url && account->davUser() == davUser) {
                 return true;
             }
         }
@@ -221,18 +224,19 @@ const QList<AccountState *> AccountManager::accounts() const
     return _accounts.values();
 }
 
-AccountPtr AccountManager::loadAccountHelper(QSettings &settings)
+Account *AccountManager::loadAccountHelper(QSettings &settings)
 {
     QVariant urlConfig = settings.value(urlC());
     if (!urlConfig.isValid()) {
         // No URL probably means a corrupted entry in the account settings
         qCWarning(lcAccountManager) << "No URL for account " << settings.group();
-        return AccountPtr();
+        return nullptr;
     }
+
     QString user = settings.value(davUserC()).toString();
     if (user.isEmpty()) {
         qCWarning(lcAccountManager) << "No user name provided for account " << settings.group();
-        return AccountPtr();
+        return nullptr;
     }
     QUrl url = urlConfig.toUrl();
 
@@ -260,7 +264,7 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings)
             if (key.contains(uid.toString(QUuid::WithoutBraces)))
                 credsGroup->remove(key);
         }
-        return AccountPtr();
+        return nullptr;
     }
 
     // 7.0 change - this special handling can be removed in 8.0
@@ -275,13 +279,13 @@ AccountPtr AccountManager::loadAccountHelper(QSettings &settings)
     }
 
 
-    auto acc = Account::create(uid, user, url);
+    auto acc = new Account(uid, user, url);
 
     acc->setDavDisplayName(settings.value(davUserDisplyNameC()).toString());
     acc->setCapabilities(caps);
     acc->setDefaultSyncRoot(settings.value(defaultSyncRootC()).toString());
 
-    acc->setCredentials(new Credentials(acc.get()));
+    acc->setCredentials(new Credentials(acc));
 
     // now the server cert, it is in the general group
     settings.beginGroup("General");
@@ -321,7 +325,7 @@ AccountState *AccountManager::accountState(const QUuid uuid)
     return _accounts.value(uuid);
 }
 
-AccountState *AccountManager::addAccount(const AccountPtr &newAccount)
+AccountState *AccountManager::addAccount(Account *newAccount)
 {
     auto id = newAccount->groupIndex();
     if (id.isEmpty() || !isAccountIndexAvailable(id)) {
@@ -330,7 +334,7 @@ AccountState *AccountManager::addAccount(const AccountPtr &newAccount)
     newAccount->_groupIndex = id;
 
 
-    return addAccountState(AccountState::fromNewAccount(newAccount));
+    return addAccountState(new AccountState(newAccount));
 }
 
 void AccountManager::deleteAccount(AccountState *account)
@@ -412,23 +416,22 @@ QString AccountManager::generateFreeAccountIndex() const
     }
 }
 
-AccountState *AccountManager::addAccountState(std::unique_ptr<AccountState> &&accountState)
+AccountState *AccountManager::addAccountState(AccountState *accountState)
 {
-    AccountState *statePtr = accountState.release();
-    if (!statePtr) // just bail. I have no idea why this is happening but fine. it's null and not usable
-        return statePtr;
+    if (!accountState || !accountState->account()) // just bail. I have no idea why this is happening but fine. it's null and not usable
+        return nullptr;
+
+    _accounts.insert(accountState->account()->uuid(), accountState);
 
-    _accounts.insert(statePtr->account()->uuid(), statePtr);
+    Account *acc = accountState->account();
 
-    auto *rawAccount = statePtr->account().get();
     // this slot can't be connected until the account state exists because saveAccount uses the state
-    connect(rawAccount, &Account::wantsAccountSaved, this, [rawAccount, this] { saveAccount(rawAccount); });
+    connect(acc, &Account::wantsAccountSaved, this, [acc, this] { saveAccount(acc); });
 
-    Q_EMIT accountAdded(statePtr);
+    Q_EMIT accountAdded(accountState);
     Q_EMIT accountsChanged();
+    accountState->checkConnectivity();
 
-    statePtr->checkConnectivity();
-
-    return statePtr;
+    return accountState;
 }
 }

src/gui/accountmanager.h

@@ -53,7 +53,7 @@ class OWNCLOUDGUI_EXPORT AccountManager : public QObject
      * @param model contains the data used to set up the new account (usually collected from the new account wizard)
      * @return the AccountPtr associated with the new account. for now.
      */
-    AccountPtr createAccount(const NewAccountModel &model);
+    Account *createAccount(const NewAccountModel &model);
 
     /**
      * Creates account objects from a given settings file.
@@ -68,7 +68,7 @@ class OWNCLOUDGUI_EXPORT AccountManager : public QObject
      * Typically called from the wizard
      */
     // todo: #28 - this should not be public and should not be called directly by any third party
-    AccountState *addAccount(const AccountPtr &newAccount);
+    AccountState *addAccount(Account *newAccount);
 
     /**
      * remove all accounts
@@ -119,15 +119,16 @@ public Q_SLOTS:
 private:
     // saving and loading Account to settings
     void saveAccountHelper(Account *account, QSettings &settings, bool saveCredentials = true);
-    AccountPtr loadAccountHelper(QSettings &settings);
+    Account *loadAccountHelper(QSettings &settings);
 
     bool isAccountIndexAvailable(const QString &index) const;
     QString generateFreeAccountIndex() const;
 
     // Adds an account to the tracked list, emitting accountAdded()
-    AccountState *addAccountState(std::unique_ptr<AccountState> &&accountState);
+    AccountState *addAccountState(AccountState *accountState);
 
     AccountManager() {}
+
     QMap<QUuid, AccountState *> _accounts;
 };
 }