From f4db6988bf2fd71f74ac851557d82c6f65cc89b1 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 8 Nov 2020 18:57:44 +0100 Subject: More robust member profile data retrieval MemberEventContent: displayname and avatarUrl are now Omittables; CS API doesn't guarantee their presence (see also https://github.com/matrix-org/matrix-doc/issues/1375) but Quotient used to assume they are always there, causing #412. RoomMemberEvent: displayname() -> newDisplayName() and avatarUrl() -> newAvatarUrl(), to emphasise the actual semantics (and also the changed interface). The old signatures still work but are deprecated. Instead of roomMembername() (with weird camel-casing), three new methods in addition to safeMemberName() are introduced to Room: - memberName() - produces the "best known" display name for a given member; User::name() uses it to avoid the pitfall of #412. - disambiguatedMemberName() - this is what roomMembername() used to be; not recommended for direct use when UI is concerned. - safeMemberName() - remains as is, with the fix to the documentation that used to mislead that the function returns HTML-escaped content (it didn't, and doesn't). - htmlSafeMemberName() - does what safeMemberName() claimed to do. Respectively, memberNames() is deprecated in favor of safeMemberNames() and htmlSafeMemberNames(). The corresponding Q_PROPERTY uses safeMemberNames() now. Similar to memberName(), Room has got memberAvatarUrl() to spare User class from diving into Room state to find the member avatar URL. Closes #412. --- lib/events/roommemberevent.cpp | 28 +++++++++----- lib/events/roommemberevent.h | 18 ++++++--- lib/room.cpp | 85 +++++++++++++++++++++++++++++++++--------- lib/room.h | 47 ++++++++++++++++++----- lib/user.cpp | 10 ++--- 5 files changed, 139 insertions(+), 49 deletions(-) (limited to 'lib') diff --git a/lib/events/roommemberevent.cpp b/lib/events/roommemberevent.cpp index 35cbdb3a..f6b29f7f 100644 --- a/lib/events/roommemberevent.cpp +++ b/lib/events/roommemberevent.cpp @@ -50,10 +50,13 @@ using namespace Quotient; MemberEventContent::MemberEventContent(const QJsonObject& json) : membership(fromJson(json["membership"_ls])) , isDirect(json["is_direct"_ls].toBool()) - , displayName(sanitized(json["displayname"_ls].toString())) - , avatarUrl(json["avatar_url"_ls].toString()) + , displayName(fromJson>(json["displayname"_ls])) + , avatarUrl(fromJson>(json["avatar_url"_ls])) , reason(json["reason"_ls].toString()) -{} +{ + if (displayName) + displayName = sanitized(*displayName); +} void MemberEventContent::fillJson(QJsonObject* o) const { @@ -62,9 +65,10 @@ void MemberEventContent::fillJson(QJsonObject* o) const "The key 'membership' must be explicit in MemberEventContent"); if (membership != MembershipType::Undefined) o->insert(QStringLiteral("membership"), membershipStrings[membership]); - o->insert(QStringLiteral("displayname"), displayName); - if (avatarUrl.isValid()) - o->insert(QStringLiteral("avatar_url"), avatarUrl.toString()); + if (displayName) + o->insert(QStringLiteral("displayname"), *displayName); + if (avatarUrl && avatarUrl->isValid()) + o->insert(QStringLiteral("avatar_url"), avatarUrl->toString()); if (!reason.isEmpty()) o->insert(QStringLiteral("reason"), reason); } @@ -111,12 +115,16 @@ bool RoomMemberEvent::isUnban() const bool RoomMemberEvent::isRename() const { - auto prevName = prevContent() ? prevContent()->displayName : QString(); - return displayName() != prevName; + auto prevName = prevContent() && prevContent()->displayName + ? *prevContent()->displayName + : QString(); + return newDisplayName() != prevName; } bool RoomMemberEvent::isAvatarUpdate() const { - auto prevAvatarUrl = prevContent() ? prevContent()->avatarUrl : QUrl(); - return avatarUrl() != prevAvatarUrl; + auto prevAvatarUrl = prevContent() && prevContent()->avatarUrl + ? *prevContent()->avatarUrl + : QUrl(); + return newAvatarUrl() != prevAvatarUrl; } diff --git a/lib/events/roommemberevent.h b/lib/events/roommemberevent.h index 783b8207..35fd69a9 100644 --- a/lib/events/roommemberevent.h +++ b/lib/events/roommemberevent.h @@ -24,7 +24,7 @@ namespace Quotient { class MemberEventContent : public EventContent::Base { public: - enum MembershipType : size_t { + enum MembershipType : unsigned char { Invite = 0, Join, Knock, @@ -38,8 +38,8 @@ public: MembershipType membership; bool isDirect = false; - QString displayName; - QUrl avatarUrl; + Omittable displayName; + Omittable avatarUrl; QString reason; protected: @@ -84,8 +84,16 @@ public: MembershipType membership() const { return content().membership; } QString userId() const { return fullJson()[StateKeyKeyL].toString(); } bool isDirect() const { return content().isDirect; } - QString displayName() const { return content().displayName; } - QUrl avatarUrl() const { return content().avatarUrl; } + Omittable newDisplayName() const { return content().displayName; } + Omittable newAvatarUrl() const { return content().avatarUrl; } + [[deprecated("Use newDisplayName() instead")]] QString displayName() const + { + return newDisplayName().value_or(QString()); + } + [[deprecated("Use newAvatarUrl() instead")]] QUrl avatarUrl() const + { + return newAvatarUrl().value_or(QUrl()); + } QString reason() const { return content().reason; } bool changesMembership() const; bool isBan() const; diff --git a/lib/room.cpp b/lib/room.cpp index f8e6e6ba..b564369c 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -1241,12 +1241,27 @@ QList Room::membersLeft() const { return d->membersLeft; } QList Room::users() const { return d->membersMap.values(); } -QStringList Room::memberNames() const +[[deprecated]] QStringList Room::memberNames() const +{ + return safeMemberNames(); +} + +QStringList Room::safeMemberNames() const +{ + QStringList res; + res.reserve(d->membersMap.size()); + for (auto u: std::as_const(d->membersMap)) + res.append(safeMemberName(u->id())); + + return res; +} + +QStringList Room::htmlSafeMemberNames() const { QStringList res; res.reserve(d->membersMap.size()); - for (auto u : qAsConst(d->membersMap)) - res.append(roomMembername(u)); + for (auto u: std::as_const(d->membersMap)) + res.append(htmlSafeMemberName(u->id())); return res; } @@ -1411,37 +1426,71 @@ Room::Private::moveEventsToTimeline(RoomEventsRange events, return insertedSize; } +QString Room::memberName(const QString& mxId) const +{ + // See https://github.com/matrix-org/matrix-doc/issues/1375 + const auto rme = getCurrentState(mxId); + return rme->newDisplayName() ? *rme->newDisplayName() + : rme->prevContent() ? rme->prevContent()->displayName.value_or(QString()) + : QString(); +} + QString Room::roomMembername(const User* u) const +{ + Q_ASSERT(u != nullptr); + return disambiguatedMemberName(u->id()); +} + +QString Room::roomMembername(const QString& userId) const +{ + return disambiguatedMemberName(userId); +} + +inline QString makeFullUserName(const QString& displayName, const QString& mxId) +{ + return displayName % " (" % mxId % ')'; +} + +QString Room::disambiguatedMemberName(const QString& mxId) const { // See the CS spec, section 11.2.2.3 - const auto username = u->name(this); + const auto username = memberName(mxId); if (username.isEmpty()) - return u->id(); + return mxId; 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 + // possible to invoke this function even for non-members. In such case // we return the full name, just in case. if (namesakesIt == d->membersMap.cend()) - return u->fullName(this); + return makeFullUserName(username, mxId); auto nextUserIt = namesakesIt; if (++nextUserIt == d->membersMap.cend() || nextUserIt.key() != username) return username; // No disambiguation necessary - return u->fullName(this); // Disambiguate fully + return makeFullUserName(username, mxId); // Disambiguate fully } -QString Room::roomMembername(const QString& userId) const +QString Room::safeMemberName(const QString& userId) const { - return roomMembername(user(userId)); + return sanitized(roomMembername(userId)); } -QString Room::safeMemberName(const QString& userId) const +QString Room::htmlSafeMemberName(const QString& userId) const { - return sanitized(roomMembername(userId)); + return safeMemberName(userId).toHtmlEscaped(); +} + +QUrl Room::memberAvatarUrl(const QString &mxId) const +{ + // See https://github.com/matrix-org/matrix-doc/issues/1375 + const auto rme = getCurrentState(mxId); + return rme->newAvatarUrl() ? *rme->newAvatarUrl() + : rme->prevContent() ? rme->prevContent()->avatarUrl.value_or(QUrl()) + : QUrl(); } void Room::updateData(SyncRoomData&& data, bool fromCache) @@ -2422,8 +2471,8 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) case MembershipType::Join: switch (rme.membership()) { case MembershipType::Join: // rename/avatar change or no-op - if (rme.displayName() != oldRme->displayName()) { - emit memberAboutToRename(u, rme.displayName()); + if (rme.newDisplayName()) { + emit memberAboutToRename(u, *rme.newDisplayName()); d->removeMemberFromMap(u->name(this), u); } break; @@ -2517,11 +2566,11 @@ Room::Changes Room::processStateEvent(const RoomEvent& e) d->insertMemberIntoMap(u); emit userAdded(u); } else { - if (oldMemberEvent->displayName() != evt.displayName()) { + if (evt.newDisplayName()) { d->insertMemberIntoMap(u); emit memberRenamed(u); } - if (oldMemberEvent->avatarUrl() != evt.avatarUrl()) + if (evt.newAvatarUrl()) emit memberAvatarChanged(u); } break; @@ -2858,12 +2907,12 @@ MemberSorter Room::memberSorter() const { return MemberSorter(this); } bool MemberSorter::operator()(User* u1, User* u2) const { - return operator()(u1, room->roomMembername(u2)); + return operator()(u1, room->disambiguatedMemberName(u2->id())); } bool MemberSorter::operator()(User* u1, const QString& u2name) const { - auto n1 = room->roomMembername(u1); + auto n1 = room->disambiguatedMemberName(u1->id()); if (n1.startsWith('@')) n1.remove(0, 1); auto n2 = u2name.midRef(u2name.startsWith('@') ? 1 : 0); diff --git a/lib/room.h b/lib/room.h index 4be3ed2b..d7b5c516 100644 --- a/lib/room.h +++ b/lib/room.h @@ -101,7 +101,7 @@ class Room : public QObject { Q_PROPERTY(bool usesEncryption READ usesEncryption NOTIFY encryption) Q_PROPERTY(int timelineSize READ timelineSize NOTIFY addedMessages) - Q_PROPERTY(QStringList memberNames READ memberNames NOTIFY memberListChanged) + Q_PROPERTY(QStringList memberNames READ safeMemberNames NOTIFY memberListChanged) Q_PROPERTY(int memberCount READ memberCount NOTIFY memberListChanged) Q_PROPERTY(int joinedCount READ joinedCount NOTIFY memberListChanged) Q_PROPERTY(int invitedCount READ invitedCount NOTIFY memberListChanged) @@ -209,7 +209,10 @@ public: QList membersLeft() const; Q_INVOKABLE QList users() const; + [[deprecated("Use safeMemberNames() or htmlSafeMemberNames() instead")]] QStringList memberNames() const; + QStringList safeMemberNames() const; + QStringList htmlSafeMemberNames() const; [[deprecated("Use joinedCount(), invitedCount(), totalMemberCount()")]] int memberCount() const; int timelineSize() const; @@ -251,31 +254,55 @@ public: /** * \brief Check the join state of a given user in this room * - * \note Banned and invited users are not tracked for now (Leave + * \note Banned and invited users are not tracked separately for now (Leave * will be returned for them). * * \return Join if the user is a room member; Leave otherwise */ Q_INVOKABLE Quotient::JoinState memberJoinState(Quotient::User* user) const; - /** - * Get a disambiguated name for a given user in - * the context of the room + //! \brief Get a display name (without disambiguation) for the given member + //! + //! \sa safeMemberName, htmlSafeMemberName + Q_INVOKABLE QString memberName(const QString& mxId) const; + + /*! + * \brief Get a disambiguated name for the given user in the room context + * + * \deprecated use safeMemberName() instead */ Q_INVOKABLE QString roomMembername(const Quotient::User* u) const; - /** - * Get a disambiguated name for a user with this id in - * the context of the room + /*! + * \brief Get a disambiguated name for a user with this id in the room context + * + * \deprecated use safeMemberName() instead */ Q_INVOKABLE QString roomMembername(const QString& userId) const; - /** Get a display-safe member name in the context of this room + /*! + * \brief Get a disambiguated name for the member with the given MXID * - * Display-safe means HTML-safe + without RLO/LRO markers + * This function should only be used for non-UI code; consider using + * safeMemberName() or htmlSafeMemberName() for displayed strings. + */ + Q_INVOKABLE QString disambiguatedMemberName(const QString& mxId) const; + + /*! Get a display-safe member name in the context of this room + * + * Display-safe means disambiguated and without RLO/LRO markers * (see https://github.com/quotient-im/Quaternion/issues/545). */ Q_INVOKABLE QString safeMemberName(const QString& userId) const; + /*! Get an HTML-safe member name in the context of this room + * + * This function adds HTML escaping on top of safeMemberName() safeguards. + */ + Q_INVOKABLE QString htmlSafeMemberName(const QString& userId) const; + + //! \brief Get an avatar for the member with the given MXID + QUrl memberAvatarUrl(const QString& mxId) const; + const Timeline& messageEvents() const; const PendingEvents& pendingEvents() const; diff --git a/lib/user.cpp b/lib/user.cpp index ffa4efb9..45a9c121 100644 --- a/lib/user.cpp +++ b/lib/user.cpp @@ -88,8 +88,7 @@ int User::hue() const { return int(hueF() * 359); } QString User::name(const Room* room) const { - return room ? room->getCurrentState(id())->displayName() - : d->defaultName; + return room ? room->memberName(id()) : d->defaultName; } QString User::rawName(const Room* room) const { return name(room); } @@ -169,9 +168,8 @@ bool User::isIgnored() const { return connection()->isIgnored(this); } QString User::displayname(const Room* room) const { - return room ? room->roomMembername(this) - : d->defaultName.isEmpty() ? d->id - : d->defaultName; + return room ? room->safeMemberName(id()) + : d->defaultName.isEmpty() ? d->id : d->defaultName; } QString User::fullName(const Room* room) const @@ -187,7 +185,7 @@ const Avatar& User::avatarObject(const Room* room) const if (!room) return d->defaultAvatar; - const auto& url = room->getCurrentState(id())->avatarUrl(); + const auto& url = room->memberAvatarUrl(id()); const auto& mediaId = url.authority() + url.path(); return d->otherAvatars.try_emplace(mediaId, url).first->second; } -- cgit v1.2.3