From bf1dc1484ad5aefd5b86f7f79f23f5d6fc9b940a Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 13 Apr 2018 19:49:55 +0900 Subject: converters.h: Support QVariantMap and QVariantHash --- lib/converters.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/lib/converters.h b/lib/converters.h index bba298e0..22b22f25 100644 --- a/lib/converters.h +++ b/lib/converters.h @@ -21,6 +21,7 @@ #include #include // Includes #include +#include namespace QMatrixClient { @@ -51,6 +52,18 @@ namespace QMatrixClient return QJsonValue(bytes.constData()); } + inline QJsonObject toJson(const QVariantMap& map) + { + return QJsonObject::fromVariantMap(map); + } + +#if (QT_VERSION >= QT_VERSION_CHECK(5, 5, 0)) + inline QJsonObject toJson(const QVariantHash& hMap) + { + return QJsonObject::fromVariantHash(hMap); + } +#endif + template inline QJsonObject toJson(const QHash& hashMap) { @@ -163,6 +176,24 @@ namespace QMatrixClient } }; + template <> struct FromJson + { + inline auto operator()(const QJsonValue& jv) const + { + return jv.toObject().toVariantMap(); + } + }; + +#if (QT_VERSION >= QT_VERSION_CHECK(5, 5, 0)) + template <> struct FromJson + { + inline auto operator()(const QJsonValue& jv) const + { + return jv.toObject().toVariantHash(); + } + }; +#endif + template struct FromJson> { QHash operator()(const QJsonValue& jv) const -- cgit v1.2.3 From 139a6743a4e7a3b2e380d4f3e8f43558bc3164fa Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 13 Apr 2018 20:32:08 +0900 Subject: Support Qt 5.4 That is until ubports move to xenial. --- CMakeLists.txt | 2 +- lib/connection.cpp | 8 ++++---- lib/connection.h | 6 +++++- lib/room.cpp | 8 ++++---- lib/room.h | 6 +++++- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index dae8b287..93cb16fd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -40,7 +40,7 @@ foreach (FLAG all "" pedantic extra error=return-type no-unused-parameter no-gnu endif () endforeach () -find_package(Qt5 5.5.1 REQUIRED Network Gui) +find_package(Qt5 5.4.1 REQUIRED Network Gui) get_filename_component(Qt5_Prefix "${Qt5_DIR}/../../../.." ABSOLUTE) message( STATUS ) diff --git a/lib/connection.cpp b/lib/connection.cpp index 600ab396..241fa43d 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -84,7 +84,7 @@ class Connection::Private QVector roomIdsToForget; QMap userMap; DirectChatsMap directChats; - QHash accountData; + QHash accountData; QString userId; SyncJob* syncJob = nullptr; @@ -336,7 +336,7 @@ void Connection::onSyncSuccess(SyncData &&data) { continue; } d->accountData[accountEvent->jsonType()] = - accountEvent->contentJson().toVariantHash(); + fromJson(accountEvent->contentJson()); emit accountDataChanged(accountEvent->jsonType()); } } @@ -657,7 +657,7 @@ bool Connection::hasAccountData(const QString& type) const return d->accountData.contains(type); } -QVariantHash Connection::accountData(const QString& type) const +Connection::AccountDataMap Connection::accountData(const QString& type) const { return d->accountData.value(type); } @@ -909,7 +909,7 @@ void Connection::saveState(const QUrl &toFile) const for (auto it = d->accountData.begin(); it != d->accountData.end(); ++it) accountDataEvents.append(QJsonObject { {"type", it.key()}, - {"content", QJsonObject::fromVariantHash(it.value())} + {"content", QMatrixClient::toJson(it.value())} }); rootObj.insert("account_data", QJsonObject {{ QStringLiteral("events"), accountDataEvents }}); diff --git a/lib/connection.h b/lib/connection.h index be414931..839371ef 100644 --- a/lib/connection.h +++ b/lib/connection.h @@ -66,6 +66,10 @@ namespace QMatrixClient using DirectChatsMap = QMultiHash; + using AccountDataMap = std::conditional_t< + QT_VERSION >= QT_VERSION_CHECK(5, 5, 0), + QVariantHash, QVariantMap>; + enum RoomVisibility { PublishRoom, UnpublishRoom }; // FIXME: Should go inside CreateRoomJob explicit Connection(QObject* parent = nullptr); @@ -88,7 +92,7 @@ namespace QMatrixClient * stored on the server. Direct chats map cannot be retrieved * using this method _yet_; use directChats() instead. */ - QVariantHash accountData(const QString& type) const; + AccountDataMap accountData(const QString& type) const; /** Get all Invited and Joined rooms grouped by tag * \return a hashmap from tag name to a vector of room pointers, diff --git a/lib/room.cpp b/lib/room.cpp index edbc9266..42d63574 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -109,7 +109,7 @@ class Room::Private QHash lastReadEventIds; QString serverReadMarker; TagsMap tags; - QHash accountData; + QHash accountData; QString prevBatch; QPointer roomMessagesJob; @@ -637,7 +637,7 @@ bool Room::hasAccountData(const QString& type) const return d->accountData.contains(type); } -QVariantHash Room::accountData(const QString& type) const +Room::AccountDataMap Room::accountData(const QString& type) const { return d->accountData.value(type); } @@ -1653,7 +1653,7 @@ void Room::processAccountDataEvent(EventPtr event) } default: d->accountData[event->jsonType()] = - event->contentJson().toVariantHash(); + fromJson(event->contentJson()); emit accountDataChanged(event->jsonType()); } } @@ -1816,7 +1816,7 @@ QJsonObject Room::Private::toJson() const { for (auto it = accountData.begin(); it != accountData.end(); ++it) appendEvent(accountDataEvents, it.key(), - QJsonObject::fromVariantHash(it.value())); + QMatrixClient::toJson(it.value())); } result.insert("account_data", QJsonObject {{ "events", accountDataEvents }}); diff --git a/lib/room.h b/lib/room.h index 39dee8f5..86a7b245 100644 --- a/lib/room.h +++ b/lib/room.h @@ -123,6 +123,10 @@ namespace QMatrixClient using rev_iter_t = Timeline::const_reverse_iterator; using timeline_iter_t = Timeline::const_iterator; + using AccountDataMap = std::conditional_t< + QT_VERSION >= QT_VERSION_CHECK(5, 5, 0), + QVariantHash, QVariantMap>; + Room(Connection* connection, QString id, JoinState initialJoinState); ~Room() override; @@ -269,7 +273,7 @@ namespace QMatrixClient * stored on the server. Tags and read markers cannot be retrieved * using this method _yet_. */ - QVariantHash accountData(const QString& type) const; + AccountDataMap accountData(const QString& type) const; QStringList tagNames() const; TagsMap tags() const; -- cgit v1.2.3 From 03bcadc3e85e68cd870bc3395b1e65794214175a Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 16 Apr 2018 13:40:23 +0900 Subject: ConnectionsGuard<> template to automatically disconnect subscribers Case in point is a room list model (so far in Quaternion, but planned for inclusion to the lib) that stores lists of connections and rooms; upon dropping, e.g., a room from the list the model should disconnect from the room's signals. --- lib/util.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/lib/util.h b/lib/util.h index 92198b0b..7ab1e20d 100644 --- a/lib/util.h +++ b/lib/util.h @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -41,6 +42,7 @@ namespace QMatrixClient } #endif + /** static_cast<> for unique_ptr's */ template inline auto unique_ptr_cast(PtrT2&& p) { @@ -55,5 +57,31 @@ namespace QMatrixClient template static void qAsConst(const T &&) Q_DECL_EQ_DELETE; #endif + + /** A guard pointer that disconnects an interested object upon destruction + * It's almost QPointer<> except that you have to initialise it with one + * more additional parameter - a pointer to a QObject that will be + * disconnected from signals of the underlying pointer upon the guard's + * destruction. + */ + template + class ConnectionsGuard : public QPointer + { + public: + ConnectionsGuard(T* publisher, QObject* subscriber) + : QPointer(publisher), subscriber(subscriber) + { } + ~ConnectionsGuard() + { + if (*this) + (*this)->disconnect(subscriber); + } + ConnectionsGuard(ConnectionsGuard&&) noexcept = default; + ConnectionsGuard& operator=(ConnectionsGuard&&) noexcept = default; + using QPointer::operator=; + + private: + QObject* subscriber; + }; } // namespace QMatrixClient -- cgit v1.2.3 From 3319a57871efe46d607b00b2e5ae5ea563acc98d Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 22 Apr 2018 16:38:26 +0900 Subject: Code reformatting --- lib/room.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/room.cpp b/lib/room.cpp index 42d63574..128a4f48 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -302,7 +302,8 @@ QImage Room::avatar(int dimension) QImage Room::avatar(int width, int height) { if (!d->avatar.url().isEmpty()) - return d->avatar.get(connection(), width, height, [=] { emit avatarChanged(); }); + return d->avatar.get(connection(), width, height, + [=] { emit avatarChanged(); }); // Use the other side's avatar for 1:1's if (d->membersMap.size() == 2) -- cgit v1.2.3 From 698d41f191b9a7dd2c5c1662c5f5bd6b7c2697f6 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 22 Apr 2018 18:36:40 +0900 Subject: Avatar: clear the list of callbacks after completion + general code cleanup --- lib/avatar.cpp | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/lib/avatar.cpp b/lib/avatar.cpp index 1ff2aae1..a40b0ed6 100644 --- a/lib/avatar.cpp +++ b/lib/avatar.cpp @@ -1,3 +1,5 @@ +#include + /****************************************************************************** * Copyright (C) 2017 Kitsune Ral * @@ -26,12 +28,13 @@ #include using namespace QMatrixClient; +using std::move; class Avatar::Private { public: explicit Private(QIcon di, QUrl url = {}) - : _defaultIcon(di), _url(url) + : _defaultIcon(move(di)), _url(move(url)) { } QImage get(Connection* connection, QSize size, get_callback_t callback) const; @@ -51,7 +54,6 @@ class Avatar::Private mutable QPointer _thumbnailRequest = nullptr; mutable QPointer _uploadRequest = nullptr; mutable std::vector callbacks; - mutable get_callback_t uploadCallback; }; Avatar::Avatar(QIcon defaultIcon) @@ -71,13 +73,13 @@ Avatar& Avatar::operator=(Avatar&&) = default; QImage Avatar::get(Connection* connection, int dimension, get_callback_t callback) const { - return d->get(connection, {dimension, dimension}, callback); + return d->get(connection, {dimension, dimension}, move(callback)); } QImage Avatar::get(Connection* connection, int width, int height, get_callback_t callback) const { - return d->get(connection, {width, height}, callback); + return d->get(connection, {width, height}, move(callback)); } bool Avatar::upload(Connection* connection, const QString& fileName, @@ -85,7 +87,7 @@ bool Avatar::upload(Connection* connection, const QString& fileName, { if (isJobRunning(d->_uploadRequest)) return false; - return d->upload(connection->uploadFile(fileName), callback); + return d->upload(connection->uploadFile(fileName), move(callback)); } bool Avatar::upload(Connection* connection, QIODevice* source, @@ -93,7 +95,7 @@ bool Avatar::upload(Connection* connection, QIODevice* source, { if (isJobRunning(d->_uploadRequest) || !source->isReadable()) return false; - return d->upload(connection->uploadContent(source), callback); + return d->upload(connection->uploadContent(source), move(callback)); } QString Avatar::mediaId() const @@ -116,16 +118,18 @@ QImage Avatar::Private::get(Connection* connection, QSize size, _requestedSize = size; if (isJobRunning(_thumbnailRequest)) _thumbnailRequest->abandon(); - callbacks.emplace_back(std::move(callback)); + callbacks.emplace_back(move(callback)); _thumbnailRequest = connection->getThumbnail(_url, size); - QObject::connect( _thumbnailRequest, &MediaThumbnailJob::success, [this] - { - _fetched = true; - _originalImage = _thumbnailRequest->scaledThumbnail(_requestedSize); - _scaledImages.clear(); - for (auto n: callbacks) - n(); - }); + QObject::connect( _thumbnailRequest, &MediaThumbnailJob::success, + _thumbnailRequest, [this] { + _fetched = true; + _originalImage = + _thumbnailRequest->scaledThumbnail(_requestedSize); + _scaledImages.clear(); + for (const auto& n: callbacks) + n(); + callbacks.clear(); + }); } if( _originalImage.isNull() ) @@ -137,7 +141,7 @@ QImage Avatar::Private::get(Connection* connection, QSize size, _defaultIcon.paint(&p, { QPoint(), _defaultIcon.actualSize(size) }); } - for (auto p: _scaledImages) + for (const auto& p: _scaledImages) if (p.first == size) return p.second; auto result = _originalImage.scaled(size, @@ -151,7 +155,7 @@ bool Avatar::Private::upload(UploadContentJob* job, upload_callback_t callback) _uploadRequest = job; if (!isJobRunning(_uploadRequest)) return false; - _uploadRequest->connect(_uploadRequest, &BaseJob::success, + _uploadRequest->connect(_uploadRequest, &BaseJob::success, _uploadRequest, [job,callback] { callback(job->contentUri()); }); return true; } -- cgit v1.2.3 From 5f6ac71684056aaae13335a6b867d43c8cdb1692 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 25 Apr 2018 07:50:02 +0900 Subject: Avatar: don't allow null callbacks to be registered --- lib/avatar.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/avatar.cpp b/lib/avatar.cpp index a40b0ed6..7e6dc50b 100644 --- a/lib/avatar.cpp +++ b/lib/avatar.cpp @@ -106,6 +106,11 @@ QString Avatar::mediaId() const QImage Avatar::Private::get(Connection* connection, QSize size, get_callback_t callback) const { + if (!callback) + { + qCCritical(MAIN) << "Null callbacks are not allowed in Avatar::get"; + Q_ASSERT(false); + } // FIXME: Alternating between longer-width and longer-height requests // is a sure way to trick the below code into constantly getting another // image from the server because the existing one is alleged unsatisfactory. @@ -118,7 +123,8 @@ QImage Avatar::Private::get(Connection* connection, QSize size, _requestedSize = size; if (isJobRunning(_thumbnailRequest)) _thumbnailRequest->abandon(); - callbacks.emplace_back(move(callback)); + if (callback) + callbacks.emplace_back(move(callback)); _thumbnailRequest = connection->getThumbnail(_url, size); QObject::connect( _thumbnailRequest, &MediaThumbnailJob::success, _thumbnailRequest, [this] { -- cgit v1.2.3 From aa49fbcf8b20b7f9e184be41cbe3d0c13a115e6b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 25 Apr 2018 15:58:19 +0900 Subject: BaseJob: rewrite error detection using genuine HTTP codes Qt Network error codes don't represent well some cases. Closes #200. --- lib/jobs/basejob.cpp | 64 +++++++++++++++++++++++++++++----------------------- lib/jobs/basejob.h | 4 ++++ 2 files changed, 40 insertions(+), 28 deletions(-) diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index 4e35cb01..ab07f6ad 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -340,36 +340,44 @@ BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const const auto httpCode = reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); - if (httpCode == 429) // Qt doesn't know about it yet - return { TooManyRequestsError, tr("Too many requests") }; - - // Should we check httpCode instead? Maybe even use it in BaseJob::Status? - // That would make codes in logs slightly more readable. - switch( reply->error() ) + // QNetworkReply error codes seem to be flawed when it comes to HTTP; + // see, e.g., https://github.com/QMatrixClient/libqmatrixclient/issues/200 + // The below processing is based on + // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes + if (httpCode / 100 == 2) // 2xx { - case QNetworkReply::NoError: - if (checkContentType(reply->rawHeader("Content-Type"), - d->expectedContentTypes)) - return NoError; - else // A warning in the logs might be more proper instead - return { IncorrectResponseError, - "Incorrect content type of the response" }; - - case QNetworkReply::AuthenticationRequiredError: - case QNetworkReply::ContentAccessDenied: - case QNetworkReply::ContentOperationNotPermittedError: - return { ContentAccessError, reply->errorString() }; - - case QNetworkReply::ProtocolInvalidOperationError: - case QNetworkReply::UnknownContentError: - return { IncorrectRequestError, reply->errorString() }; - - case QNetworkReply::ContentNotFoundError: - return { NotFoundError, reply->errorString() }; - - default: - return { NetworkError, reply->errorString() }; + if (checkContentType(reply->rawHeader("Content-Type"), + d->expectedContentTypes)) + return NoError; + else // A warning in the logs might be more proper instead + return { UnexpectedResponseTypeWarning, + "Unexpected content type of the response" }; } + + return { [httpCode]() -> StatusCode { + if (httpCode / 10 == 41) + return httpCode == 410 ? IncorrectRequestError : NotFoundError; + switch (httpCode) + { + case 401: case 403: case 407: + return ContentAccessError; + case 404: + return NotFoundError; + case 400: case 405: case 406: case 426: case 428: + case 505: + case 494: // Unofficial nginx "Request header too large" + case 497: // Unofficial nginx "HTTP request sent to HTTPS port" + return IncorrectRequestError; + case 429: + return TooManyRequestsError; + case 501: case 510: + return RequestNotImplementedError; + case 511: + return NetworkAuthRequiredError; + default: + return NetworkError; + } + }(), reply->errorString() }; } BaseJob::Status BaseJob::parseReply(QNetworkReply* reply) diff --git a/lib/jobs/basejob.h b/lib/jobs/basejob.h index f243066f..763ef75a 100644 --- a/lib/jobs/basejob.h +++ b/lib/jobs/basejob.h @@ -53,6 +53,8 @@ namespace QMatrixClient enum StatusCode { NoError = 0 // To be compatible with Qt conventions , Success = 0 , Pending = 1 + , WarningLevel = 20 + , UnexpectedResponseTypeWarning = 21 , Abandoned = 50 //< A very brief period between abandoning and object deletion , ErrorLevel = 100 //< Errors have codes starting from this , NetworkError = 100 @@ -63,6 +65,8 @@ namespace QMatrixClient , IncorrectRequestError , IncorrectResponseError , TooManyRequestsError + , RequestNotImplementedError + , NetworkAuthRequiredError , UserDefinedError = 200 }; -- cgit v1.2.3 From 12dc0570e2467619ef8c8b22fd2fb4a7419c92e4 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 25 Apr 2018 16:07:50 +0900 Subject: BaseJob::doCheckReply: catch non-HTTP errors too --- lib/jobs/basejob.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/jobs/basejob.cpp b/lib/jobs/basejob.cpp index ab07f6ad..56565859 100644 --- a/lib/jobs/basejob.cpp +++ b/lib/jobs/basejob.cpp @@ -337,13 +337,16 @@ bool checkContentType(const QByteArray& type, const QByteArrayList& patterns) BaseJob::Status BaseJob::doCheckReply(QNetworkReply* reply) const { - const auto httpCode = - reply->attribute(QNetworkRequest::HttpStatusCodeAttribute).toInt(); - // QNetworkReply error codes seem to be flawed when it comes to HTTP; // see, e.g., https://github.com/QMatrixClient/libqmatrixclient/issues/200 - // The below processing is based on + // so check genuine HTTP codes. The below processing is based on // https://en.wikipedia.org/wiki/List_of_HTTP_status_codes + const auto httpCodeHeader = + reply->attribute(QNetworkRequest::HttpStatusCodeAttribute); + if (!httpCodeHeader.isValid()) // Woah, we didn't even get HTTP headers + return { NetworkError, reply->errorString() }; + + const auto httpCode = httpCodeHeader.toInt(); if (httpCode / 100 == 2) // 2xx { if (checkContentType(reply->rawHeader("Content-Type"), -- cgit v1.2.3 From 9be5c32d812a55ca55e1cf80d8e29fe593c85a62 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 27 Apr 2018 13:26:28 +0900 Subject: User::processEvent: fix bridge postfix not being stripped Closes #197. --- lib/user.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/user.cpp b/lib/user.cpp index 6143a061..f469128c 100644 --- a/lib/user.cpp +++ b/lib/user.cpp @@ -389,12 +389,12 @@ void User::processEvent(RoomMemberEvent* event, const Room* room) // FIXME: the hint doesn't work for bridged users auto oldNameHint = d->nameForRoom(room, event->prevContent()->displayName); - updateName(event->displayName(), oldNameHint, room); + updateName(newName, oldNameHint, room); updateAvatarUrl(event->avatarUrl(), d->avatarUrlForRoom(room, event->prevContent()->avatarUrl), room); } else { - updateName(event->displayName(), room); + updateName(newName, room); updateAvatarUrl(event->avatarUrl(), d->avatarUrlForRoom(room), room); } } -- cgit v1.2.3 From e74e48507f68e36c289c5dbe4b75f32a6910f3c1 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 27 Apr 2018 13:28:35 +0900 Subject: User::rawName(); bonus, bring order to doc comments This new function allows to get the username along with its bridge (basically, undoing the change applied by processEvent for cases when it is undesirable). --- lib/user.cpp | 6 ++++++ lib/user.h | 27 ++++++++++++++++++--------- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/lib/user.cpp b/lib/user.cpp index f469128c..88d20276 100644 --- a/lib/user.cpp +++ b/lib/user.cpp @@ -220,6 +220,12 @@ QString User::name(const Room* room) const return d->nameForRoom(room); } +QString User::rawName(const Room* room) const +{ + return d->bridged.isEmpty() ? name(room) : + name(room) % " (" % d->bridged % ')'; +} + void User::updateName(const QString& newName, const Room* room) { updateName(newName, d->nameForRoom(room), room); diff --git a/lib/user.h b/lib/user.h index f76f9e0a..f94fbee4 100644 --- a/lib/user.h +++ b/lib/user.h @@ -50,24 +50,33 @@ namespace QMatrixClient /** Get the name chosen by the user * This may be empty if the user didn't choose the name or cleared - * it. - * \sa displayName + * it. If the user is bridged, the bridge postfix (such as '(IRC)') + * is stripped out. No disambiguation for the room is done. + * \sa displayName, rawName */ QString name(const Room* room = nullptr) const; + /** Get the user name along with the bridge postfix + * This function is similar to name() but appends the bridge postfix + * (such as '(IRC)') to the user name. No disambiguation is done. + * \sa name, displayName + */ + QString rawName(const Room* room = nullptr) const; + /** Get the displayed user name - * This method returns the result of name() if its non-empty; - * otherwise it returns user id. This is convenient to show a user - * name outside of a room context. In a room context, user names - * should be disambiguated. - * \sa name, id, fullName Room::roomMembername + * When \p room is null, this method returns result of name() if + * the name is non-empty; otherwise it returns user id. + * When \p room is non-null, this call is equivalent to + * Room::roomMembername invocation for the user (i.e. the user's + * disambiguated room-specific name is returned). + * \sa name, id, fullName, Room::roomMembername */ QString displayname(const Room* room = nullptr) const; /** Get user name and id in one string * The constructed string follows the format 'name (id)' - * used for users disambiguation in a room context and in other - * places. + * which the spec recommends for users disambiguation in + * a room context and in other places. * \sa displayName, Room::roomMembername */ QString fullName(const Room* room = nullptr) const; -- cgit v1.2.3 From 3a763fd470b8aeffa3d412e6f605231492fb5b0c Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 27 Apr 2018 13:34:52 +0900 Subject: Fix broken User::displayName() logic Also, add an assert to Connection::user() to make sure it doesn't create users with invalid ids. Closes #201. --- lib/connection.cpp | 1 + lib/user.cpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/connection.cpp b/lib/connection.cpp index 241fa43d..f2bbf903 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -590,6 +590,7 @@ Room* Connection::invitation(const QString& roomId) const User* Connection::user(const QString& userId) { + Q_ASSERT(userId.startsWith('@') && userId.contains(':')); if( d->userMap.contains(userId) ) return d->userMap.value(userId); auto* user = userFactory(this, userId); diff --git a/lib/user.cpp b/lib/user.cpp index 88d20276..c4fbfe35 100644 --- a/lib/user.cpp +++ b/lib/user.cpp @@ -311,7 +311,7 @@ QString User::displayname(const Room* room) const { auto name = d->nameForRoom(room); return name.isEmpty() ? d->userId : - room ? room->roomMembername(name) : name; + room ? room->roomMembername(this) : name; } QString User::fullName(const Room* room) const -- cgit v1.2.3 From f95a915f2e496631a04e0a4d8d75788e7295aae3 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 27 Apr 2018 13:39:17 +0900 Subject: Cache bridge names along with user names Otherwise the bridge is forgotten after restart. This bumps the cache version. --- lib/connection.cpp | 2 +- lib/room.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/connection.cpp b/lib/connection.cpp index f2bbf903..5f930d57 100644 --- a/lib/connection.cpp +++ b/lib/connection.cpp @@ -848,7 +848,7 @@ void Connection::setHomeserver(const QUrl& url) emit homeserverChanged(homeserver()); } -static constexpr int CACHE_VERSION_MAJOR = 7; +static constexpr int CACHE_VERSION_MAJOR = 8; static constexpr int CACHE_VERSION_MINOR = 0; void Connection::saveState(const QUrl &toFile) const diff --git a/lib/room.cpp b/lib/room.cpp index 128a4f48..c6c1bafd 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -1796,7 +1796,7 @@ QJsonObject Room::Private::toJson() const for (const auto *m : membersMap) appendStateEvent(stateEvents, QStringLiteral("m.room.member"), { { QStringLiteral("membership"), QStringLiteral("join") } - , { QStringLiteral("displayname"), m->name(q) } + , { QStringLiteral("displayname"), m->rawName(q) } , { QStringLiteral("avatar_url"), m->avatarUrl(q).toString() } }, m->id()); -- cgit v1.2.3 From f5af25428212f139c59941bb294a184242c8b5e0 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Fri, 27 Apr 2018 13:40:19 +0900 Subject: Use bridge names as the first line of disambiguation If can get away without showing MXIDs we should do it. --- lib/room.cpp | 40 +++++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/lib/room.cpp b/lib/room.cpp index c6c1bafd..1fa9212e 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -992,26 +992,32 @@ QString Room::roomMembername(const User* u) const if (username.isEmpty()) return u->id(); - // Get the list of users with the same display name. Most likely, - // there'll be one, but there's a chance there are more. - if (d->membersMap.count(username) == 1) - return username; + auto namesakesIt = qAsConst(d->membersMap).find(username); // We expect a user to be a member of the room - but technically it is // possible to invoke roomMemberName() even for non-members. In such case - // we return the name _with_ id, to stay on a safe side. - // XXX: Causes a storm of false alarms when scrolling through older events - // with left users; commented out until we have a proper backtracking of - // room state ("room time machine"). -// if ( !namesakes.contains(u) ) -// { -// qCWarning() -// << "Room::roomMemberName(): user" << u->id() -// << "is not a member of the room" << id(); -// } - - // In case of more than one namesake, use the full name to disambiguate - return u->fullName(this); + // we return the full name, just in case. + if (namesakesIt == d->membersMap.cend()) + return u->fullName(this); + + auto nextUserIt = namesakesIt + 1; + if (nextUserIt == d->membersMap.cend() || nextUserIt.key() != username) + return username; // No disambiguation necessary + + // Check if we can get away just attaching the bridge postfix + // (extension to the spec) + QVector bridges; + for (; namesakesIt != d->membersMap.cend() && namesakesIt.key() == username; + ++namesakesIt) + { + const auto bridgeName = (*namesakesIt)->bridged(); + if (bridges.contains(bridgeName)) // Two accounts on the same bridge + return u->fullName(this); // Disambiguate fully + // Don't bother sorting, not so many bridges out there + bridges.push_back(bridgeName); + } + + return u->rawName(this); // Disambiguate using the bridge postfix only } QString Room::roomMembername(const QString& userId) const -- cgit v1.2.3