From 107b6fc4fc91d2cde3fc3d6ac4c532f1ef986e89 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Sun, 10 Apr 2016 18:08:29 +0900 Subject: Store left room members as well. In particular, this will be needed to render room names according to the CS spec. --- room.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index c5b674ae..bf3477e3 100644 --- a/room.cpp +++ b/room.cpp @@ -59,6 +59,7 @@ class Room::Private: public QObject int notificationCount; QList users; QList usersTyping; + QList membersLeft; QHash lastReadEvent; QString prevBatch; bool gettingNewContent; @@ -194,6 +195,11 @@ QList< User* > Room::usersTyping() const return d->usersTyping; } +QList< User* > Room::membersLeft() const +{ + return d->membersLeft; +} + QList< User* > Room::users() const { return d->users; @@ -336,6 +342,8 @@ void Room::processStateEvent(Event* event) and d->users.contains(u) ) { d->users.removeAll(u); + if ( !d->membersLeft.contains(u) ) + d->membersLeft.append(u); emit userRemoved(u); } } -- cgit v1.2.3 From 32b055f56287e97d84a4431a36909027991ef1fc Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 12 Apr 2016 16:03:29 +0900 Subject: Change the way room members are stored to the one recommended by the CS spec. According to section 11.2.2.3 of the CS spec, clients SHOULD follow a certain algorithm of making a non-ambiguous display name of a user in the room context. This algorithm implies checking whether other room members have the same display name. This commit prepares for implementation of the algorithm: 1. Use a hash map instead of a list to store room members. The external Room::users() API is kept intact. 2. Convenience CRUD methods are implemented to deal with the hash map. 3. An additional slot for user renaming is introduced (because renaming affects the hash map). Binding of actual signals is left for the next commit. 4. nullptr is the recommended representation of a null pointer since C++11. Use that and mandate compiler support of that. --- room.cpp | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 67 insertions(+), 11 deletions(-) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index bf3477e3..b770c85c 100644 --- a/room.cpp +++ b/room.cpp @@ -18,6 +18,7 @@ #include "room.h" +#include #include #include @@ -40,6 +41,9 @@ using namespace QMatrixClient; class Room::Private: public QObject { public: + /** Map of user names to users. User names potentially duplicate, hence a multi-hashmap. */ + typedef QMultiHash members_map_t; + Private(Room* parent): q(parent) {} Room* q; @@ -57,12 +61,23 @@ class Room::Private: public QObject JoinState joinState; int highlightCount; int notificationCount; - QList users; + members_map_t membersMap; QList usersTyping; QList membersLeft; QHash lastReadEvent; QString prevBatch; bool gettingNewContent; + + // Convenience methods to work with the membersMap and usersLeft. addMember() + // and removeMember() emit respective Room:: signals after a succesful + // operation. + //void inviteUser(User* u); // We might get it at some point in time. + void addMember(User* u); + bool hasMember(User* u); + // You can't identify a single user by displayname, only by id + User* member(QString id); + void renameMember(User* u, QString oldName); + void removeMember(User* u); }; Room::Room(Connection* connection, QString id) @@ -202,7 +217,53 @@ QList< User* > Room::membersLeft() const QList< User* > Room::users() const { - return d->users; + return d->membersMap.values(); +} + +void Room::Private::addMember(User *u) +{ + if (!membersMap.values(u->name()).contains(u)) + { + membersMap.insert(u->name(), u); + emit q->userAdded(u); + } +} + +bool Room::Private::hasMember(User* u) +{ + return membersMap.values(u->name()).contains(u); +} + +User* Room::Private::member(QString id) +{ + User* u = connection->user(id); + return hasMember(u) ? u : nullptr; +} + +void Room::Private::renameMember(User* u, QString oldName) +{ + // We can't use hasUser because we need to search by oldName + if (membersMap.values(oldName).contains(u)) + { + membersMap.remove(oldName, u); + membersMap.insert(u->name(), u); + } +} + +void Room::Private::removeMember(User* u) +{ + if (hasMember(u)) + { + if ( !membersLeft.contains(u) ) + membersLeft.append(u); + membersMap.remove(u->name(), u); + emit q->userRemoved(u); + } +} + +void Room::memberRenamed(User* user, QString oldName) +{ + d->renameMember(user, oldName); } void Room::addMessage(Event* event) @@ -333,18 +394,13 @@ void Room::processStateEvent(Event* event) RoomMemberEvent* memberEvent = static_cast(event); User* u = d->connection->user(memberEvent->userId()); u->processEvent(event); - if( memberEvent->membership() == MembershipType::Join and !d->users.contains(u) ) + if( memberEvent->membership() == MembershipType::Join ) { - d->users.append(u); - emit userAdded(u); + d->addMember(u); } - else if( memberEvent->membership() == MembershipType::Leave - and d->users.contains(u) ) + else if( memberEvent->membership() == MembershipType::Leave ) { - d->users.removeAll(u); - if ( !d->membersLeft.contains(u) ) - d->membersLeft.append(u); - emit userRemoved(u); + d->removeMember(u); } } } -- cgit v1.2.3 From abfdceda30179c7e7f6fcd72bcb994ee4a9bba5e Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 12 Apr 2016 19:24:49 +0900 Subject: Mark read-only accessors in Room::Private as const. --- room.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index b770c85c..bc8aaf1d 100644 --- a/room.cpp +++ b/room.cpp @@ -73,9 +73,9 @@ class Room::Private: public QObject // operation. //void inviteUser(User* u); // We might get it at some point in time. void addMember(User* u); - bool hasMember(User* u); + bool hasMember(User* u) const; // You can't identify a single user by displayname, only by id - User* member(QString id); + User* member(QString id) const; void renameMember(User* u, QString oldName); void removeMember(User* u); }; @@ -229,12 +229,12 @@ void Room::Private::addMember(User *u) } } -bool Room::Private::hasMember(User* u) +bool Room::Private::hasMember(User* u) const { return membersMap.values(u->name()).contains(u); } -User* Room::Private::member(QString id) +User* Room::Private::member(QString id) const { User* u = connection->user(id); return hasMember(u) ? u : nullptr; -- cgit v1.2.3 From 0a3fab2d361b260aa280a4ac9e8a162977ec3534 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Tue, 12 Apr 2016 19:47:45 +0900 Subject: Introduced Room::roomMemberName(User*) that follows CS spec section 11.2.2.3 --- room.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index bc8aaf1d..9caaf97e 100644 --- a/room.cpp +++ b/room.cpp @@ -20,6 +20,7 @@ #include #include +#include // for efficient string concats (operator%) #include #include "connection.h" @@ -266,6 +267,34 @@ void Room::memberRenamed(User* user, QString oldName) d->renameMember(user, oldName); } +QString Room::roomMembername(User *u) const +{ + // See the CS spec, section 11.2.2.3 + + QString username = u->name(); + 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. + auto namesakes = d->membersMap.values(username); + if (namesakes.size() == 1) + return 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. + if ( !namesakes.contains(u) ) + { + qWarning() + << "Room::roomMemberName(): user" << u->id() + << "is not a member of the room" << id(); + } + + // In case of more than one namesake, disambiguate with user id. + return username % " <" % u->id() % ">"; +} + void Room::addMessage(Event* event) { processMessageEvent(event); -- cgit v1.2.3 From a83eee76db60d3df327a8d8b4ebf6158a2a6f033 Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Wed, 13 Apr 2016 11:12:26 +0900 Subject: Calculate room displayname according to CS spec, with a new signal fired on a displayname update. This changes the way displayname is supplied to a client application - instead of calculating immediately, displayname becomes a separate stored value that is refreshed with every change of the list of members, or the name, or the canonical alias. displaynameChanged signal is supplied to subscribe to these updates: in case of displaying a room in the roomlist a client should use this new signal instead of Room::namesChanged. The displaname calculation algorithm is described in section 11.2.2.5 of the CS spec: https://matrix.org/docs/spec/r0.0.1/client_server.html#calculating-the-display-name-for-a-room --- room.cpp | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 105 insertions(+), 23 deletions(-) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index 9caaf97e..bdc8bf24 100644 --- a/room.cpp +++ b/room.cpp @@ -18,6 +18,8 @@ #include "room.h" +#include + #include #include #include // for efficient string concats (operator%) @@ -51,6 +53,9 @@ class Room::Private: public QObject //static LogMessage* parseMessage(const QJsonObject& message); void gotMessages(KJob* job); + // This updates the room displayname field (which is the way a room should be shown in the room list) + // It should be called whenever the list of members or the room name (m.room.name) or canonical alias change. + void updateDisplayname(); Connection* connection; QList messageEvents; @@ -58,6 +63,7 @@ class Room::Private: public QObject QStringList aliases; QString canonicalAlias; QString name; + QString displayname; QString topic; JoinState joinState; int highlightCount; @@ -79,6 +85,9 @@ class Room::Private: public QObject User* member(QString id) const; void renameMember(User* u, QString oldName); void removeMember(User* u); + + private: + QString roomNameFromMemberNames(const QList& userlist) const; }; Room::Room(Connection* connection, QString id) @@ -126,29 +135,7 @@ QString Room::canonicalAlias() const QString Room::displayName() const { - if (name().isEmpty()) - { - // Without a human name, try to find a substitute - if (!canonicalAlias().isEmpty()) - return canonicalAlias(); - if (!aliases().empty() && !aliases().at(0).isEmpty()) - return aliases().at(0); - // Ok, last attempt - one on one chat - if (users().size() == 2) { - return users().at(0)->displayname() + " and " + - users().at(1)->displayname(); - } - // Fail miserably - return id(); - } - - // If we have a non-empty name, try to stack canonical alias to it. - // The format is unwittingly borrowed from the email address format. - QString dispName = name(); - if (!canonicalAlias().isEmpty()) - dispName += " <" + canonicalAlias() + ">"; - - return dispName; + return d->displayname; } QString Room::topic() const @@ -226,6 +213,7 @@ void Room::Private::addMember(User *u) if (!membersMap.values(u->name()).contains(u)) { membersMap.insert(u->name(), u); + updateDisplayname(); emit q->userAdded(u); } } @@ -248,6 +236,7 @@ void Room::Private::renameMember(User* u, QString oldName) { membersMap.remove(oldName, u); membersMap.insert(u->name(), u); + updateDisplayname(); } } @@ -258,6 +247,7 @@ void Room::Private::removeMember(User* u) if ( !membersLeft.contains(u) ) membersLeft.append(u); membersMap.remove(u->name(), u); + updateDisplayname(); emit q->userRemoved(u); } } @@ -391,6 +381,7 @@ void Room::processStateEvent(Event* event) { d->name = nameEvent->name(); qDebug() << "room name:" << d->name; + d->updateDisplayname(); emit namesChanged(this); } else { @@ -403,6 +394,7 @@ void Room::processStateEvent(Event* event) RoomAliasesEvent* aliasesEvent = static_cast(event); d->aliases = aliasesEvent->aliases(); qDebug() << "room aliases:" << d->aliases; + // No displayname update - aliases are not used to render a displayname emit namesChanged(this); } if( event->type() == EventType::RoomCanonicalAlias ) @@ -410,6 +402,7 @@ void Room::processStateEvent(Event* event) RoomCanonicalAliasEvent* aliasEvent = static_cast(event); d->canonicalAlias = aliasEvent->alias(); qDebug() << "room canonical alias:" << d->canonicalAlias; + d->updateDisplayname(); emit namesChanged(this); } if( event->type() == EventType::RoomTopic ) @@ -460,6 +453,95 @@ void Room::processEphemeralEvent(Event* event) } } +QString Room::Private::roomNameFromMemberNames(const QList &userlist) const +{ + // This is part 3(i,ii,iii) in the room displayname algorithm described + // in the CS spec (see also Room::Private::updateDisplayname() ). + // The spec requires to sort users lexicographically by state_key (user id) + // and use disambiguated display names of two topmost users excluding + // the current one to render the name of the room. + + // std::array is the leanest C++ container + std::array first_two { nullptr, nullptr }; + std::partial_sort_copy( + userlist.begin(), userlist.end(), + first_two.begin(), first_two.end(), + [this](const User* u1, const User* u2) { + // Filter out the "me" user so that it never hits the room name + return u1 != connection->user() && u1->id() < u2->id(); + } + ); + + // i. One-on-one chat. first_two[1] == connection->user() in this case. + if (userlist.size() == 2) + return q->roomMembername(first_two[0]); + + // ii. Two users besides the current one. + if (userlist.size() == 3) + return tr("%1 and %2") + .arg(q->roomMembername(first_two[0])) + .arg(q->roomMembername(first_two[1])); + + // iii. More users. + if (userlist.size() > 3) + return tr("%1 and %L2 others") + .arg(q->roomMembername(first_two[0])) + .arg(userlist.size() - 3); + + // userlist.size() < 2 - apparently, there's only current user in the room + return QString(); +} + +void Room::Private::updateDisplayname() +{ + const QString old_name = displayname; + + // CS spec, section 11.2.2.5 Calculating the display name for a room + // Numbers and i's below refer to respective parts in the spec. + do { + // it's while (false) down below - we'll break out of the sequence once + // the displayname is ready inside 'if' statements + + // 1. Name (from m.room.name) + if (!name.isEmpty()) { + displayname = name; + // The below is a spec extension. This takes care of the case + // when there are two rooms with the same name. + // The format is unwittingly borrowed from the email address format. + if (!canonicalAlias.isEmpty()) + displayname += " <" % canonicalAlias % ">"; + break; + } + + // 2. Canonical alias + if (!canonicalAlias.isEmpty()) { + displayname = canonicalAlias; + break; + } + + // 3. Room members + displayname = roomNameFromMemberNames(membersMap.values()); + if (!displayname.isEmpty()) + break; + + // 4. Users that previously left the room + displayname = tr("Empty room (was: %1)") + .arg(roomNameFromMemberNames(membersLeft)); + if (!displayname.isEmpty()) + break; + + // 5. Fail miserably + displayname = tr("Empty room (%1)").arg(id); + + // Using m.room.aliases is explicitly discouraged by the spec + //if (!aliases.empty() && !aliases.at(0).isEmpty()) + // displayname = aliases.at(0); + } while (false); + + if (old_name != displayname) + emit q->displaynameChanged(q); +} + // void Room::setAlias(QString alias) // { // d->alias = alias; -- cgit v1.2.3 From b36a9f32b1fcdce404e9a0a8cb780ba2ab89c24b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 2 May 2016 12:20:54 +0900 Subject: Reworked room name calculation to get rid of do {} while (false) (as requested at PR review) --- room.cpp | 73 +++++++++++++++++++++++++++++++--------------------------------- 1 file changed, 35 insertions(+), 38 deletions(-) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index bdc8bf24..5b6a3a30 100644 --- a/room.cpp +++ b/room.cpp @@ -87,6 +87,7 @@ class Room::Private: public QObject void removeMember(User* u); private: + QString calculateDisplayname() const; QString roomNameFromMemberNames(const QList& userlist) const; }; @@ -492,52 +493,48 @@ QString Room::Private::roomNameFromMemberNames(const QList &userlist) co return QString(); } -void Room::Private::updateDisplayname() +QString Room::Private::calculateDisplayname() const { - const QString old_name = displayname; - // CS spec, section 11.2.2.5 Calculating the display name for a room - // Numbers and i's below refer to respective parts in the spec. - do { - // it's while (false) down below - we'll break out of the sequence once - // the displayname is ready inside 'if' statements - - // 1. Name (from m.room.name) - if (!name.isEmpty()) { - displayname = name; - // The below is a spec extension. This takes care of the case - // when there are two rooms with the same name. - // The format is unwittingly borrowed from the email address format. - if (!canonicalAlias.isEmpty()) - displayname += " <" % canonicalAlias % ">"; - break; - } + // Numbers below refer to respective parts in the spec. - // 2. Canonical alias - if (!canonicalAlias.isEmpty()) { - displayname = canonicalAlias; - break; - } + // 1. Name (from m.room.name) + if (!name.isEmpty()) { + // The below two lines extend the spec. They take care of the case + // when there are two rooms with the same name. + // The format is unwittingly borrowed from the email address format. + if (!canonicalAlias.isEmpty()) + return name % " <" % canonicalAlias % ">"; + + return name; + } + + // 2. Canonical alias + if (!canonicalAlias.isEmpty()) + return canonicalAlias; - // 3. Room members - displayname = roomNameFromMemberNames(membersMap.values()); - if (!displayname.isEmpty()) - break; + // 3. Room members + QString topMemberNames = roomNameFromMemberNames(membersMap.values()); + if (!topMemberNames.isEmpty()) + return topMemberNames; - // 4. Users that previously left the room - displayname = tr("Empty room (was: %1)") - .arg(roomNameFromMemberNames(membersLeft)); - if (!displayname.isEmpty()) - break; + // 4. Users that previously left the room + topMemberNames = roomNameFromMemberNames(membersLeft); + if (!topMemberNames.isEmpty()) + return tr("Empty room (was: %1)").arg(topMemberNames); - // 5. Fail miserably - displayname = tr("Empty room (%1)").arg(id); + // 5. Fail miserably + return tr("Empty room (%1)").arg(id); - // Using m.room.aliases is explicitly discouraged by the spec - //if (!aliases.empty() && !aliases.at(0).isEmpty()) - // displayname = aliases.at(0); - } while (false); + // Using m.room.aliases is explicitly discouraged by the spec + //if (!aliases.empty() && !aliases.at(0).isEmpty()) + // displayname = aliases.at(0); +} +void Room::Private::updateDisplayname() +{ + const QString old_name = displayname; + displayname = calculateDisplayname(); if (old_name != displayname) emit q->displaynameChanged(q); } -- cgit v1.2.3 From 9a415111fefd0d21f8b4abbbe9af9066d78c971b Mon Sep 17 00:00:00 2001 From: Kitsune Ral Date: Mon, 2 May 2016 22:26:06 +0900 Subject: Actually connect User::nameChanged to Room; make a new Room::memberRenamed signal, which clients should use in the room context. Processing changes of user displaynames is tricky: we have to not only deal with the currently renamed user but also with its past and new namesakes which might change representation due to that renaming. So in the worst case a single User::nameChanged signal may lead to three Room::memberRenamed references (and 3 user displaynames updated in the UI, respectively). And the newly added users should be taken care of in a similar manner, of course. --- room.cpp | 54 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 9 deletions(-) (limited to 'room.cpp') diff --git a/room.cpp b/room.cpp index 5b6a3a30..ac4275af 100644 --- a/room.cpp +++ b/room.cpp @@ -89,6 +89,9 @@ class Room::Private: public QObject private: QString calculateDisplayname() const; QString roomNameFromMemberNames(const QList& userlist) const; + + void insertMemberIntoMap(User* u); + void removeMemberFromMap(QString username, User* u); }; Room::Room(Connection* connection, QString id) @@ -209,12 +212,37 @@ QList< User* > Room::users() const return d->membersMap.values(); } +void Room::Private::insertMemberIntoMap(User *u) +{ + QList namesakes = membersMap.values(u->name()); + membersMap.insert(u->name(), u); + // If there is exactly one namesake of the added user, signal member renaming + // for that other one because the two should be disambiguated now. + if (namesakes.size() == 1) + emit q->memberRenamed(namesakes[0]); + + updateDisplayname(); +} + +void Room::Private::removeMemberFromMap(QString username, User* u) +{ + membersMap.remove(username, u); + // If there was one namesake besides the removed user, signal member renaming + // for it because it doesn't need to be disambiguated anymore. + // TODO: Think about left users. + QList formerNamesakes = membersMap.values(username); + if (formerNamesakes.size() == 1) + emit q->memberRenamed(formerNamesakes[0]); + + updateDisplayname(); +} + void Room::Private::addMember(User *u) { - if (!membersMap.values(u->name()).contains(u)) + if (!hasMember(u)) { - membersMap.insert(u->name(), u); - updateDisplayname(); + insertMemberIntoMap(u); + connect(u, &User::nameChanged, q, &Room::userRenamed); emit q->userAdded(u); } } @@ -232,11 +260,20 @@ User* Room::Private::member(QString id) const void Room::Private::renameMember(User* u, QString oldName) { - // We can't use hasUser because we need to search by oldName + if (hasMember(u)) + { + qWarning() << "Room::Private::renameMember(): the user " + << u->name() + << "is already known in the room under a new name."; + return; + } + if (membersMap.values(oldName).contains(u)) { - membersMap.remove(oldName, u); - membersMap.insert(u->name(), u); + removeMemberFromMap(oldName, u); + insertMemberIntoMap(u); + emit q->memberRenamed(u); + updateDisplayname(); } } @@ -247,13 +284,12 @@ void Room::Private::removeMember(User* u) { if ( !membersLeft.contains(u) ) membersLeft.append(u); - membersMap.remove(u->name(), u); - updateDisplayname(); + removeMemberFromMap(u->name(), u); emit q->userRemoved(u); } } -void Room::memberRenamed(User* user, QString oldName) +void Room::userRenamed(User* user, QString oldName) { d->renameMember(user, oldName); } -- cgit v1.2.3