From 610c27f75150f86c3c9666364822670cbdfb9675 Mon Sep 17 00:00:00 2001 From: Andreas Traczyk <andreas.traczyk@savoirfairelinux.com> Date: Thu, 15 Feb 2024 17:38:36 -0500 Subject: [PATCH] contactmodel: refactor profile loading Loading the profile elements from disk is now deferred to consumption. Implements a basic caching mechanism for the displayname and avatar elements. Gitlab: #1459 Change-Id: Ic5aeec6649c198f617b9269409ded647c7536b8f --- src/app/avatarimageprovider.h | 66 ++- src/app/avatarregistry.cpp | 3 +- src/app/avatarregistry.h | 2 +- src/app/contactadapter.cpp | 2 +- src/app/conversationlistmodel.cpp | 3 +- src/app/conversationlistmodelbase.cpp | 8 +- src/app/lrcinstance.cpp | 2 + src/app/mainapplication.cpp | 5 +- .../mainview/components/AccountComboBox.qml | 1 + .../components/AccountComboBoxPopup.qml | 1 + .../components/AccountItemDelegate.qml | 1 + .../components/SmartListItemDelegate.qml | 1 + src/app/quickimageproviderbase.h | 63 ++- src/app/utils.cpp | 10 +- src/app/utils.h | 2 + src/app/utilsadapter.cpp | 6 +- src/libclient/accountmodel.cpp | 22 +- src/libclient/api/contactmodel.h | 20 +- src/libclient/authority/storagehelper.cpp | 341 +++++++++------ src/libclient/authority/storagehelper.h | 37 +- src/libclient/contactmodel.cpp | 407 ++++++++---------- src/libclient/conversationmodel.cpp | 64 +-- src/libclient/pluginmodel.cpp | 7 +- src/libclient/typedefs.h | 2 + src/libclient/vcard.cpp | 22 +- tests/unittests/contact_unittest.cpp | 2 +- 26 files changed, 633 insertions(+), 467 deletions(-) diff --git a/src/app/avatarimageprovider.h b/src/app/avatarimageprovider.h index 5b6f50af3..88d62d0c1 100644 --- a/src/app/avatarimageprovider.h +++ b/src/app/avatarimageprovider.h @@ -25,52 +25,74 @@ #include <QImage> -class AvatarImageProvider : public QuickImageProviderBase +class AsyncAvatarImageResponseRunnable : public AsyncImageResponseRunnable { + Q_OBJECT public: - AvatarImageProvider(LRCInstance* instance = nullptr) - : QuickImageProviderBase(QQuickImageProvider::Image, - QQmlImageProviderBase::ForceAsynchronousImageLoading, - instance) + AsyncAvatarImageResponseRunnable(const QString& id, + const QSize& requestedSize, + LRCInstance* lrcInstance) + : AsyncImageResponseRunnable(id, requestedSize, lrcInstance) {} - QImage requestImage(const QString& id, QSize* size, const QSize& requestedSize) override + void run() override { - Q_UNUSED(size) - - if (requestedSize == QSize(0, 0)) { - qWarning() << Q_FUNC_INFO << "Image request has no dimensions"; - return {}; + // For avatar images, the requested size should be a square. Anything else + // is a request made prior to an aspect ratio guard calculation. + if (requestedSize_ == QSize(0, 0) || requestedSize_.width() != requestedSize_.height()) { + return; } // the first string is the item uri and the second is a uid // that is used for trigger a reload of the underlying image // data and can be discarded at this point - auto idInfo = id.split("_"); + auto idInfo = id_.split("_"); if (idInfo.size() < 2) { qWarning() << Q_FUNC_INFO << "Missing element(s) in the image url"; - return {}; + return; } const auto& imageId = idInfo.at(1); if (!imageId.size()) { qWarning() << Q_FUNC_INFO << "Missing id in the image url"; - return {}; + return; } + QImage image; const auto& type = idInfo.at(0); + if (type == "conversation") { if (imageId == "temp") - return Utils::tempConversationAvatar(requestedSize); - return Utils::conversationAvatar(lrcInstance_, imageId, requestedSize); + image = Utils::tempConversationAvatar(requestedSize_); + else + image = Utils::conversationAvatar(lrcInstance_, imageId, requestedSize_); + } else if (type == "account") { + image = Utils::accountPhoto(lrcInstance_, imageId, requestedSize_); + } else if (type == "contact") { + image = Utils::contactPhoto(lrcInstance_, imageId, requestedSize_); + } else { + qWarning() << Q_FUNC_INFO << "Missing valid prefix in the image url"; + return; } - if (type == "account") - return Utils::accountPhoto(lrcInstance_, imageId, requestedSize); - if (type == "contact") - return Utils::contactPhoto(lrcInstance_, imageId, requestedSize); - qWarning() << Q_FUNC_INFO << "Missing valid prefix in the image url"; - return {}; + Q_EMIT done(image); + } +}; + +class AvatarImageProvider : public AsyncImageProviderBase +{ +public: + AvatarImageProvider(LRCInstance* instance = nullptr) + : AsyncImageProviderBase(instance) + {} + + QQuickImageResponse* requestImageResponse(const QString& id, const QSize& requestedSize) override + { + auto response = new AsyncImageResponse<AsyncAvatarImageResponseRunnable>(id, + requestedSize, + &pool_, + lrcInstance_); + return response; } }; diff --git a/src/app/avatarregistry.cpp b/src/app/avatarregistry.cpp index 4ebbca80a..24d9f7d60 100644 --- a/src/app/avatarregistry.cpp +++ b/src/app/avatarregistry.cpp @@ -33,8 +33,7 @@ AvatarRegistry::AvatarRegistry(LRCInstance* instance, QObject* parent) connect(&lrcInstance_->accountModel(), &AccountModel::profileUpdated, this, - &AvatarRegistry::addOrUpdateImage, - Qt::UniqueConnection); + &AvatarRegistry::addOrUpdateImage); connect(lrcInstance_, &LRCInstance::base64SwarmAvatarChanged, this, [&] { addOrUpdateImage("temp"); diff --git a/src/app/avatarregistry.h b/src/app/avatarregistry.h index b224e20f1..6076896a6 100644 --- a/src/app/avatarregistry.h +++ b/src/app/avatarregistry.h @@ -43,7 +43,7 @@ public: Q_INVOKABLE QString getUid(const QString& id); // add or update a specific image in the cache - QString addOrUpdateImage(const QString& id); + Q_SLOT QString addOrUpdateImage(const QString& id); Q_SIGNALS: void avatarUidChanged(const QString& id); diff --git a/src/app/contactadapter.cpp b/src/app/contactadapter.cpp index f9067e1d3..23dc1fff0 100644 --- a/src/app/contactadapter.cpp +++ b/src/app/contactadapter.cpp @@ -259,7 +259,7 @@ ContactAdapter::connectSignals() &ContactAdapter::bannedStatusChanged, Qt::UniqueConnection); connect(lrcInstance_->getCurrentContactModel(), - &ContactModel::modelUpdated, + &ContactModel::contactAdded, this, &ContactAdapter::onModelUpdated, Qt::UniqueConnection); diff --git a/src/app/conversationlistmodel.cpp b/src/app/conversationlistmodel.cpp index b914f7318..5ee54f9d5 100644 --- a/src/app/conversationlistmodel.cpp +++ b/src/app/conversationlistmodel.cpp @@ -112,7 +112,8 @@ ConversationListProxyModel::filterAcceptsRow(int sourceRow, const QModelIndex& s using namespace ConversationList; if (index.data(Role::Uris).toStringList().isEmpty()) { // TODO: Find out why, and fix in libjami/libjamiclient. - qCritical() << "Filtering 0 member conversation. Fix me"; + qCritical() << "Filtering 0 member conversation. Fix me" + << index.data(Role::UID).toString(); return false; } diff --git a/src/app/conversationlistmodelbase.cpp b/src/app/conversationlistmodelbase.cpp index b8b772740..180b5db44 100644 --- a/src/app/conversationlistmodelbase.cpp +++ b/src/app/conversationlistmodelbase.cpp @@ -19,6 +19,8 @@ #include "conversationlistmodelbase.h" +#include "global.h" + #include <api/contact.h> ConversationListModelBase::ConversationListModelBase(LRCInstance* instance, QObject* parent) @@ -218,9 +220,9 @@ ConversationListModelBase::dataForItem(item_t item, int role) const try { contact = contactModel->getContact(peerUri); } catch (const std::exception&) { - qWarning() << Q_FUNC_INFO << "Can't find contact" << peerUri << " for account " - << lrcInstance_->accountModel().bestNameForAccount(accInfo.id) - << " - Conv: " << item.uid; + C_WARN << "Can't find contact" << peerUri << "for account" + << lrcInstance_->accountModel().bestNameForAccount(accInfo.id) + << "- Conv:" << item.uid; } switch (role) { diff --git a/src/app/lrcinstance.cpp b/src/app/lrcinstance.cpp index 576737395..0f3fa8dd1 100644 --- a/src/app/lrcinstance.cpp +++ b/src/app/lrcinstance.cpp @@ -19,6 +19,8 @@ */ #include "lrcinstance.h" + +#include "global.h" #include "connectivitymonitor.h" #include <QBuffer> diff --git a/src/app/mainapplication.cpp b/src/app/mainapplication.cpp index 200dee065..89d5875ea 100644 --- a/src/app/mainapplication.cpp +++ b/src/app/mainapplication.cpp @@ -63,6 +63,7 @@ messageHandler(QtMsgType type, const QMessageLogContext& context, const QString& const static std::string fmt[5] = {"DBG", "WRN", "CRT", "FTL", "INF"}; const QByteArray localMsg = msg.toUtf8(); const auto ts = QString::number(QDateTime::currentMSecsSinceEpoch()); + const auto tid = QString::number(reinterpret_cast<quintptr>(QThread::currentThreadId()), 16); QString fileLineInfo = ""; const auto isQml = QString(context.category) == QLatin1String("qml"); @@ -84,8 +85,8 @@ messageHandler(QtMsgType type, const QMessageLogContext& context, const QString& } #endif - const auto fmtMsg = QString("[%1][%2]:%3 %4") - .arg(ts, fmt[type].c_str(), fileLineInfo, localMsg.constData()); + const auto fmtMsg = QString("[%1][%2][%3]:%4 %5") + .arg(ts, fmt[type].c_str(), tid, fileLineInfo, localMsg.constData()); (*QT_DEFAULT_MESSAGE_HANDLER)(type, context, fmtMsg); } diff --git a/src/app/mainview/components/AccountComboBox.qml b/src/app/mainview/components/AccountComboBox.qml index c2f7c3b1c..ea149f614 100644 --- a/src/app/mainview/components/AccountComboBox.qml +++ b/src/app/mainview/components/AccountComboBox.qml @@ -111,6 +111,7 @@ Label { Avatar { id: avatar + objectName: "accountComboBoxAvatar" Layout.preferredWidth: JamiTheme.accountListAvatarSize Layout.preferredHeight: JamiTheme.accountListAvatarSize diff --git a/src/app/mainview/components/AccountComboBoxPopup.qml b/src/app/mainview/components/AccountComboBoxPopup.qml index 745ec4915..9bfac5c91 100644 --- a/src/app/mainview/components/AccountComboBoxPopup.qml +++ b/src/app/mainview/components/AccountComboBoxPopup.qml @@ -105,6 +105,7 @@ Popup { Avatar { id: avatar + objectName: "accountComboBoxPopupAvatar" Layout.preferredWidth: JamiTheme.accountListAvatarSize Layout.preferredHeight: JamiTheme.accountListAvatarSize diff --git a/src/app/mainview/components/AccountItemDelegate.qml b/src/app/mainview/components/AccountItemDelegate.qml index 38230d28a..90c49a553 100644 --- a/src/app/mainview/components/AccountItemDelegate.qml +++ b/src/app/mainview/components/AccountItemDelegate.qml @@ -62,6 +62,7 @@ ItemDelegate { spacing: 10 Avatar { + objectName: "accountComboBoxDelegateAvatar" Layout.preferredWidth: JamiTheme.accountListAvatarSize Layout.preferredHeight: JamiTheme.accountListAvatarSize Layout.alignment: Qt.AlignVCenter diff --git a/src/app/mainview/components/SmartListItemDelegate.qml b/src/app/mainview/components/SmartListItemDelegate.qml index a3d801942..299595b88 100644 --- a/src/app/mainview/components/SmartListItemDelegate.qml +++ b/src/app/mainview/components/SmartListItemDelegate.qml @@ -76,6 +76,7 @@ ItemDelegate { ConversationAvatar { id: avatar + objectName: "smartlistItemDelegateAvatar" imageId: UID presenceStatus: Presence diff --git a/src/app/quickimageproviderbase.h b/src/app/quickimageproviderbase.h index 28c3af066..c9d05b28a 100644 --- a/src/app/quickimageproviderbase.h +++ b/src/app/quickimageproviderbase.h @@ -21,9 +21,71 @@ #include <QImage> #include <QObject> #include <QQuickImageProvider> +#include <QThreadPool> class LRCInstance; +class AsyncImageResponseRunnable : public QObject, public QRunnable +{ + Q_OBJECT +public: + AsyncImageResponseRunnable(const QString& id, + const QSize& requestedSize, + LRCInstance* lrcInstance) + : id_(id) + , requestedSize_(requestedSize) + , lrcInstance_(lrcInstance) + {} + + Q_SIGNAL void done(QImage image); + +protected: + QString id_; + QSize requestedSize_; + LRCInstance* lrcInstance_; +}; + +template<typename T_Runnable> +class AsyncImageResponse : public QQuickImageResponse +{ +public: + AsyncImageResponse(const QString& id, + const QSize& requestedSize, + QThreadPool* pool, + LRCInstance* instance) + { + auto runnable = new T_Runnable(id, requestedSize, instance); + connect(runnable, &T_Runnable::done, this, &AsyncImageResponse::handleDone); + pool->start(runnable); + } + + void handleDone(QImage image) + { + image_ = image; + Q_EMIT finished(); + } + + QQuickTextureFactory* textureFactory() const override + { + return QQuickTextureFactory::textureFactoryForImage(image_); + } + + QImage image_; +}; + +class AsyncImageProviderBase : public QQuickAsyncImageProvider +{ +public: + AsyncImageProviderBase(LRCInstance* instance = nullptr) + : QQuickAsyncImageProvider() + , lrcInstance_(instance) + {} + +protected: + LRCInstance* lrcInstance_ {nullptr}; + QThreadPool pool_; +}; + class QuickImageProviderBase : public QQuickImageProvider { public: @@ -35,6 +97,5 @@ public: {} protected: - // LRCInstance pointer LRCInstance* lrcInstance_ {nullptr}; }; diff --git a/src/app/utils.cpp b/src/app/utils.cpp index da9e73f38..b8b95afb4 100644 --- a/src/app/utils.cpp +++ b/src/app/utils.cpp @@ -496,8 +496,7 @@ Utils::conversationAvatar(LRCInstance* instance, QImage Utils::tempConversationAvatar(const QSize& size) { - QString img = QByteArrayFromFile(QStandardPaths::writableLocation(QStandardPaths::CacheLocation) - + "tmpSwarmImage"); + QString img = QByteArrayFromFile(getTempSwarmAvatarPath()); if (img.isEmpty()) return fallbackAvatar(QString(), QString(), size); return scaleAndFrame(imageFromBase64String(img, true), size); @@ -933,3 +932,10 @@ Utils::generateUid() { return QUuid::createUuid().toString(QUuid::Id128); } + +QString +Utils::getTempSwarmAvatarPath() +{ + return QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + QDir::separator() + + "tmpSwarmImage"; +} diff --git a/src/app/utils.h b/src/app/utils.h index e1cee798f..9a51009ba 100644 --- a/src/app/utils.h +++ b/src/app/utils.h @@ -101,6 +101,7 @@ QImage getCirclePhoto(const QImage original, int sizePhoto); QImage halfCrop(const QImage original, bool leftSide); QColor getAvatarColor(const QString& canonicalUri); QImage tempConversationAvatar(const QSize& size); +QString getTempSwarmAvatarPath(); QImage fallbackAvatar(const QString& canonicalUriStr, const QString& letterStr = {}, const QSize& size = defaultAvatarSize); @@ -123,4 +124,5 @@ QString generateUid(); // Misc QString humanFileSize(qint64 fileSize); QString getDebugFilePath(); + } // namespace Utils diff --git a/src/app/utilsadapter.cpp b/src/app/utilsadapter.cpp index 882474ecc..75dee66eb 100644 --- a/src/app/utilsadapter.cpp +++ b/src/app/utilsadapter.cpp @@ -561,8 +561,7 @@ QString UtilsAdapter::tempCreationImage(const QString& imageId) const { if (imageId == "temp") - return Utils::QByteArrayFromFile( - QStandardPaths::writableLocation(QStandardPaths::CacheLocation) + "tmpSwarmImage"); + return Utils::QByteArrayFromFile(Utils::getTempSwarmAvatarPath()); if (lrcInstance_->getCurrentConversationModel()) return lrcInstance_->getCurrentConversationModel()->avatar(imageId); return {}; @@ -597,8 +596,7 @@ UtilsAdapter::setTempCreationImageFromImage(const QImage& image, const QString& } // Save the image if (imageId == "temp") { - QFile file(QStandardPaths::writableLocation(QStandardPaths::CacheLocation) - + "tmpSwarmImage"); + QFile file(Utils::getTempSwarmAvatarPath()); file.open(QIODevice::WriteOnly); file.write(ba.toBase64()); file.close(); diff --git a/src/libclient/accountmodel.cpp b/src/libclient/accountmodel.cpp index 51f7e50a0..fc516ad2c 100644 --- a/src/libclient/accountmodel.cpp +++ b/src/libclient/accountmodel.cpp @@ -54,6 +54,7 @@ namespace lrc { +using namespace authority; using namespace api; class AccountModelPimpl : public QObject @@ -288,7 +289,7 @@ AccountModel::setAlias(const QString& accountId, const QString& alias, bool save accountInfo.profileInfo.alias = alias; if (save) - authority::storage::createOrUpdateProfile(accountInfo.id, accountInfo.profileInfo); + storage::vcard::setProfile(accountInfo.id, accountInfo.profileInfo); Q_EMIT profileUpdated(accountId); } @@ -301,7 +302,7 @@ AccountModel::setAvatar(const QString& accountId, const QString& avatar, bool sa accountInfo.profileInfo.avatar = avatar; if (save) - authority::storage::createOrUpdateProfile(accountInfo.id, accountInfo.profileInfo); + storage::vcard::setProfile(accountInfo.id, accountInfo.profileInfo); Q_EMIT profileUpdated(accountId); } @@ -382,7 +383,7 @@ AccountModelPimpl::AccountModelPimpl(AccountModel& linked, return; } - for (const auto& id : accountIds) { + for (auto& id : accountIds) { addToAccounts(id); } @@ -691,6 +692,8 @@ AccountModelPimpl::slotAccountProfileReceived(const QString& accountId, const QString& displayName, const QString& userPhoto) { + LC_WARN << accountId << displayName; + auto account = accounts.find(accountId); if (account == accounts.end()) return; @@ -698,7 +701,7 @@ AccountModelPimpl::slotAccountProfileReceived(const QString& accountId, accountInfo.profileInfo.avatar = userPhoto; accountInfo.profileInfo.alias = displayName; - authority::storage::createOrUpdateProfile(accountInfo.id, accountInfo.profileInfo); + storage::vcard::setProfile(accountInfo.id, accountInfo.profileInfo); Q_EMIT linked.profileUpdated(accountId); } @@ -716,7 +719,7 @@ AccountModelPimpl::slotNewPosition(const QString& accountId, void AccountModelPimpl::addToAccounts(const QString& accountId) { - auto appPath = authority::storage::getPath(); + auto appPath = storage::getPath(); auto dbName = accountId + "/history"; // Create and load the database. @@ -743,7 +746,7 @@ AccountModelPimpl::addToAccounts(const QString& accountId) // Initialize the profile. account::Info& newAccInfo = (it.first)->second.first; newAccInfo.id = accountId; - newAccInfo.profileInfo.avatar = authority::storage::getAccountAvatar(accountId); + newAccInfo.profileInfo.avatar = storage::getAccountAvatar(accountId); updateAccountDetails(newAccInfo); // Initialize models for this account. @@ -779,7 +782,7 @@ AccountModelPimpl::removeFromAccounts(const QString& accountId) } auto& accountInfo = account->second.first; if (accountInfo.profileInfo.type == profile::Type::SIP) { - auto accountDir = QDir(authority::storage::getPath() + accountId); + auto accountDir = QDir(storage::getPath() + accountId); accountDir.removeRecursively(); } @@ -1107,8 +1110,7 @@ AccountModel::setTopAccount(const QString& accountId) QString AccountModel::accountVCard(const QString& accountId, bool compressImage) const { - return authority::storage::vcard::profileToVcard(getAccountInfo(accountId).profileInfo, - compressImage); + return storage::vcard::profileToVcard(getAccountInfo(accountId).profileInfo, compressImage); } const QString @@ -1202,7 +1204,7 @@ AccountModel::reloadHistory() QString AccountModel::avatar(const QString& accountId) const { - return authority::storage::avatar(accountId); + return storage::avatar(accountId); } } // namespace lrc diff --git a/src/libclient/api/contactmodel.h b/src/libclient/api/contactmodel.h index 1c8244986..1d33f7353 100644 --- a/src/libclient/api/contactmodel.h +++ b/src/libclient/api/contactmodel.h @@ -67,11 +67,6 @@ public: * @param contactInfo */ void addContact(contact::Info contactInfo); - /** - * Add a pending item into the contact list - * @param contactUri - */ - void addToContacts(const QString& contactUri); /** * Ask the daemon to remove a contact. * @param contactUri @@ -131,11 +126,17 @@ public: */ QString avatar(const QString& contactUri) const; + /** + * Get display name from storage + * @param contactUri + */ + QString displayName(const QString& contactUri) const; + Q_SIGNALS: /** - * Connect this signal to know when this model was updated. + * Connect this signal to know when a contact was updated. */ - void modelUpdated(const QString& uri) const; + void contactUpdated(const QString& uri) const; /** * Connect this signal to know when a contact was added. * @param contactUri @@ -158,7 +159,10 @@ Q_SIGNALS: * @param isOutgoing * @param toUri */ - void newCall(const QString& from, const QString& callId, bool isOutgoing, const QString& toUri) const; + void newCall(const QString& from, + const QString& callId, + bool isOutgoing, + const QString& toUri) const; /** * Connect this signal to know when a text message arrives for this account * @param accountId diff --git a/src/libclient/authority/storagehelper.cpp b/src/libclient/authority/storagehelper.cpp index b7a4181e5..8789c457f 100644 --- a/src/libclient/authority/storagehelper.cpp +++ b/src/libclient/authority/storagehelper.cpp @@ -32,14 +32,11 @@ #include <QImage> #include <QByteArray> #include <QBuffer> -#include <QLockFile> #include <QJsonObject> #include <QJsonDocument> #include <fstream> -#if !defined(Q_OS_LINUX) || __GNUC__ > 8 #include <filesystem> -#endif #include <thread> #include <cstring> @@ -137,6 +134,7 @@ prepareUri(const QString& uri, api::profile::Type type) } namespace vcard { + QString compressedAvatar(const QString& image) { @@ -217,33 +215,12 @@ profileToVcard(const api::profile::Info& profileInfo, bool compressImage) void setProfile(const QString& accountId, const api::profile::Info& profileInfo, bool isPeer, bool ov) { - auto vcard = vcard::profileToVcard(profileInfo); - auto path = profileVcardPath(accountId, isPeer ? profileInfo.uri : "", ov); - QLockFile lf(path + ".lock"); - QFile file(path); - QFileInfo fileInfo(path); - auto dir = fileInfo.dir(); - if (!dir.exists()) { -#if !defined(Q_OS_LINUX) || __GNUC__ > 8 - if (!std::filesystem::create_directory(dir.path().toStdString())) { -#endif - qWarning() << "Cannot create " << dir.path(); -#if !defined(Q_OS_LINUX) || __GNUC__ > 8 - } -#endif - } - if (!lf.lock()) { - qWarning().noquote() << "Can't lock file for writing: " << file.fileName(); - return; - } - if (!file.open(QIODevice::WriteOnly)) { - lf.unlock(); - qWarning().noquote() << "Can't open file for writing: " << file.fileName(); - return; - } - QTextStream(&file) << vcard; - file.close(); - lf.unlock(); + withProfile( + accountId, + isPeer ? profileInfo.uri : "", + QIODevice::WriteOnly, + [&](const QByteArray&, QTextStream& stream) { stream << profileToVcard(profileInfo, ov); }, + isPeer ? ov : false); } } // namespace vcard @@ -265,68 +242,65 @@ getPeerParticipantsForConversation(Database& db, const QString& conversationId) } void -createOrUpdateProfile(const QString& accountId, - const api::profile::Info& profileInfo, - bool isPeer, - bool ov) -{ - if (isPeer) { - auto contact = storage::buildContactFromProfile(accountId, - profileInfo.uri, - profileInfo.type); - if (!profileInfo.alias.isEmpty()) - contact.profileInfo.alias = profileInfo.alias; - if (!profileInfo.avatar.isEmpty()) - contact.profileInfo.avatar = profileInfo.avatar; - vcard::setProfile(accountId, contact.profileInfo, isPeer, ov); - return; - } - vcard::setProfile(accountId, profileInfo, isPeer, ov); +createOrUpdateProfile(const QString& accountId, const api::profile::Info& profileInfo, bool ov) +{ + auto contact = storage::buildContactFromProfile(accountId, profileInfo.uri, profileInfo.type); + if (!profileInfo.alias.isEmpty()) + contact.profileInfo.alias = profileInfo.alias; + if (!profileInfo.avatar.isEmpty()) + contact.profileInfo.avatar = profileInfo.avatar; + vcard::setProfile(accountId, contact.profileInfo, true /*isPeer*/, ov); } void removeProfile(const QString& accountId, const QString& peerUri) { auto path = profileVcardPath(accountId, peerUri); - if (!QFile::remove(path)) + if (!QFile::remove(path)) { qWarning() << "Couldn't remove vcard for" << peerUri << "at" << path; - auto opath = profileVcardPath(accountId, peerUri, true); - QFile::remove(opath); + } + auto overridePath = profileVcardPath(accountId, peerUri, true); + QFile::remove(overridePath); } QString getAccountAvatar(const QString& accountId) { - auto accountLocalPath = getPath() + accountId + "/"; - QString filePath; - filePath = accountLocalPath + "profile.vcf"; - QFile file(filePath); - if (!file.open(QIODevice::ReadOnly)) { - return {}; - } - const auto vCard = lrc::vCard::utils::toHashMap(file.readAll()); - for (const auto& key : vCard.keys()) { - if (key.contains("PHOTO")) - return vCard[key]; - } - return {}; + QString avatar; + withProfile( + accountId, + "", + QIODevice::ReadOnly, + [&](const QByteArray& readData, QTextStream&) { + QHash<QByteArray, QByteArray> vCard = lrc::vCard::utils::toHashMap(readData); + for (auto it = vCard.cbegin(); it != vCard.cend(); ++it) + if (it.key().contains("PHOTO")) { + avatar = it.value(); + return; + } + }, + false); + return avatar; } static QPair<QString, QString> getOverridenInfos(const QString& accountId, const QString& peerUri) { - QString b64filePathOverride = profileVcardPath(accountId, peerUri, true); - QFile fileOverride(b64filePathOverride); - - QHash<QByteArray, QByteArray> overridenVCard; QString overridenAlias, overridenAvatar; - if (fileOverride.open(QIODevice::ReadOnly)) { - overridenVCard = lrc::vCard::utils::toHashMap(fileOverride.readAll()); - overridenAlias = overridenVCard[vCard::Property::FORMATTED_NAME]; - for (const auto& key : overridenVCard.keys()) - if (key.contains("PHOTO")) - overridenAvatar = overridenVCard[key]; - } + withProfile( + accountId, + peerUri, + QIODevice::ReadOnly, + [&](const QByteArray& readData, QTextStream&) { + QHash<QByteArray, QByteArray> vCard = lrc::vCard::utils::toHashMap(readData); + overridenAlias = vCard[vCard::Property::FORMATTED_NAME]; + for (auto it = vCard.cbegin(); it != vCard.cend(); ++it) + if (it.key().contains("PHOTO")) { + overridenAvatar = it.value(); + return; + } + }, + true); return {overridenAlias, overridenAvatar}; } @@ -335,58 +309,142 @@ buildContactFromProfile(const QString& accountId, const QString& peerUri, const api::profile::Type& type) { + // Get base contact info lrc::api::profile::Info profileInfo; profileInfo.uri = peerUri; profileInfo.type = type; - auto accountLocalPath = getPath() + accountId + "/"; - QString b64filePath = profileVcardPath(accountId, peerUri); - QFile file(b64filePath); - if (!file.open(QIODevice::ReadOnly)) { - // try non-base64 path - QString filePath = accountLocalPath + "profiles/" + peerUri + ".vcf"; - file.setFileName(filePath); - if (!file.open(QIODevice::ReadOnly)) { - return {profileInfo, "", true, false}; - } - // rename it - qWarning().noquote() << "Renaming profile: " << filePath; - file.rename(b64filePath); - // reopen it - if (!file.open(QIODevice::ReadOnly)) { - qWarning().noquote() << "Can't open file: " << b64filePath; - return {profileInfo, "", true, false}; - } - } + // Try to get overriden infos first auto [overridenAlias, overridenAvatar] = getOverridenInfos(accountId, peerUri); + if (!overridenAlias.isEmpty()) + profileInfo.alias = overridenAlias; + if (!overridenAvatar.isEmpty()) + profileInfo.avatar = overridenAvatar; + + // If either alias or avatar is empty, get from profile + if (profileInfo.alias.isEmpty() || profileInfo.avatar.isEmpty()) { + withProfile( + accountId, + peerUri, + QIODevice::ReadOnly, + [&](const QByteArray& readData, QTextStream&) { + QHash<QByteArray, QByteArray> vCard = lrc::vCard::utils::toHashMap(readData); + if (profileInfo.alias.isEmpty()) + profileInfo.alias = vCard[vCard::Property::FORMATTED_NAME]; + if (profileInfo.avatar.isEmpty()) + for (auto it = vCard.cbegin(); it != vCard.cend(); ++it) + if (it.key().contains("PHOTO")) { + profileInfo.avatar = it.value(); + return; + } + }, + false); + } - const auto vCard = lrc::vCard::utils::toHashMap(file.readAll()); - const auto alias = vCard[vCard::Property::FORMATTED_NAME]; - profileInfo.alias = overridenAlias.isEmpty() ? alias : overridenAlias; return {profileInfo, "", type == api::profile::Type::JAMI, false}; } +bool +withProfile(const QString& accountId, + const QString& peerUri, + QIODevice::OpenMode flags, + ProfileLoadedCb&& callback, + bool ov) +{ + QString path = profileVcardPath(accountId, !peerUri.isEmpty() ? peerUri : "", ov); + + // Ensure the directory exists if we are writing + if (flags & QIODevice::WriteOnly && !QDir().mkpath((QFileInfo(path).absolutePath()))) { + LC_WARN << "Cannot create directory for path:" << path; + return false; + } + + // Add QIODevice::Text to the flags + flags |= QIODevice::Text; + + QFile file(path); + if (!file.open(flags)) { + LC_DBG << "Can't open file: " << path; + return false; + } + + QByteArray readData; + QTextStream outStream(&file); + if (flags & QIODevice::ReadOnly) { + readData = file.readAll(); + } + + // Log what we are doing with the profile for now + LC_DBG << (flags & QIODevice::ReadOnly ? "Reading" : "Writing") << "profile:" << path; + + // Execute the callback with readData and outStream + callback(readData, outStream); + + file.close(); + return true; +} + +QMap<QString, QString> +getProfileData(const QString& accountId, const QString& peerUri) +{ + QMap<QString, QString> profileData; + + // Try to get overriden infos first + auto [overridenAlias, overridenAvatar] = getOverridenInfos(accountId, peerUri); + if (!overridenAlias.isEmpty()) + profileData["alias"] = overridenAlias; + if (!overridenAvatar.isEmpty()) + profileData["avatar"] = overridenAvatar; + + // If either alias or avatar is empty, get from profile + if (profileData["alias"].isEmpty() || profileData["avatar"].isEmpty()) { + withProfile( + accountId, + peerUri, + QIODevice::ReadOnly, + [&](const QByteArray& readData, QTextStream&) { + QHash<QByteArray, QByteArray> vCard = lrc::vCard::utils::toHashMap(readData); + if (profileData["alias"].isEmpty()) + profileData["alias"] = vCard[vCard::Property::FORMATTED_NAME]; + if (profileData["avatar"].isEmpty()) + for (auto it = vCard.cbegin(); it != vCard.cend(); ++it) + if (it.key().contains("PHOTO")) { + profileData["avatar"] = it.value(); + return; + } + }, + false); + } + + return profileData; +} + QString avatar(const QString& accountId, const QString& peerUri) { if (peerUri.isEmpty()) return getAccountAvatar(accountId); - auto [_overridenAlias, overridenAvatar] = getOverridenInfos(accountId, peerUri); + auto [_, overridenAvatar] = getOverridenInfos(accountId, peerUri); if (!overridenAvatar.isEmpty()) return overridenAvatar; - QString b64filePath; - b64filePath = profileVcardPath(accountId, peerUri); - QFile file(b64filePath); - if (!file.open(QIODevice::ReadOnly)) - return {}; - const auto vCard = lrc::vCard::utils::toHashMap(file.readAll()); - for (const auto& key : vCard.keys()) { - if (key.contains("PHOTO")) - return vCard[key]; - } - return {}; + QString avatar; + withProfile( + accountId, + peerUri, + QIODevice::ReadOnly, + [&](const QByteArray& readData, QTextStream&) { + QHash<QByteArray, QByteArray> vCard = lrc::vCard::utils::toHashMap(readData); + for (auto it = vCard.cbegin(); it != vCard.cend(); ++it) + if (it.key().contains("PHOTO")) { + avatar = it.value(); + return; + } + }, + false); + + return avatar; } VectorString @@ -461,41 +519,40 @@ getHistory(Database& db, api::conversation::Info& conversation, const QString& l "conversation=:conversation", {{":conversation", conversation.uid}}); auto nCols = 8; - if (interactionsResult.nbrOfCols == nCols) { - auto payloads = interactionsResult.payloads; - for (decltype(payloads.size()) i = 0; i < payloads.size(); i += nCols) { - QString durationString; - auto extra_data_str = payloads[i + 7]; - if (!extra_data_str.isEmpty()) { - auto jsonData = JSONFromString(extra_data_str); - durationString = readJSONValue(jsonData, "duration"); - } - auto body = payloads[i + 2]; - auto type = api::interaction::to_type(payloads[i + 4]); - std::time_t duration = durationString.isEmpty() - ? 0 - : std::stoi(durationString.toStdString()); - auto status = api::interaction::to_status(payloads[i + 5]); - if (type == api::interaction::Type::CALL) { - body = api::interaction::getCallInteractionStringNonSwarm(payloads[i + 1] - == localUri, - duration); - } else if (type == api::interaction::Type::CONTACT) { - body = storage::getContactInteractionString(payloads[i + 1], status); - } - auto msg = api::interaction::Info({payloads[i + 1], - body, - std::stoi(payloads[i + 3].toStdString()), - duration, - type, - status, - (payloads[i + 6] == "1" ? true : false)}); - conversation.interactions->append(payloads[i], std::move(msg)); - if (status != api::interaction::Status::DISPLAYED || !payloads[i + 1].isEmpty()) { - continue; - } - conversation.interactions->setRead(conversation.participants.front().uri, payloads[i]); + if (interactionsResult.nbrOfCols != nCols) + return; + + auto payloads = interactionsResult.payloads; + for (decltype(payloads.size()) i = 0; i < payloads.size(); i += nCols) { + QString durationString; + auto extra_data_str = payloads[i + 7]; + if (!extra_data_str.isEmpty()) { + auto jsonData = JSONFromString(extra_data_str); + durationString = readJSONValue(jsonData, "duration"); + } + auto body = payloads[i + 2]; + auto type = api::interaction::to_type(payloads[i + 4]); + std::time_t duration = durationString.isEmpty() ? 0 + : std::stoi(durationString.toStdString()); + auto status = api::interaction::to_status(payloads[i + 5]); + if (type == api::interaction::Type::CALL) { + body = api::interaction::getCallInteractionStringNonSwarm(payloads[i + 1] == localUri, + duration); + } else if (type == api::interaction::Type::CONTACT) { + body = storage::getContactInteractionString(payloads[i + 1], status); + } + auto msg = api::interaction::Info({payloads[i + 1], + body, + std::stoi(payloads[i + 3].toStdString()), + duration, + type, + status, + (payloads[i + 6] == "1" ? true : false)}); + conversation.interactions->append(payloads[i], std::move(msg)); + if (status != api::interaction::Status::DISPLAYED || !payloads[i + 1].isEmpty()) { + continue; } + conversation.interactions->setRead(conversation.participants.front().uri, payloads[i]); } } diff --git a/src/libclient/authority/storagehelper.h b/src/libclient/authority/storagehelper.h index b649ae331..0aa40b7b4 100644 --- a/src/libclient/authority/storagehelper.h +++ b/src/libclient/authority/storagehelper.h @@ -40,6 +40,8 @@ namespace authority { namespace storage { +using ProfileLoadedCb = std::function<void(const QByteArray&, QTextStream&)>; + /** * Get the base path for the application's local storage * @return local storage path @@ -105,15 +107,15 @@ VectorString getConversationsWithPeer(Database& db, const QString& participant_u VectorString getPeerParticipantsForConversation(Database& db, const QString& conversationId); /** - * Creates or updates a contact or account vCard file with profile data. + * Creates or updates a contact vCard file with profile data. Will read the profile + * from disk, update the fields and write it back to disk. * @param accountId * @param profileInfo the contact info containing peer profile information - * @param isPeer indicates that a the profileInfo is that of a peer + * @param onCompleted callback to invoke when the profile has been created or updated * @param ov if the client is storing a new vcard */ void createOrUpdateProfile(const QString& accountId, const api::profile::Info& profileInfo, - bool isPeer = false, bool ov = false); /** @@ -131,7 +133,8 @@ void removeProfile(const QString& accountId, const QString& peerUri); QString getAccountAvatar(const QString& accountId); /** - * Build a contact info struct from a vCard + * Build a contact info struct from a local profile which includes + * combining information from the override and regular vCard files. * @param accountId * @param peer_uri * @param type of contact to build @@ -141,6 +144,32 @@ api::contact::Info buildContactFromProfile(const QString& accountId, const QString& peer_uri, const api::profile::Type& type); +/** + * This will return the most pertinent data from the vCard set, including the + * avatar and display name. Both the override and regular vCard files are + * checked and the result is an aggregation of the two. + * @param accountId + * @param peerUri + * @return a map containing the pertinent data (avatar and display name) + */ +QMap<QString, QString> getProfileData(const QString& accountId, const QString& peerUri); + +/** + * @brief withProfileVCard + * @param accountId + * @param peerUri + * @param flag the open mode flag + * @param onLoadedCb callback to invoke when the profile has been loaded + * @param ov if we want to use an override vcard + * @return true if the profile was loaded successfully + */ +bool +withProfile(const QString& accountId, + const QString& peerUri, + QIODevice::OpenMode flags, + ProfileLoadedCb&& onLoadedCb, + bool ov = false); + /** * Get a contact's avatar * @param accountId diff --git a/src/libclient/contactmodel.cpp b/src/libclient/contactmodel.cpp index 0d210492e..e9641655b 100644 --- a/src/libclient/contactmodel.cpp +++ b/src/libclient/contactmodel.cpp @@ -60,8 +60,7 @@ public: Database& db, const CallbacksHandler& callbacksHandler, const BehaviorController& behaviorController); - - ~ContactModelPimpl(); + ~ContactModelPimpl() = default; /** * Fills the contacts based on database's conversations @@ -105,6 +104,17 @@ public: */ QString sipUriReceivedFilter(const QString& uri); + /** + * Update the cached profile info for a contact. + * Warning: this method assumes the caller has locked the contacts mutex. + * @param profileInfo + */ + void updateCachedProfile(profile::Info& profileInfo); + + // Base template function to get a property from the contact + template<typename Func> + QString getCachedProfileProperty(const QString& contactUri, Func extractor); + // Helpers const BehaviorController& behaviorController; const ContactModel& linked; @@ -121,7 +131,8 @@ public: QString searchStatus_ {}; QMap<QString, QString> nonContactLookup_; - QThreadPool profileThreadPool; + // Store if a profile is cached for a given URI. + QSet<QString> cachedProfiles; public Q_SLOTS: /** @@ -298,13 +309,13 @@ ContactModel::addContact(contact::Info contactInfo) return; } - storage::createOrUpdateProfile(owner.id, profile, true); + storage::createOrUpdateProfile(owner.id, profile); { std::lock_guard<std::mutex> lk(pimpl_->contactsMtx_); auto iter = pimpl_->contacts.find(contactInfo.profileInfo.uri); if (iter == pimpl_->contacts.end()) - pimpl_->contacts.insert(iter, contactInfo.profileInfo.uri, contactInfo); + pimpl_->contacts.insert(contactInfo.profileInfo.uri, contactInfo); else { // On non-DBus platform, contactInfo.profileInfo.type may be wrong as the contact // may be trusted already. We must use Profile::Type from pimpl_->contacts @@ -322,21 +333,6 @@ ContactModel::addContact(contact::Info contactInfo) } } -void -ContactModel::addToContacts(const QString& contactUri) -{ - std::lock_guard<std::mutex> lk(pimpl_->contactsMtx_); - auto iter = pimpl_->contacts.find(contactUri); - if (iter != pimpl_->contacts.end()) - return; - - auto contactInfo = storage::buildContactFromProfile(owner.id, - contactUri, - profile::Type::PENDING); - pimpl_->contacts.insert(iter, contactUri, contactInfo); - ConfigurationManager::instance().lookupAddress(owner.id, "", contactUri); -} - void ContactModel::removeContact(const QString& contactUri, bool banned) { @@ -391,21 +387,29 @@ void ContactModel::updateContact(const QString& uri, const MapStringString& infos) { std::unique_lock<std::mutex> lk(pimpl_->contactsMtx_); - auto ci = pimpl_->contacts.find(uri); - if (ci != pimpl_->contacts.end()) { - if (infos.contains("avatar")) { - ci->profileInfo.avatar = storage::vcard::compressedAvatar(infos["avatar"]); - } else { - // Else it will be resetted - ci->profileInfo.avatar = storage::avatar(owner.id, uri); - } - if (infos.contains("title")) - ci->profileInfo.alias = infos["title"]; - storage::createOrUpdateProfile(owner.id, ci->profileInfo, true, true); - lk.unlock(); - Q_EMIT profileUpdated(uri); - Q_EMIT modelUpdated(uri); + auto it = pimpl_->contacts.find(uri); + if (it == pimpl_->contacts.end()) { + return; } + + // Write the updated profile to the in-memory cache + auto& profileInfo = it->profileInfo; + if (infos.contains("avatar")) + profileInfo.avatar = storage::vcard::compressedAvatar(infos["avatar"]); + if (infos.contains("title")) + profileInfo.alias = infos["title"]; + + // Update the profile in the database + storage::vcard::setProfile(owner.id, profileInfo, true /*isPeer*/, true /*ov*/); + + // We can consider the contact profile as cached + pimpl_->cachedProfiles.insert(uri); + + // Update observers + lk.unlock(); + LC_WARN << "ContactModel::updateContact" << uri; + Q_EMIT profileUpdated(uri); + Q_EMIT contactUpdated(uri); } const QList<QString>& @@ -466,7 +470,7 @@ ContactModelPimpl::searchContact(const URI& query) | URI::Section::PORT); if (query.isEmpty()) { // This will remove the temporary item - Q_EMIT linked.modelUpdated(uriId); + Q_EMIT linked.contactUpdated(uriId); updateTemporaryMessage(""); return; } @@ -481,7 +485,7 @@ ContactModelPimpl::searchContact(const URI& query) temporaryContact.profileInfo.uri = uriId; temporaryContact.profileInfo.alias = uriId; temporaryContact.profileInfo.type = profile::Type::TEMPORARY; - Q_EMIT linked.modelUpdated(uriId); + Q_EMIT linked.contactUpdated(uriId); } else { updateTemporaryMessage(tr("Searching…")); @@ -501,7 +505,7 @@ ContactModelPimpl::searchSipContact(const URI& query) | URI::Section::PORT); if (query.isEmpty()) { // This will remove the temporary item - Q_EMIT linked.modelUpdated(uriId); + Q_EMIT linked.contactUpdated(uriId); updateTemporaryMessage(""); return; } @@ -516,7 +520,7 @@ ContactModelPimpl::searchSipContact(const URI& query) temporaryContact.profileInfo.type = profile::Type::TEMPORARY; } } - Q_EMIT linked.modelUpdated(uriId); + Q_EMIT linked.contactUpdated(uriId); } uint64_t @@ -549,7 +553,7 @@ ContactModel::bestNameForContact(const QString& contactUri) const QString res = contactUri; try { auto contact = getContact(contactUri); - auto alias = contact.profileInfo.alias.simplified(); + auto alias = displayName(contactUri).simplified(); if (alias.isEmpty()) { return bestIdFromContactInfo(contact); } @@ -567,18 +571,19 @@ ContactModel::bestNameForContact(const QString& contactUri) const } QString -ContactModel::avatar(const QString& uri) const +ContactModel::avatar(const QString& contactUri) const { - { - std::lock_guard<std::mutex> lk(pimpl_->contactsMtx_); - // For search results it's loaded and not in storage yet. - if (pimpl_->searchResult.contains(uri)) { - auto contact = pimpl_->searchResult.value(uri); - return contact.profileInfo.avatar; - } - } - // Else search in storage (because not cached!) - return storage::avatar(owner.id, uri); + return pimpl_->getCachedProfileProperty(contactUri, [](const profile::Info& profile) { + return profile.avatar; + }); +} + +QString +ContactModel::displayName(const QString& contactUri) const +{ + return pimpl_->getCachedProfileProperty(contactUri, [](const profile::Info& profile) { + return profile.alias; + }); } const QString @@ -613,12 +618,6 @@ ContactModelPimpl::ContactModelPimpl(const ContactModel& linked, , behaviorController(behaviorController) , callbacksHandler(callbacksHandler) { - // Init contacts map - if (linked.owner.profileInfo.type == profile::Type::SIP) - fillWithSIPContacts(); - else - fillWithJamiContacts(); - // connect the signals connect(&callbacksHandler, &CallbacksHandler::newBuddySubscription, @@ -653,44 +652,11 @@ ContactModelPimpl::ContactModelPimpl(const ContactModel& linked, &ConfigurationManagerInterface::userSearchEnded, this, &ContactModelPimpl::slotUserSearchEnded); -} -ContactModelPimpl::~ContactModelPimpl() -{ - disconnect(&callbacksHandler, - &CallbacksHandler::newBuddySubscription, - this, - &ContactModelPimpl::slotNewBuddySubscription); - disconnect(&callbacksHandler, - &CallbacksHandler::contactAdded, - this, - &ContactModelPimpl::slotContactAdded); - disconnect(&callbacksHandler, - &CallbacksHandler::contactRemoved, - this, - &ContactModelPimpl::slotContactRemoved); - disconnect(&callbacksHandler, - &CallbacksHandler::registeredNameFound, - this, - &ContactModelPimpl::slotRegisteredNameFound); - disconnect(&*linked.owner.callModel, &CallModel::newCall, this, &ContactModelPimpl::slotNewCall); - disconnect(&callbacksHandler, - &lrc::CallbacksHandler::newAccountMessage, - this, - &ContactModelPimpl::slotNewAccountMessage); - disconnect(&callbacksHandler, - &CallbacksHandler::transferStatusCreated, - this, - &ContactModelPimpl::slotNewAccountTransfer); - disconnect(&ConfigurationManager::instance(), - &ConfigurationManagerInterface::profileReceived, - this, - &ContactModelPimpl::slotProfileReceived); - disconnect(&ConfigurationManager::instance(), - &ConfigurationManagerInterface::userSearchEnded, - this, - &ContactModelPimpl::slotUserSearchEnded); - profileThreadPool.waitForDone(); + if (this->linked.owner.profileInfo.type == profile::Type::SIP) + fillWithSIPContacts(); + else + fillWithJamiContacts(); } bool @@ -717,18 +683,16 @@ ContactModelPimpl::fillWithSIPContacts() bool ContactModelPimpl::fillWithJamiContacts() { - // Add contacts from daemon + // Add existing contacts from libjami + // Note: explicit type is required here for DBus build const VectorMapStringString& contacts_vector = ConfigurationManager::instance().getContacts( linked.owner.id); - for (auto contact_info : contacts_vector) { - std::lock_guard<std::mutex> lk(contactsMtx_); - bool banned = contact_info["banned"] == "true" ? true : false; + for (auto contact_info : contacts_vector) addToContacts(contact_info["id"], linked.owner.profileInfo.type, "", - banned, + contact_info["banned"] == "true", contact_info["conversationId"]); - } // Add pending contacts const VectorMapStringString& pending_tr { @@ -759,7 +723,7 @@ ContactModelPimpl::fillWithJamiContacts() } // create profile vcard for contact - storage::createOrUpdateProfile(linked.owner.id, contactInfo.profileInfo, true); + storage::vcard::setProfile(linked.owner.id, contactInfo.profileInfo, true); } // Update presence @@ -779,7 +743,7 @@ ContactModelPimpl::fillWithJamiContacts() auto it = contacts.find(uri); if (it != contacts.end()) { it->presence = key == "Online" ? 1 : 0; - linked.modelUpdated(uri); + Q_EMIT linked.contactUpdated(uri); } } break; @@ -794,6 +758,7 @@ ContactModelPimpl::slotNewBuddySubscription(const QString& accountId, const QString& contactUri, int state) { + // LC_WARN << "ContactModelPimpl::slotNewBuddySubscription" << accountId << contactUri << state; if (accountId != linked.owner.id) return; { @@ -804,7 +769,7 @@ ContactModelPimpl::slotNewBuddySubscription(const QString& accountId, } else return; } - Q_EMIT linked.modelUpdated(contactUri); + Q_EMIT linked.contactUpdated(contactUri); } void @@ -829,37 +794,33 @@ ContactModelPimpl::slotContactAdded(const QString& accountId, const QString& con if (isJamsAccount) { auto result = searchResult.find(contactUri); if (result != searchResult.end()) { - storage::createOrUpdateProfile(linked.owner.id, result->profileInfo, true); + storage::createOrUpdateProfile(linked.owner.id, result->profileInfo); } } bool isBanned = false; - { // Always get contactsMtx_ lock before bannedContactsMtx_. std::lock_guard<std::mutex> lk(contactsMtx_); - { // Check whether contact is banned or not std::lock_guard<std::mutex> lk(bannedContactsMtx_); - auto it = std::find(bannedContacts.begin(), bannedContacts.end(), contactUri); + auto it = std::find(bannedContacts.cbegin(), bannedContacts.cend(), contactUri); - isBanned = (it != bannedContacts.end()); + isBanned = (it != bannedContacts.cend()); // If contact is banned, do not re-add it, simply update its flag and the banned contacts list if (isBanned) { bannedContacts.erase(it); } - - MapStringString details = ConfigurationManager::instance() - .getContactDetails(linked.owner.id, contactUri); - addToContacts(contactUri, - linked.owner.profileInfo.type, - "", - false, - details["conversationId"]); } } + + // Note: explicit type is required here for DBus build + MapStringString details = ConfigurationManager::instance().getContactDetails(linked.owner.id, + contactUri); + addToContacts(contactUri, linked.owner.profileInfo.type, "", false, details["conversationId"]); + if (isBanned) { // Update the smartlist linked.owner.conversationModel->refreshFilter(); @@ -901,10 +862,10 @@ ContactModelPimpl::slotContactRemoved(const QString& accountId, if (contact->isBanned) { // Contact was banned, update bannedContacts std::lock_guard<std::mutex> lk(bannedContactsMtx_); - auto it = std::find(bannedContacts.begin(), - bannedContacts.end(), + auto it = std::find(bannedContacts.cbegin(), + bannedContacts.cend(), contact->profileInfo.uri); - if (it == bannedContacts.end()) { + if (it == bannedContacts.cend()) { // should not happen LC_DBG << "Contact is banned but not present in bannedContacts. This is most " "likely the result of an earlier bug."; @@ -936,23 +897,10 @@ ContactModelPimpl::addToContacts(const QString& contactUri, { // create a vcard if necessary profile::Info profileInfo {contactUri, {}, displayName, linked.owner.profileInfo.type}; - auto contactInfo = storage::buildContactFromProfile(linked.owner.id, contactUri, type); - auto updateProfile = false; - if (!profileInfo.alias.isEmpty() && contactInfo.profileInfo.alias != profileInfo.alias) { - updateProfile = true; - contactInfo.profileInfo.alias = profileInfo.alias; - } - auto oldAvatar = storage::avatar(linked.owner.id, contactUri); - if (!profileInfo.avatar.isEmpty() && oldAvatar != profileInfo.avatar) { - updateProfile = true; - contactInfo.profileInfo.avatar = profileInfo.avatar; - } - if (updateProfile) - storage::vcard::setProfile(linked.owner.id, contactInfo.profileInfo, true); + api::contact::Info contactInfo = {profileInfo, "", type == api::profile::Type::JAMI, false}; contactInfo.isBanned = banned; contactInfo.conversationId = conversationId; - contactInfo.profileInfo.avatar.clear(); if (type == profile::Type::JAMI) { ConfigurationManager::instance().lookupAddress(linked.owner.id, "", contactUri); @@ -962,17 +910,21 @@ ContactModelPimpl::addToContacts(const QString& contactUri, } contactInfo.profileInfo.type = type; // Because PENDING should not be stored in the database - auto iter = contacts.find(contactInfo.profileInfo.uri); - if (iter != contacts.end()) { - auto info = iter.value(); - contactInfo.registeredName = info.registeredName; - contactInfo.presence = info.presence; - iter.value() = contactInfo; - } else - contacts.insert(iter, contactInfo.profileInfo.uri, contactInfo); - - if (banned) { - bannedContacts.append(contactUri); + { + std::lock_guard<std::mutex> lk(contactsMtx_); + auto iter = contacts.find(contactInfo.profileInfo.uri); + if (iter != contacts.end()) { + auto info = iter.value(); + contactInfo.registeredName = info.registeredName; + contactInfo.presence = info.presence; + iter.value() = contactInfo; + } else { + contacts.insert(contactInfo.profileInfo.uri, contactInfo); + } + if (banned) { + std::lock_guard<std::mutex> lk(bannedContactsMtx_); + bannedContacts.append(contactUri); + } } } @@ -997,9 +949,9 @@ ContactModelPimpl::slotRegisteredNameFound(const QString& accountId, // we are notified that a previous lookup ended return; } - auto& temporaryContact = searchResult[uri]; + // Update the temporary item lrc::api::profile::Info profileInfo = {uri, "", "", profile::Type::TEMPORARY}; - temporaryContact = {profileInfo, registeredName, false, false}; + searchResult[uri] = {profileInfo, registeredName, false, false}; } } else { { @@ -1027,7 +979,8 @@ ContactModelPimpl::slotRegisteredNameFound(const QString& accountId, return; } updateTemporaryMessage(""); - Q_EMIT linked.modelUpdated(uri); + // TODO: be more granular about this update (add a signal for registeredName)? + Q_EMIT linked.contactUpdated(uri); } void @@ -1038,27 +991,29 @@ ContactModelPimpl::slotNewCall(const QString& fromId, const QString& toUri) { if (!isOutgoing && toUri == linked.owner.profileInfo.uri) { - bool emitContactAdded = false; + bool addContact = false; { std::lock_guard<std::mutex> lk(contactsMtx_); auto it = contacts.find(fromId); if (it == contacts.end()) { // Contact not found, load profile from database. // The conversation model will create an entry and link the incomingCall. - auto type = (linked.owner.profileInfo.type == profile::Type::JAMI) - ? profile::Type::PENDING - : profile::Type::SIP; - addToContacts(fromId, type, displayname, false); - emitContactAdded = true; + addContact = true; } else { // Update the display name if (!displayname.isEmpty()) { it->profileInfo.alias = displayname; - storage::createOrUpdateProfile(linked.owner.id, it->profileInfo, true); + storage::vcard::setProfile(linked.owner.id, it->profileInfo, true); + cachedProfiles.insert(it->profileInfo.uri); } } } - if (emitContactAdded) { + if (addContact) { + auto type = (linked.owner.profileInfo.type == profile::Type::JAMI) + ? profile::Type::PENDING + : profile::Type::SIP; + addToContacts(fromId, type, displayname, false); + if (linked.owner.profileInfo.type == profile::Type::SIP) Q_EMIT linked.contactAdded(fromId); else if (linked.owner.profileInfo.type == profile::Type::JAMI) @@ -1080,30 +1035,31 @@ ContactModelPimpl::slotNewAccountMessage(const QString& accountId, QString peerId2(peerId); - auto emitNewTrust = false; + auto addContact = false; { std::lock_guard<std::mutex> lk(contactsMtx_); if (contacts.find(peerId) == contacts.end()) { // Contact not found, load profile from database. // The conversation model will create an entry and link the incomingCall. + addContact = true; + } + } - if (linked.owner.profileInfo.type == profile::Type::SIP) { - QString potentialContact = sipUriReceivedFilter(peerId); - if (potentialContact.isEmpty()) { - addToContacts(peerId, profile::Type::SIP, "", false); - } else { - // equivalent uri exist, use that uri - peerId2 = potentialContact; - } + if (addContact) { + if (linked.owner.profileInfo.type == profile::Type::SIP) { + QString potentialContact = sipUriReceivedFilter(peerId); + if (potentialContact.isEmpty()) { + addToContacts(peerId, profile::Type::SIP, "", false); } else { - addToContacts(peerId, profile::Type::PENDING, "", false); - emitNewTrust = true; + // equivalent uri exist, use that uri + peerId2 = potentialContact; } + } else { + addToContacts(peerId, profile::Type::PENDING, "", false); + Q_EMIT behaviorController.newTrustRequest(linked.owner.id, "", peerId); } } - if (emitNewTrust) { - Q_EMIT behaviorController.newTrustRequest(linked.owner.id, "", peerId); - } + Q_EMIT linked.newAccountMessage(accountId, peerId2, msgId, payloads); } @@ -1141,10 +1097,11 @@ ContactModelPimpl::sipUriReceivedFilter(const QString& uri) } else { // if not "+" from incoming // sub "+" char from contacts to see if user exit - for (auto& contactUri : contacts.keys()) { + for (auto it = contacts.cbegin(); it != contacts.cend(); ++it) { + const QString& contactUri = it.key(); if (!contactUri.isEmpty()) { for (int j = 2; j <= 4; j++) { - if (QString(contactUri).remove(0, j) == remoteUserQStr) { + if (contactUri.mid(j) == remoteUserQStr) { return contactUri; } } @@ -1161,13 +1118,27 @@ ContactModelPimpl::sipUriReceivedFilter(const QString& uri) return ""; } +void +ContactModelPimpl::updateCachedProfile(profile::Info& profileInfo) +{ + // WARNING: this method assumes the caller has locked the contacts mutex + const auto newProfileInfo = storage::getProfileData(linked.owner.id, profileInfo.uri); + + profileInfo.alias = newProfileInfo["alias"]; + profileInfo.avatar = newProfileInfo["avatar"]; + + // No matter what has been updated here, we want to make sure the contact + // is considered cached now. + cachedProfiles.insert(profileInfo.uri); +} + void ContactModelPimpl::slotNewAccountTransfer(const QString& fileId, datatransfer::Info info) { if (info.accountId != linked.owner.id) return; - bool emitNewTrust = false; + auto addContact = false; { std::lock_guard<std::mutex> lk(contactsMtx_); // Note: just add a contact for compatibility (so not for swarm). @@ -1175,14 +1146,14 @@ ContactModelPimpl::slotNewAccountTransfer(const QString& fileId, datatransfer::I && contacts.find(info.peerUri) == contacts.end()) { // Contact not found, load profile from database. // The conversation model will create an entry and link the incomingCall. - auto type = (linked.owner.profileInfo.type == profile::Type::JAMI) - ? profile::Type::PENDING - : profile::Type::SIP; - addToContacts(info.peerUri, type, "", false); - emitNewTrust = (linked.owner.profileInfo.type == profile::Type::JAMI); + addContact = true; } } - if (emitNewTrust) { + + if (addContact) { + auto type = (linked.owner.profileInfo.type == profile::Type::JAMI) ? profile::Type::PENDING + : profile::Type::SIP; + addToContacts(info.peerUri, type, "", false); Q_EMIT behaviorController.newTrustRequest(linked.owner.id, "", info.peerUri); } @@ -1194,49 +1165,21 @@ ContactModelPimpl::slotProfileReceived(const QString& accountId, const QString& peer, const QString& path) { + Q_UNUSED(path); + if (accountId != linked.owner.id) return; - profileThreadPool.start([=] { - QFile vCardFile(path); - if (!vCardFile.open(QIODevice::ReadOnly | QIODevice::Text)) - return; - QTextStream in(&vCardFile); - - auto vCard = in.readAll(); - - profile::Info profileInfo; - profileInfo.uri = peer; - profileInfo.type = profile::Type::JAMI; - - for (auto& e : QString(vCard).split("\n")) { - if (e.contains("PHOTO")) { - auto splitted = e.split(":"); - if (splitted.size() > 1) - profileInfo.avatar = e.split(":")[1]; - } else if (e.contains("FN")) { - auto splitted = e.split(":"); - if (splitted.size() > 1) - profileInfo.alias = e.split(":")[1]; - } - } - - if (peer == linked.owner.profileInfo.uri) { - if (profileInfo.avatar.isEmpty()) - return; // In this case, probably a new device without avatar. - // Profile is saved by daemon, just update client - linked.owner.accountModel->setAlias(linked.owner.id, profileInfo.alias, false); - linked.owner.accountModel->setAvatar(linked.owner.id, profileInfo.avatar, false); - return; - } - vCardFile.remove(); - - contact::Info contactInfo; - contactInfo.profileInfo = profileInfo; - - linked.owner.contactModel->addContact(contactInfo); - contactInfo.profileInfo.avatar.clear(); // Do not store after update - }); + // Make sure this is for a contact and not the linked account, + // then just remove the URI from the cache list and notify. + std::lock_guard<std::mutex> lk(contactsMtx_); + if (contacts.find(peer) != contacts.end()) { + // Remove the URI from the cache list and notify. + cachedProfiles.remove(peer); + // This signal should be listened to in order to update contact display names + // and avatars in the client. + Q_EMIT linked.profileUpdated(peer); + } } void @@ -1274,7 +1217,35 @@ ContactModelPimpl::slotUserSearchEnded(const QString& accountId, default: break; } - Q_EMIT linked.modelUpdated(query); + Q_EMIT linked.contactUpdated(query); +} + +template<typename Func> +QString +ContactModelPimpl::getCachedProfileProperty(const QString& contactUri, Func extractor) +{ + std::lock_guard<std::mutex> lk(contactsMtx_); + // For search results it's loaded and not in storage yet. + if (searchResult.contains(contactUri)) { + auto contact = searchResult.value(contactUri); + return extractor(contact.profileInfo); + } + + // Try to find the contact. + auto it = contacts.find(contactUri); + if (it == contacts.end()) { + return {}; + } + + // If we have a profile that appears to be recently cached, return the extracted property. + if (cachedProfiles.contains(contactUri)) { + return extractor(it->profileInfo); + } + + // Otherwise, update the profile info and return the extracted property. + updateCachedProfile(it->profileInfo); + + return extractor(it->profileInfo); } } // namespace lrc diff --git a/src/libclient/conversationmodel.cpp b/src/libclient/conversationmodel.cpp index 3f196f6ea..f5c4e54e4 100644 --- a/src/libclient/conversationmodel.cpp +++ b/src/libclient/conversationmodel.cpp @@ -241,9 +241,9 @@ public: public Q_SLOTS: /** - * Listen from contactModel when updated (like new alias, avatar, etc.) + * Listen for a peer update from contactModel (new alias, avatar, etc.) */ - void slotContactModelUpdated(const QString& uri); + void slotContactUpdated(const QString& uri); /** * Listen from contactModel when a new contact is added * @param uri @@ -1671,9 +1671,9 @@ ConversationModelPimpl::ConversationModelPimpl(const ConversationModel& linked, // Contact related connect(&*linked.owner.contactModel, - &ContactModel::modelUpdated, + &ContactModel::contactUpdated, this, - &ConversationModelPimpl::slotContactModelUpdated); + &ConversationModelPimpl::slotContactUpdated); connect(&*linked.owner.contactModel, &ContactModel::contactAdded, this, @@ -1838,9 +1838,9 @@ ConversationModelPimpl::~ConversationModelPimpl() { // Contact related disconnect(&*linked.owner.contactModel, - &ContactModel::modelUpdated, + &ContactModel::contactUpdated, this, - &ConversationModelPimpl::slotContactModelUpdated); + &ConversationModelPimpl::slotContactUpdated); disconnect(&*linked.owner.contactModel, &ContactModel::contactAdded, this, @@ -2867,7 +2867,7 @@ ConversationModelPimpl::slotPendingContactAccepted(const QString& uri) } catch (std::out_of_range& e) { } profile::Info profileInfo {uri, {}, {}, type}; - storage::createOrUpdateProfile(linked.owner.id, profileInfo, true); + storage::vcard::setProfile(linked.owner.id, profileInfo, true); auto convs = storage::getConversationsWithPeer(db, uri); if (!convs.empty()) { try { @@ -2916,9 +2916,9 @@ ConversationModelPimpl::slotContactRemoved(const QString& uri) } void -ConversationModelPimpl::slotContactModelUpdated(const QString& uri) +ConversationModelPimpl::slotContactUpdated(const QString& uri) { - // Update presence for all conversations with this peer + // Update all conversations with this peer for (auto& conversation : conversations) { auto members = peersForConversation(conversation); if (members.indexOf(uri) != -1) { @@ -2928,6 +2928,8 @@ ConversationModelPimpl::slotContactModelUpdated(const QString& uri) } } + // TODO: investigate and possibly refactor the following search list management. + // Might just need comments to clarify the intent. if (currentFilter.isEmpty()) { if (searchResults.empty()) return; @@ -2935,6 +2937,7 @@ ConversationModelPimpl::slotContactModelUpdated(const QString& uri) Q_EMIT linked.searchResultUpdated(); return; } + searchResults.clear(); auto users = linked.owner.contactModel->getSearchResults(); for (auto& user : users) { @@ -3490,28 +3493,27 @@ ConversationModelPimpl::updateInteractionStatus(const QString& accountId, using namespace libjami::Account; auto msgState = static_cast<MessageStates>(status); auto& interactions = conversation.interactions; - interactions->with(messageId, - [&](const QString& id, const interaction::Info& interaction) { - if (interaction.type != interaction::Type::DATA_TRANSFER) { - interaction::Status newState; - if (msgState == MessageStates::SENDING) { - newState = interaction::Status::SENDING; - } else if (msgState == MessageStates::SENT) { - newState = interaction::Status::SUCCESS; - } else if (msgState == MessageStates::DISPLAYED) { - newState = interaction::Status::DISPLAYED; - } else { - return; - } - if (interactions->updateStatus(id, newState) - && newState == interaction::Status::DISPLAYED) { - emitDisplayed = true; - } - } else if (msgState == MessageStates::DISPLAYED) { - emitDisplayed = true; // Status for file transfer is managed otherwise, - // But at least set interaction as read - } - }); + interactions->with(messageId, [&](const QString& id, const interaction::Info& interaction) { + if (interaction.type != interaction::Type::DATA_TRANSFER) { + interaction::Status newState; + if (msgState == MessageStates::SENDING) { + newState = interaction::Status::SENDING; + } else if (msgState == MessageStates::SENT) { + newState = interaction::Status::SUCCESS; + } else if (msgState == MessageStates::DISPLAYED) { + newState = interaction::Status::DISPLAYED; + } else { + return; + } + if (interactions->updateStatus(id, newState) + && newState == interaction::Status::DISPLAYED) { + emitDisplayed = true; + } + } else if (msgState == MessageStates::DISPLAYED) { + emitDisplayed = true; // Status for file transfer is managed otherwise, + // But at least set interaction as read + } + }); if (emitDisplayed) conversation.interactions->setRead(peerUri, messageId); diff --git a/src/libclient/pluginmodel.cpp b/src/libclient/pluginmodel.cpp index 78fc8ed9a..0e6e03769 100644 --- a/src/libclient/pluginmodel.cpp +++ b/src/libclient/pluginmodel.cpp @@ -147,8 +147,9 @@ bool PluginModel::uninstallPlugin(const QString& rootPath) { auto result = PluginManager::instance().uninstallPlugin(rootPath); - for (auto plugin : pluginsPath_.keys(rootPath)) { - pluginsPath_.remove(plugin); + QList<QString> keysToRemove = pluginsPath_.keys(rootPath); + for (const auto& key : keysToRemove) { + pluginsPath_.remove(key); } Q_EMIT modelUpdated(); return result; @@ -163,7 +164,7 @@ PluginModel::getPluginPath(const QString& pluginId) void PluginModel::setPluginsPath() { - for (auto plugin : getInstalledPlugins()) { + for (const auto& plugin : getInstalledPlugins()) { auto details = getPluginDetails(plugin); pluginsPath_[details.id] = details.path; } diff --git a/src/libclient/typedefs.h b/src/libclient/typedefs.h index 7c79f8930..de6796b11 100644 --- a/src/libclient/typedefs.h +++ b/src/libclient/typedefs.h @@ -26,6 +26,8 @@ #include <QtCore/QDebug> #include <QtCore/QDateTime> +#include <QThread> + #include <QtCore/QLoggingCategory> Q_DECLARE_LOGGING_CATEGORY(libclientLog) diff --git a/src/libclient/vcard.cpp b/src/libclient/vcard.cpp index 45f07f66c..f21b862fb 100644 --- a/src/libclient/vcard.cpp +++ b/src/libclient/vcard.cpp @@ -25,25 +25,25 @@ namespace utils { QHash<QByteArray, QByteArray> toHashMap(const QByteArray& content) { - // TODO without Qt QHash<QByteArray, QByteArray> vCard; QByteArray previousKey, previousValue; const QList<QByteArray> lines = content.split('\n'); Q_FOREACH (const QByteArray& property, lines) { // Ignore empty lines - if (property.size()) { - // Some properties are over multiple lines - if (property[0] == ' ' && previousKey.size()) { - previousValue += property.right(property.size() - 1); - } + if (!property.size()) + continue; - // Do not use split, URIs can have : in them - const int dblptPos = property.indexOf(':'); - const QByteArray k(property.left(dblptPos)), - v(property.right(property.size() - dblptPos - 1)); - vCard[k] = v; + // Some properties are over multiple lines + if (property[0] == ' ' && previousKey.size()) { + previousValue += property.right(property.size() - 1); } + + // Do not use split, URIs can have : in them + const int separatorPos = property.indexOf(':'); + const QByteArray key(property.left(separatorPos)); + const QByteArray value(property.right(property.size() - separatorPos - 1)); + vCard[key] = value; } return vCard; } diff --git a/tests/unittests/contact_unittest.cpp b/tests/unittests/contact_unittest.cpp index f182dbf76..fa0307b40 100644 --- a/tests/unittests/contact_unittest.cpp +++ b/tests/unittests/contact_unittest.cpp @@ -64,7 +64,7 @@ TEST_F(ContactFixture, AddSIPContactTest) // ModelUpdated signal spy QSignalSpy modelUpdatedSpy(globalEnv.lrcInstance->getCurrentContactModel(), - &lrc::api::ContactModel::modelUpdated); + &lrc::api::ContactModel::contactUpdated); // Add temp contact test globalEnv.lrcInstance->getCurrentConversationModel()->setFilter("test"); -- GitLab