Room: user() and memberJoinState(); code cleanup

Room::user() came instead of Room::Private::member() and memberJoinState() came instead of Private::hasMember.

2 files changed, 100 insertions, 91 deletions

diff --git a/room.cpp b/room.cpp
index 51c5f5cc..3c6fb223 100644
--- a/room.cpp
+++ b/room.cpp
@@ -140,16 +140,10 @@ class Room::Private
 
         const RoomMessageEvent* getEventWithFile(const QString& eventId) const;
 
-        // 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) const;
-        // You can't identify a single user by displayname, only by id
-        User* member(const QString& id) const;
+        void insertMemberIntoMap(User* u);
         void renameMember(User* u, QString oldName);
-        void removeMember(User* u);
+        void removeMemberFromMap(const QString& username, User* u);
 
         void getPreviousContent(int limit = 10);
 
@@ -202,12 +196,9 @@ class Room::Private
         QString calculateDisplayname() const;
         QString roomNameFromMemberNames(const QList<User*>& userlist) const;
 
-        void insertMemberIntoMap(User* u);
-        void removeMemberFromMap(const QString& username, User* u);
-
         bool isLocalUser(const User* u) const
         {
-            return u == connection->user();
+            return u == q->localUser();
         }
 };
 
@@ -301,6 +292,18 @@ QImage Room::avatar(int width, int height)
     return {};
 }
 
+User* Room::user(const QString& userId) const
+{
+    return connection()->user(userId);
+}
+
+JoinState Room::memberJoinState(User* user) const
+{
+    return
+        d->membersMap.contains(user->name(), user) ? JoinState::Join :
+        JoinState::Leave;
+}
+
 JoinState Room::joinState() const
 {
     return d->joinState;
@@ -700,7 +703,7 @@ QStringList Room::memberNames() const
 {
     QStringList res;
     for (auto u : d->membersMap)
-        res.append( this->roomMembername(u) );
+        res.append( roomMembername(u) );
 
     return res;
 }
@@ -717,12 +720,31 @@ int Room::timelineSize() const
 
 void Room::Private::insertMemberIntoMap(User *u)
 {
-    auto namesakes = membersMap.values(u->name());
-    membersMap.insert(u->name(), u);
+    const auto userName = u->name();
+    auto namesakes = membersMap.values(userName);
+    membersMap.insert(userName, 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]);
+        emit q->memberRenamed(namesakes.front());
+}
+
+void Room::Private::renameMember(User* u, QString oldName)
+{
+    if (q->memberJoinState(u) == JoinState::Join)
+    {
+        qCWarning(MAIN) << "Room::Private::renameMember(): the user "
+                        << u->fullName()
+                        << "is already known in the room under a new name.";
+        return;
+    }
+
+    if (membersMap.contains(oldName, u))
+    {
+        removeMemberFromMap(oldName, u);
+        insertMemberIntoMap(u);
+        emit q->memberRenamed(u);
+    }
 }
 
 void Room::Private::removeMemberFromMap(const QString& username, User* u)
@@ -731,9 +753,8 @@ void Room::Private::removeMemberFromMap(const QString& username, User* 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.
-    auto formerNamesakes = membersMap.values(username);
-    if (formerNamesakes.size() == 1)
-        emit q->memberRenamed(formerNamesakes[0]);
+    if (membersMap.count(username) == 1)
+        emit q->memberRenamed(membersMap.value(username));
 }
 
 inline auto makeErrorStr(const Event& e, QByteArray msg)
@@ -772,69 +793,17 @@ Room::Timeline::size_type Room::Private::insertEvents(RoomEventsRange&& events,
     return events.size();
 }
 
-void Room::Private::addMember(User *u)
-{
-    if (!hasMember(u))
-    {
-        insertMemberIntoMap(u);
-        connect(u, &User::nameChanged, q,
-                std::bind(&Private::renameMember, this, u, _2));
-        emit q->userAdded(u);
-    }
-}
-
-bool Room::Private::hasMember(User* u) const
-{
-    return membersMap.values(u->name()).contains(u);
-}
-
-User* Room::Private::member(const QString& id) const
-{
-    auto u = connection->user(id);
-    return hasMember(u) ? u : nullptr;
-}
-
-void Room::Private::renameMember(User* u, QString oldName)
-{
-    if (hasMember(u))
-    {
-        qCWarning(MAIN) << "Room::Private::renameMember(): the user "
-                        << u->name()
-                        << "is already known in the room under a new name.";
-        return;
-    }
-
-    if (membersMap.values(oldName).contains(u))
-    {
-        removeMemberFromMap(oldName, u);
-        insertMemberIntoMap(u);
-        emit q->memberRenamed(u);
-    }
-}
-
-void Room::Private::removeMember(User* u)
-{
-    if (hasMember(u))
-    {
-        if ( !membersLeft.contains(u) )
-            membersLeft.append(u);
-        removeMemberFromMap(u->name(), u);
-        emit q->userRemoved(u);
-    }
-}
-
 QString Room::roomMembername(const User* u) const
 {
     // See the CS spec, section 11.2.2.3
 
-    QString username = u->name();
+    const auto 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)
+    if (d->membersMap.count(username) == 1)
         return username;
 
     // We expect a user to be a member of the room - but technically it is
@@ -856,7 +825,7 @@ QString Room::roomMembername(const User* u) const
 
 QString Room::roomMembername(const QString& userId) const
 {
-    return roomMembername(connection()->user(userId));
+    return roomMembername(user(userId));
 }
 
 void Room::updateData(SyncRoomData&& data)
