AccountListModel: avoid calling getAccountList when unneeded

Gitlab: #1459
Change-Id: I962f38935acd8e97895587076a448125213fc4bc

src/app/MainApplicationWindow.qml

@@ -269,6 +269,12 @@ ApplicationWindow {
             raise();
             layoutManager.restoreApp();
         }
+
+        function onCurrentAccountRemoved() {
+            if (UtilsAdapter.getAccountListSize() === 0) {
+                viewCoordinator.present("WizardView");
+            }
+        }
     }
 
     Connections {

src/app/accountadapter.cpp

@@ -227,7 +227,7 @@ AccountAdapter::createJAMSAccount(const QVariantMap& settings)
         &lrcInstance_->accountModel(),
         &lrc::api::AccountModel::accountAdded,
         [this](const QString& accountId) {
-            if (!lrcInstance_->accountModel().getAccountList().size())
+            if (!lrcInstance_->accountModel().getAccountCount())
                 return;
 
             Utils::oneShotConnect(&lrcInstance_->accountModel(),

src/app/accountlistmodel.cpp

@@ -35,7 +35,7 @@ int
 AccountListModel::rowCount(const QModelIndex& parent) const
 {
     if (!parent.isValid() && lrcInstance_) {
-        return lrcInstance_->accountModel().getAccountList().size();
+        return lrcInstance_->accountModel().getAccountCount();
     }
     return 0;
 }
@@ -71,7 +71,7 @@ AccountListModel::data(const QModelIndex& index, int role) const
 void
 AccountListModel::updateNotifications()
 {
-    for (int i = 0; i < lrcInstance_->accountModel().getAccountList().size(); ++i) {
+    for (int i = 0; i < lrcInstance_->accountModel().getAccountCount(); ++i) {
         QModelIndex modelIndex = QAbstractListModel::index(i, 0);
         Q_EMIT dataChanged(modelIndex, modelIndex, {Role::NotificationCount});
     }

src/app/bannedlistmodel.cpp

@@ -20,6 +20,7 @@
 #include "bannedlistmodel.h"
 
 #include "lrcinstance.h"
+#include "global.h"
 
 #include <api/contact.h>
 
@@ -108,7 +109,13 @@ void
 BannedListModel::reset()
 {
     beginResetModel();
-    bannedlist_ = lrcInstance_->getCurrentAccountInfo().contactModel->getBannedContacts();
+    auto contactModel = lrcInstance_->getCurrentContactModel();
+    if (!contactModel) {
+        C_DBG << "Contact model is not available.";
+        bannedlist_.clear();
+    } else {
+        bannedlist_ = contactModel->getBannedContacts();
+    }
     endResetModel();
     set_count(rowCount());
 }

src/app/conversationsadapter.cpp

@@ -603,9 +603,11 @@ ConversationsAdapter::openDialogConversationWith(const QString& peerUri)
 void
 ConversationsAdapter::onCurrentAccountRemoved()
 {
-    // Unbind proxy model source models.
+    // Unbind proxy model source models if there is no current account
+    if (lrcInstance_->get_currentAccountId().isEmpty()) {
         convModel_->bindSourceModel(nullptr);
         searchModel_->bindSourceModel(nullptr);
+    }
 }
 
 void

src/app/lrcinstance.cpp

@@ -282,7 +282,7 @@ LRCInstance::getCurrentContactModel()
 int
 LRCInstance::getCurrentAccountIndex()
 {
-    for (int i = 0; i < accountModel().getAccountList().size(); i++) {
+    for (int i = 0; i < accountModel().getAccountCount(); i++) {
         if (accountModel().getAccountList()[i] == get_currentAccountId()) {
             return i;
         }
@@ -410,6 +410,10 @@ LRCInstance::indexOfActiveCall(const QString& confId, const QString& uri, const
 void
 LRCInstance::deselectConversation()
 {
+    // Only do this if we have an account selected
+    if (get_currentAccountId().isEmpty()) {
+        return;
+    }
     set_selectedConvUid();
 }
 
@@ -482,5 +486,15 @@ LRCInstance::onAccountRemoved(const QString& accountId)
 {
     if (accountId != currentAccountId_)
         return;
+
+    // If there are any accounts left, select the first one, otherwise clear the current account
+    // and request presentation of the wizard view.
+    auto accountList = accountModel().getAccountList();
+    if (accountList.size()) {
+        set_currentAccountId(accountList.at(0));
+    } else {
+        set_currentAccountId();
+    }
+
     Q_EMIT currentAccountRemoved();
 }

src/app/settingsview/SettingsView.qml

@@ -75,11 +75,6 @@ ListSelectionView {
         // Trigger an update to messages if needed.
         // Currently needed when changing the show link preview setting.
         CurrentConversation.reloadInteractions();
-        if (UtilsAdapter.getAccountListSize() === 0) {
-            viewCoordinator.present("WizardView");
-        } else {
-            AccountAdapter.changeAccount(0);
-        }
     }
 
     property int selectedMenu: index

src/app/utilsadapter.cpp

@@ -274,7 +274,7 @@ UtilsAdapter::setConversationFilter(const QString& filter)
 int
 UtilsAdapter::getAccountListSize()
 {
-    return lrcInstance_->accountModel().getAccountList().size();
+    return lrcInstance_->accountModel().getAccountCount();
 }
 
 bool

src/libclient/accountmodel.cpp

@@ -73,6 +73,7 @@ public:
     Lrc& lrc;
     const CallbacksHandler& callbacksHandler;
     const BehaviorController& behaviorController;
+    QList<QString> accountIdList;
     AccountInfoDbMap accounts;
 
     // Synchronization tools
@@ -200,20 +201,16 @@ AccountModel::AccountModel(Lrc& lrc,
 
 AccountModel::~AccountModel() {}
 
-QStringList
+const QStringList&
 AccountModel::getAccountList() const
 {
-    QStringList filteredAccountIds;
-    const QStringList accountIds = ConfigurationManager::instance().getAccountList();
+    return pimpl_->accountIdList;
+}
 
-    for (auto const& id : accountIds) {
-        auto account = pimpl_->accounts.find(id);
-        // Do not include accounts flagged for removal
-        if (account != pimpl_->accounts.end() && account->second.first.valid)
-            filteredAccountIds.push_back(id);
-    }
-
-    return filteredAccountIds;
+size_t
+AccountModel::getAccountCount() const
+{
+    return pimpl_->accountIdList.size();
 }
 
 void
@@ -332,12 +329,11 @@ void
 AccountModel::removeAccount(const QString& accountId) const
 {
     auto account = pimpl_->accounts.find(accountId);
-    if (account == pimpl_->accounts.end()) {
-        return;
+    if (account != pimpl_->accounts.end()) {
+        account->second.second->close();
     }
 
     // Close db here for its removal
-    account->second.second->close();
     ConfigurationManager::instance().removeAccount(accountId);
 }
 
@@ -356,7 +352,7 @@ AccountModel::getAccountInfo(const QString& accountId) const
 {
     auto accountInfo = pimpl_->accounts.find(accountId);
     if (accountInfo == pimpl_->accounts.end())
-        throw std::out_of_range("AccountModel::getAccountInfo, can't find "
+        throw std::out_of_range("AccountModel::getAccountInfo, can't find account "
                                 + accountId.toStdString());
 
     return accountInfo->second.first;
@@ -372,18 +368,18 @@ AccountModelPimpl::AccountModelPimpl(AccountModel& linked,
     , callbacksHandler(callbacksHandler)
     , username_changed(false)
 {
-    const QStringList accountIds = ConfigurationManager::instance().getAccountList();
+    accountIdList = ConfigurationManager::instance().getAccountList();
 
     // NOTE: If the daemon is down, but dbus answered, id can contains
     // "Remote peer disconnected", "The name is not activable", etc.
     // So avoid to migrate useless directories.
-    for (auto& id : accountIds)
+    for (const auto& id : accountIdList)
         if (id.indexOf(" ") != -1) {
             qWarning() << "Invalid dbus answer. Daemon not running";
             return;
         }
 
-    for (auto& id : accountIds) {
+    for (const auto& id : accountIdList) {
         addToAccounts(id);
     }
 
@@ -434,15 +430,22 @@ AccountModelPimpl::~AccountModelPimpl() {}
 void
 AccountModelPimpl::updateAccounts()
 {
-    qDebug() << "Syncing lrc accounts list with the daemon";
     ConfigurationManagerInterface& configurationManager = ConfigurationManager::instance();
-    QStringList accountIds = configurationManager.getAccountList();
+    const auto previousAccountIdListSize = accountIdList.size();
+    accountIdList = configurationManager.getAccountList();
+
+    // If this is just a reordering of the accounts, don't do anything
+    if (accountIdList.size() == previousAccountIdListSize) {
+        return;
+    }
+
+    LC_DBG << "Syncing account list";
 
     // Detect removed accounts
     QStringList toBeRemoved;
     for (auto& it : accounts) {
         auto& accountInfo = it.second.first;
-        if (!accountIds.contains(accountInfo.id)) {
+        if (!accountIdList.contains(accountInfo.id)) {
             qDebug() << QString("detected account removal %1").arg(accountInfo.id);
             toBeRemoved.push_back(accountInfo.id);
         }
@@ -453,7 +456,7 @@ AccountModelPimpl::updateAccounts()
     }
 
     // Detect new accounts
-    for (auto& id : accountIds) {
+    for (auto& id : accountIdList) {
         auto account = accounts.find(id);
         // NOTE: If the daemon is down, but dbus answered, id can contains
         // "Remote peer disconnected", "The name is not activable", etc.
@@ -572,7 +575,7 @@ AccountModelPimpl::slotVolatileAccountDetailsChanged(const QString& accountId,
 {
     auto account = accounts.find(accountId);
     if (account == accounts.end()) {
-        qWarning() << Q_FUNC_INFO << ": can't find " << accountId;
+        LC_DBG << Q_FUNC_INFO << ": can't find " << accountId;
         return;
     }
     auto& accountInfo = account->second.first;
@@ -789,7 +792,7 @@ AccountModelPimpl::removeFromAccounts(const QString& accountId)
     /* Inform client about account removal. Do *not* free account structures
        before we are sure that the client stopped using it, otherwise we might
        get into use-after-free troubles. */
-    accountInfo.valid = false;
+    accountIdList.removeOne(accountId);
     Q_EMIT linked.accountRemoved(accountId);
 
     // Now we can free them

src/libclient/api/account.h

@@ -209,7 +209,6 @@ Q_ENUM_NS(LookupStatus)
 
 struct Info
 {
-    bool valid = true;
     QString registeredName;
     Status status = account::Status::INVALID;
     std::unique_ptr<lrc::api::CallModel> callModel;

src/libclient/api/accountmodel.h

@@ -58,7 +58,13 @@ public:
     /**
      * @return a list of all acountId.
      */
-    Q_INVOKABLE QStringList getAccountList() const;
+    Q_INVOKABLE const QStringList& getAccountList() const;
+
+    /**
+     * @return account list size
+     */
+    Q_INVOKABLE size_t getAccountCount() const;
+
     /**
      * get account informations associated to an accountId.
      * @param accountId.

src/libclient/authority/storagehelper.cpp

@@ -364,7 +364,7 @@ withProfile(const QString& accountId,
 
     QFile file(path);
     if (!file.open(flags)) {
-        LC_DBG << "Can't open file: " << path;
+        LC_DBG << "Can't open file:" << path;
         return false;
     }
 

tests/unittests/account_unittest.cpp

@@ -40,7 +40,7 @@ public:
  */
 TEST_F(AccountFixture, InitialAccountListCheck)
 {
-    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountList().size();
+    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountCount();
 
     ASSERT_EQ(accountListSize, 0);
 }
@@ -66,7 +66,7 @@ TEST_F(AccountFixture, CreateSIPAccountTest)
     // Select the created account
     globalEnv.lrcInstance->set_currentAccountId(accountAddedArguments.at(0).toString());
 
-    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountList().size();
+    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountCount();
     ASSERT_EQ(accountListSize, 1);
 
     // Make sure the account setup is done
@@ -86,6 +86,6 @@ TEST_F(AccountFixture, CreateSIPAccountTest)
     accountRemovedSpy.wait();
     EXPECT_EQ(accountRemovedSpy.count(), 1);
 
-    accountListSize = globalEnv.lrcInstance->accountModel().getAccountList().size();
+    accountListSize = globalEnv.lrcInstance->accountModel().getAccountCount();
     ASSERT_EQ(accountListSize, 0);
 }

tests/unittests/contact_unittest.cpp

@@ -100,6 +100,6 @@ TEST_F(ContactFixture, AddSIPContactTest)
     accountRemovedSpy.wait();
     EXPECT_EQ(accountRemovedSpy.count(), 1);
 
-    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountList().size();
+    auto accountListSize = globalEnv.lrcInstance->accountModel().getAccountCount();
     ASSERT_EQ(accountListSize, 0);
 }