User/Room: signal that a user/member is about to change the name

Enables to address QMatrixClient/Quaternion#284. Also fixes a gibberish condition in Room::Private::renameMember() that led to improper warnings and a too early return.

4 files changed, 80 insertions, 46 deletions

diff --git a/room.cpp b/room.cpp
index 97f046d3..06041090 100644
--- a/room.cpp
+++ b/room.cpp
@@ -142,7 +142,7 @@ class Room::Private
 
         //void inviteUser(User* u); // We might get it at some point in time.
         void insertMemberIntoMap(User* u);
-        void renameMember(User* u, QString oldName, const Room* context);
+        void renameMember(User* u, QString oldName);
         void removeMemberFromMap(const QString& username, User* u);
 
         void getPreviousContent(int limit = 10);
@@ -721,42 +721,49 @@ int Room::timelineSize() const
 void Room::Private::insertMemberIntoMap(User *u)
 {
     const auto userName = u->name(q);
-    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.
+    auto namesakes = membersMap.values(userName);
+    if (namesakes.size() == 1)
+        emit q->memberAboutToRename(namesakes.front(),
+                                    namesakes.front()->fullName(q));
+    membersMap.insert(userName, u);
     if (namesakes.size() == 1)
         emit q->memberRenamed(namesakes.front());
 }
 
-void Room::Private::renameMember(User* u, QString oldName, const Room* context)
+void Room::Private::renameMember(User* u, QString oldName)
 {
-    if (context != q)
-        return; // It's not a rename for this room
-    if (q->memberJoinState(u) == JoinState::Join)
+    if (u->name(q) == oldName)
     {
         qCWarning(MAIN) << "Room::Private::renameMember(): the user "
                         << u->fullName(q)
                         << "is already known in the room under a new name.";
-        return;
     }
-
-    if (membersMap.contains(oldName, u))
+    else if (membersMap.contains(oldName, u))
     {
         removeMemberFromMap(oldName, u);
         insertMemberIntoMap(u);
-        emit q->memberRenamed(u);
     }
+    emit q->memberRenamed(u);
 }
 
 void Room::Private::removeMemberFromMap(const QString& username, User* u)
 {
+    User* namesake = nullptr;
+    auto namesakes = membersMap.values(username);
+    if (namesakes.size() == 2)
+    {
+        namesake = namesakes.front() == u ? namesakes.back() : namesakes.front();
+        Q_ASSERT_X(namesake != u, __FUNCTION__, "Room members list is broken");
+        emit q->memberAboutToRename(namesake, username);
+    }
     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.
-    if (membersMap.count(username) == 1)
-        emit q->memberRenamed(membersMap.value(username));
+    if (namesake)
+        emit q->memberRenamed(namesake);
 }
 
 inline auto makeErrorStr(const Event& e, QByteArray msg)
@@ -1327,8 +1334,16 @@ void Room::processStateEvents(const RoomEvents& events)
                     if (memberJoinState(u) != JoinState::Join)
                     {
                         d->insertMemberIntoMap(u);
+                        connect(u, &User::nameAboutToChange, this,
+                            [=] (QString newName, QString, const Room* context) {
+                                if (context == this)
+                                    emit memberAboutToRename(u, newName);
+                            });
                         connect(u, &User::nameChanged, this,
-                                std::bind(&Private::renameMember, d, u, _2, _3));
+                            [=] (QString, QString oldName, const Room* context) {
+                                if (context == this)
+                                    d->renameMember(u, oldName);
+                            });
                         emit userAdded(u);
                     }
                 }
@@ -1603,11 +1618,15 @@ MemberSorter Room::memberSorter() const
 
 bool MemberSorter::operator()(User *u1, User *u2) const
 {
+    return operator()(u1, room->roomMembername(u2));
+}
+
+bool MemberSorter::operator ()(User* u1, const QString& u2name) const
+{
     auto n1 = room->roomMembername(u1);
-    auto n2 = room->roomMembername(u2);
     if (n1.startsWith('@'))
         n1.remove(0, 1);
-    if (n2.startsWith('@'))
-        n2.remove(0, 1);
+    auto n2 = u2name.midRef(u2name.startsWith('@') ? 1 : 0);
+
     return n1.localeAwareCompare(n2) < 0;
 }
diff --git a/room.h b/room.h
index 0ef17abb..5253a7c6 100644
--- a/room.h
+++ b/room.h
@@ -301,6 +301,7 @@ namespace QMatrixClient
             void avatarChanged();
             void userAdded(User* user);
             void userRemoved(User* user);
+            void memberAboutToRename(User* user, QString newName);
             void memberRenamed(User* user);
             void memberListChanged();
 
@@ -345,12 +346,13 @@ namespace QMatrixClient
             explicit MemberSorter(const Room* r) : room(r) { }
 
             bool operator()(User* u1, User* u2) const;
+            bool operator()(User* u1, const QString& u2name) const;
 
-            template <typename ContT>
+            template <typename ContT, typename ValT>
             typename ContT::size_type lowerBoundIndex(const ContT& c,
-                                                      typename ContT::value_type v) const
+                                                      const ValT& v) const
             {
-                return  std::lower_bound(c.begin(), c.end(), v, *this) - c.begin();
+                return std::lower_bound(c.begin(), c.end(), v, *this) - c.begin();
             }
 
         private:
diff --git a/user.cpp b/user.cpp
index 2a3071af..8a9fa515 100644
--- a/user.cpp
+++ b/user.cpp
@@ -62,8 +62,8 @@ class User::Private
 
         mutable int totalRooms = 0;
 