@@ -1250,7 +1219,7 @@ void Room::Private::checkUnreadMessages(timeline_iter_t from)
     // read receipts from the server (or, for the local user,
     // markMessagesAsRead() invocation) to promote their read markers over
     // the new message events.
-    auto firstWriter = connection->user((*from)->senderId());
+    auto firstWriter = q->user((*from)->senderId());
     if (q->readMarker(firstWriter) != timeline.crend())
     {
         promoteReadMarker(firstWriter, q->findInTimeline((*from)->id()));
@@ -1349,16 +1318,27 @@ void Room::processStateEvents(const RoomEvents& events)
             }
             case EventType::RoomMember: {
                 auto memberEvent = static_cast<RoomMemberEvent*>(event);
-                // Can't use d->member() below because the user may be not a member (yet)
-                auto u = d->connection->user(memberEvent->userId());
-                u->processEvent(event);
+                auto u = user(memberEvent->userId());
+                u->processEvent(memberEvent);
                 if( memberEvent->membership() == MembershipType::Join )
                 {
-                    d->addMember(u);
+                    if (memberJoinState(u) != JoinState::Join)
+                    {
+                        d->insertMemberIntoMap(u);
+                        connect(u, &User::nameChanged, this,
+                                std::bind(&Private::renameMember, d, u, _2));
+                        emit userAdded(u);
+                    }
                 }
                 else if( memberEvent->membership() == MembershipType::Leave )
                 {
-                    d->removeMember(u);
+                    if (memberJoinState(u) == JoinState::Join)
+                    {
+                        if (!d->membersLeft.contains(u))
+                            d->membersLeft.append(u);
+                        d->removeMemberFromMap(u->name(), u);
+                        emit userRemoved(u);
+                    }
                 }
                 break;
             }
@@ -1380,8 +1360,9 @@ void Room::processEphemeralEvent(EventPtr event)
             d->usersTyping.clear();
             for( const QString& userId: typingEvent->users() )
             {
-                if (auto m = d->member(userId))
-                    d->usersTyping.append(m);
+                auto u = user(userId);
+                if (memberJoinState(u) == JoinState::Join)
+                    d->usersTyping.append(u);
             }
             emit typingChanged();
             break;
@@ -1403,8 +1384,11 @@ void Room::processEphemeralEvent(EventPtr event)
                 {
                     const auto newMarker = findInTimeline(p.evtId);
                     for( const Receipt& r: p.receipts )
-                        if (auto m = d->member(r.userId))
-                            d->promoteReadMarker(m, newMarker);
+                    {
+                        auto u = user(r.userId);
+                        if (memberJoinState(u) == JoinState::Join)
+                            d->promoteReadMarker(u, newMarker);
+                    }
                 } else
                 {
                     qCDebug(EPHEMERAL) << "Event" << p.evtId
@@ -1414,9 +1398,12 @@ void Room::processEphemeralEvent(EventPtr event)
                     // a previous marker for a user, keep the previous marker.
                     // Otherwise, blindly store the event id for this user.
                     for( const Receipt& r: p.receipts )
-                        if (auto m = d->member(r.userId))
-                            if (readMarker(m) == timelineEdge())
-                                d->setLastReadEvent(m, p.evtId);
+                    {
+                        auto u = user(r.userId);
+                        if (memberJoinState(u) == JoinState::Join &&
+                                readMarker(u) == timelineEdge())
+                            d->setLastReadEvent(u, p.evtId);
+                    }
                 }
             }
             if (receiptEvent->unreadMessages())
@@ -1541,16 +1528,16 @@ QJsonObject Room::Private::toJson() const
         appendStateEvent(stateEvents, "m.room.canonical_alias", "alias",
                          canonicalAlias);
 
-        for (const auto &i : membersMap)
+        for (const auto *m : membersMap)
         {
             QJsonObject content;
             content.insert("membership", QStringLiteral("join"));
-            content.insert("displayname", i->name());
-            content.insert("avatar_url", i->avatarUrl().toString());
+            content.insert("displayname", m->name());
+            content.insert("avatar_url", m->avatarUrl().toString());
 
             QJsonObject memberEvent;
             memberEvent.insert("type", QStringLiteral("m.room.member"));
-            memberEvent.insert("state_key", i->id());
+            memberEvent.insert("state_key", m->id());
             memberEvent.insert("content", content);
             stateEvents.append(memberEvent);
         }
diff --git a/room.h b/room.h
index a0dad8b3..0ef17abb 100644
--- a/room.h
+++ b/room.h
@@ -161,6 +161,28 @@ namespace QMatrixClient
             Q_INVOKABLE QImage avatar(int width, int height);
 
             /**
+             * @brief Get a user object for a given user id
+             * This is the recommended way to get a user object in a room
+             * context. The actual object type may be changed in further
+             * versions to provide room-specific user information (display name,
+             * avatar etc.).
+             * \note The method will return a valid user regardless of
+             *       the membership.
+             */
+            Q_INVOKABLE User* user(const QString& userId) const;
+
+            /**
+             * \brief Check the join state of a given user in this room
+             *
+             * \note Banned and invited users are not tracked for now (Leave
+             *       will be returned for them).
+             *
+             * \return either of Join, Leave, depending on the given
+             *         user's state in the room
+             */
+            Q_INVOKABLE JoinState memberJoinState(User* user) const;
+
+            /**
              * @brief Produces a disambiguated name for a given user in
              * the context of the room
              */