From 406a251c855a63f623230449b66a93466150a759 Mon Sep 17 00:00:00 2001
From: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com>
Date: Fri, 7 Jun 2024 14:44:02 -0400
Subject: [PATCH] accountmodel: improve handling for account list reordering
This update fixes an issue where a stale account list might be used
in AccountListModel by synchronizing list invalidation with a newly
introduced signal (accountsReordered). This change prevents a data race
that could occur due to the asynchronous nature of setting the account
order over D-Bus, or when queuing the signal handling for
accountsChanged, which is emitted only once the reordering is saved.
As a result, QAbstractItemModel mutations are now performed within AccountListModel instead of in the UI.
Gitlab: #1638 (Account list in popup is incorrect after selection)
Change-Id: I7ed6eeb45eb319f21e40554f3d023ad24e139a6f
---
src/app/accountlistmodel.cpp | 30 +++++++++++++++++++
src/app/lrcinstance.cpp | 10 +++++--
.../mainview/components/AccountComboBox.qml | 18 -----------
.../components/AccountComboBoxPopup.qml | 22 --------------
src/libclient/accountmodel.cpp | 3 +-
src/libclient/api/accountmodel.h | 4 +++
6 files changed, 43 insertions(+), 44 deletions(-)
diff --git a/src/app/accountlistmodel.cpp b/src/app/accountlistmodel.cpp
index ac01baccc..2b70f7834 100644
--- a/src/app/accountlistmodel.cpp
+++ b/src/app/accountlistmodel.cpp
@@ -29,6 +29,35 @@ AccountListModel::AccountListModel(LRCInstance* instance, QObject* parent)
: AbstractListModelBase(parent)
{
lrcInstance_ = instance;
+
+ // Avoid resetting/redrawing the model when the account status changes.
+ QObject::connect(&lrcInstance_->accountModel(),
+ &AccountModel::accountStatusChanged,
+ this,
+ [&](const QString& accountId) {
+ auto accountList = lrcInstance_->accountModel().getAccountList();
+ auto index = accountList.indexOf(accountId);
+ if (index != -1) {
+ QModelIndex modelIndex = QAbstractListModel::index(index, 0);
+ Q_EMIT dataChanged(modelIndex, modelIndex /*, ALL ROLES */);
+ }
+ });
+ // If there's a reorder, it's reasonable to reset the model for simplicity, instead
+ // of computing the difference. The same goes for accounts being added and removed.
+ // These operations will only occur when the list is hidden, unless dbus is used while
+ // the list is visible.
+ QObject::connect(&lrcInstance_->accountModel(),
+ &AccountModel::accountsReordered,
+ this,
+ &AccountListModel::reset);
+ QObject::connect(&lrcInstance_->accountModel(),
+ &AccountModel::accountAdded,
+ this,
+ &AccountListModel::reset);
+ QObject::connect(&lrcInstance_->accountModel(),
+ &AccountModel::accountRemoved,
+ this,
+ &AccountListModel::reset);
}
int
@@ -91,6 +120,7 @@ AccountListModel::roleNames() const
void
AccountListModel::reset()
{
+ // Used to invalidate proxy models.
beginResetModel();
endResetModel();
}
diff --git a/src/app/lrcinstance.cpp b/src/app/lrcinstance.cpp
index af6247934..e7c381ad5 100644
--- a/src/app/lrcinstance.cpp
+++ b/src/app/lrcinstance.cpp
@@ -43,9 +43,8 @@ LRCInstance::LRCInstance(const QString& updateUrl,
muteDaemon_ = muteDaemon;
threadPool_->setMaxThreadCount(1);
- connect(this, &LRCInstance::currentAccountIdChanged, [this] {
- // save to config, editing the accountlistmodel's underlying data
- accountModel().setTopAccount(currentAccountId_);
+ // Update the current account when the account list changes.
+ connect(&accountModel(), &AccountModel::accountsReordered, this, [this] {
Q_EMIT accountListChanged();
profile::Info profileInfo;
@@ -62,6 +61,11 @@ LRCInstance::LRCInstance(const QString& updateUrl,
set_currentAccountAvatarSet(!profileInfo.avatar.isEmpty());
});
+ connect(this, &LRCInstance::currentAccountIdChanged, [this] {
+ // This will trigger `AccountModel::accountsReordered`.
+ accountModel().setTopAccount(currentAccountId_);
+ });
+
connect(&accountModel(), &AccountModel::profileUpdated, this, [this](const QString& id) {
if (id != currentAccountId_)
return;
diff --git a/src/app/mainview/components/AccountComboBox.qml b/src/app/mainview/components/AccountComboBox.qml
index ea149f614..7173cc80d 100644
--- a/src/app/mainview/components/AccountComboBox.qml
+++ b/src/app/mainview/components/AccountComboBox.qml
@@ -34,24 +34,6 @@ Label {
property bool inSettings: viewCoordinator.currentViewName === "SettingsView"
- // TODO: remove these refresh hacks use QAbstractItemModels correctly
- Connections {
- target: AccountAdapter
-
- function onAccountStatusChanged(accountId) {
- AccountListModel.reset();
- }
- }
-
- Connections {
- target: LRCInstance
-
- function onAccountListChanged() {
- root.update();
- AccountListModel.reset();
- }
- }
-
function togglePopup() {
if (root.popup.opened) {
root.popup.close();
diff --git a/src/app/mainview/components/AccountComboBoxPopup.qml b/src/app/mainview/components/AccountComboBoxPopup.qml
index 9bfac5c91..35118d233 100644
--- a/src/app/mainview/components/AccountComboBoxPopup.qml
+++ b/src/app/mainview/components/AccountComboBoxPopup.qml
@@ -60,23 +60,6 @@ Popup {
property bool inSettings: viewCoordinator.currentViewName === "SettingsView"
- // TODO: remove these refresh hacks use QAbstractItemModels correctly
- Connections {
- target: AccountAdapter
-
- function onAccountStatusChanged(accountId) {
- AccountListModel.reset();
- }
- }
-
- Connections {
- target: LRCInstance
-
- function onAccountListChanged() {
- AccountListModel.reset();
- }
- }
-
RowLayout {
id: mainLayout
anchors.fill: parent
@@ -257,11 +240,6 @@ Popup {
color: JamiTheme.smartListHoveredColor
}
- // fake footer item as workaround for Qt 5.15 bug
- // https://bugreports.qt.io/browse/QTBUG-85302
- // don't use the clip trick and footer item overlay
- // explained here https://stackoverflow.com/a/64625149
- // as it causes other complexities in handling the drop shadow
ItemDelegate {
id: addAccountItem
diff --git a/src/libclient/accountmodel.cpp b/src/libclient/accountmodel.cpp
index 45983b970..07af96f4c 100644
--- a/src/libclient/accountmodel.cpp
+++ b/src/libclient/accountmodel.cpp
@@ -428,8 +428,9 @@ AccountModelPimpl::updateAccounts()
const auto previousAccountIdListSize = accountIdList.size();
accountIdList = configurationManager.getAccountList();
- // If this is just a reordering of the accounts, don't do anything
+ // If this is just a reordering of the accounts, just notify the view.
if (accountIdList.size() == previousAccountIdListSize) {
+ Q_EMIT linked.accountsReordered();
return;
}
diff --git a/src/libclient/api/accountmodel.h b/src/libclient/api/accountmodel.h
index 6f199811f..ba58eddf2 100644
--- a/src/libclient/api/accountmodel.h
+++ b/src/libclient/api/accountmodel.h
@@ -288,6 +288,10 @@ Q_SIGNALS:
* @param accountID
*/
void accountRemoved(const QString& accountID);
+ /**
+ * Emitted when the account list order has changed.
+ */
+ void accountsReordered();
/**
* Connect this signal to know when an account was updated.
* @param accountID
--
GitLab