-        QString nameForRoom(const Room* r) const;
-        std::pair<bool, QString> setNameForRoom(const Room* r, QString newName);
+        QString nameForRoom(const Room* r, const QString& hint = {}) const;
+        void setNameForRoom(const Room* r, QString newName, QString oldName);
         const Avatar& avatarForRoom(const Room* r) const;
         bool setAvatarUrlForRoom(const Room* r, const QUrl& avatarUrl);
 
@@ -78,34 +78,29 @@ QIcon User::Private::defaultIcon()
     return icon;
 }
 
-QString User::Private::nameForRoom(const Room* r) const
+QString User::Private::nameForRoom(const Room* r, const QString& hint) const
 {
+    // If the hint is accurate, this function is O(1) instead of O(n)
+    if (hint == mostUsedName || otherNames.contains(hint, r))
+        return hint;
     return otherNames.key(r, mostUsedName);
 }
 
-static constexpr int MIN_JOINED_ROOMS_TO_LOG = 100;
+static constexpr int MIN_JOINED_ROOMS_TO_LOG = 20;
 
-std::pair<bool, QString> User::Private::setNameForRoom(const Room* r,
-                                                       QString newName)
+void User::Private::setNameForRoom(const Room* r, QString newName,
+                                   QString oldName)
 {
+    Q_ASSERT(oldName != newName);
+    Q_ASSERT(oldName == mostUsedName || otherNames.contains(oldName, r));
     if (totalRooms < 2)
     {
         Q_ASSERT_X(totalRooms > 0 && otherNames.empty(), __FUNCTION__,
                    "Internal structures inconsistency");
-        // The below uses that initialization list evaluation is ordered
-        return { mostUsedName != newName,
-                    exchange(mostUsedName, move(newName)) };
-    }
-    QString oldName;
-    // The below works because QMultiHash iterators dereference to stored values
-    auto it = std::find(otherNames.begin(), otherNames.end(), r);
-    if (it != otherNames.end())
-    {
-        oldName = it.key();
-        if (oldName == newName)
-            return { false, oldName }; // old name and new name coincide
-        otherNames.erase(it);
+        mostUsedName = move(newName);
+        return;
     }
+    otherNames.remove(oldName, r);
     if (newName != mostUsedName)
     {
         // Check if the newName is about to become most used.
@@ -134,7 +129,6 @@ std::pair<bool, QString> User::Private::setNameForRoom(const Room* r,
         else
             otherNames.insert(newName, r);
     }
-    return { true, oldName };
 }
 
 const Avatar& User::Private::avatarForRoom(const Room* r) const
@@ -255,11 +249,19 @@ QString User::name(const Room* room) const
 
 void User::updateName(const QString& newName, const Room* room)
 {
-    const auto setNameResult = d->setNameForRoom(room, newName);
-    if (setNameResult.first)
+    updateName(newName, d->nameForRoom(room), room);
+}
+
+void User::updateName(const QString& newName, const QString& oldName,
+                      const Room* room)
+{
+    Q_ASSERT(oldName == d->mostUsedName || d->otherNames.contains(oldName, room));
+    if (newName != oldName)
     {
+        emit nameAboutToChange(newName, oldName, room);
+        d->setNameForRoom(room, newName, oldName);
         setObjectName(displayname());
-        emit nameChanged(newName, setNameResult.second, room);
+        emit nameChanged(newName, oldName, room);
     }
 }
 
@@ -337,7 +339,8 @@ QImage User::avatar(int width, int height, const Room* room)
     return avatar(width, height, room, []{});
 }
 
-QImage User::avatar(int width, int height, const Room* room, Avatar::get_callback_t callback)
+QImage User::avatar(int width, int height, const Room* room,
+                    Avatar::get_callback_t callback)
 {
     return avatarObject(room).get(d->connection, width, height,
                 [=] { emit avatarChanged(this, room); callback(); });
@@ -385,7 +388,12 @@ void User::processEvent(RoomMemberEvent* event, const Room* room)
         }
         newName.truncate(match.capturedStart(0));
     }
-    updateName(event->displayName(), room);
+    if (event->prevContent())
+        updateName(event->displayName(),
+                   d->nameForRoom(room, event->prevContent()->displayName),
+                   room);
+    else
+        updateName(event->displayName(), room);
     if (d->setAvatarUrlForRoom(room, event->avatarUrl()))
         emit avatarChanged(this, room);
 }
diff --git a/user.h b/user.h
index 0a9d3da1..d1d28312 100644
--- a/user.h
+++ b/user.h
@@ -88,7 +88,8 @@ namespace QMatrixClient
             Q_INVOKABLE QImage avatar(int dimension, const Room* room = nullptr);
             Q_INVOKABLE QImage avatar(int requestedWidth, int requestedHeight,
                                       const Room* room = nullptr);
-            QImage avatar(int width, int height, const Room* room, Avatar::get_callback_t callback);
+            QImage avatar(int width, int height, const Room* room,
+                          Avatar::get_callback_t callback);
 
             QString avatarMediaId(const Room* room = nullptr) const;
             QUrl avatarUrl(const Room* room = nullptr) const;
@@ -102,12 +103,16 @@ namespace QMatrixClient
             bool setAvatar(QIODevice* source);
 
         signals:
+            void nameAboutToChange(QString newName, QString oldName,
+                                   const Room* roomContext);
             void nameChanged(QString newName, QString oldName,
                              const Room* roomContext);
             void avatarChanged(User* user, const Room* roomContext);
 
         private slots:
             void updateName(const QString& newName, const Room* room = nullptr);
+            void updateName(const QString& newName, const QString& oldName,
+                            const Room* room = nullptr);
 
         private:
             class Private;