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.

5 files changed, 139 insertions, 49 deletions

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<MembershipType>(json["membership"_ls]))
     , isDirect(json["is_direct"_ls].toBool())
-    , displayName(sanitized(json["displayname"_ls].toString()))
-    , avatarUrl(json["avatar_url"_ls].toString())
+    , displayName(fromJson<Omittable<QString>>(json["displayname"_ls]))
+    , avatarUrl(fromJson<Omittable<QUrl>>(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<QString> displayName;
+    Omittable<QUrl> 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<QString> newDisplayName() const { return content().displayName; }
+    Omittable<QUrl> 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<User*> Room::membersLeft() const { return d->membersLeft; }
 
 QList<User*> 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<RoomMemberEvent>(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<RoomMemberEvent>(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<User*> membersLeft() const;
 
     Q_INVOKABLE QList<Quotient::User*> 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<RoomMemberEvent>(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<RoomMemberEvent>(id())->avatarUrl();
+    const auto& url = room->memberAvatarUrl(id());
     const auto& mediaId = url.authority() + url.path();
     return d->otherAvatars.try_emplace(mediaId, url).first->second;
 }