Direct chat (un)marking: update internal structure synchronously

The asynchronous update first implemented was more verbose and caused
more problems than provided solutions. The idea was that the internal
directChats map would better reflect the server state if updated
asynchronously. However, it also causes a local race condition; e.g., to
quickly remove rooms from direct chats one after another becomes very
non-trivial (one has to wait until the previous operation succeeds). So
after some playing with the code, hitting pitfalls along the way, I
decided to align the logic with the one for room tags; synchronously
issued signals look uglyish but at least work predictably. And race
conditions between several clients generally cannot be cleanly resolved
anyway.

3 files changed, 52 insertions, 51 deletions

diff --git a/connection.cpp b/connection.cpp
index df9fd35f..b32f38ea 100644
--- a/connection.cpp
+++ b/connection.cpp
@@ -80,7 +80,7 @@ class Connection::Private
 
         void connectWithToken(const QString& user, const QString& accessToken,
                               const QString& deviceId);
-        void applyDirectChatUpdates(const DirectChatsMap& newMap);
+        void broadcastDirectChatUpdates();
 };
 
 Connection::Connection(const QUrl& server, QObject* parent)
@@ -655,16 +655,11 @@ QJsonObject toJson(const DirectChatsMap& directChats)
     return json;
 }
 
-void Connection::Private::applyDirectChatUpdates(const DirectChatsMap& newMap)
+void Connection::Private::broadcastDirectChatUpdates()
 {
-    auto j = q->callApi<SetAccountDataJob>(userId, "m.direct", toJson(newMap));
-    connect(j, &BaseJob::success, q, [this, newMap] {
-        if (directChats != newMap)
-        {
-            directChats = newMap;
-            emit q->directChatsListChanged();
-        }
-    });
+    auto j = q->callApi<SetAccountDataJob>(userId, QStringLiteral("m.direct"),
+                                           toJson(directChats));
+    emit q->directChatsListChanged();
 }
 
 void Connection::addToDirectChats(const Room* room, const User* user)
@@ -672,29 +667,32 @@ void Connection::addToDirectChats(const Room* room, const User* user)
     Q_ASSERT(room != nullptr && user != nullptr);
     if (d->directChats.contains(user, room->id()))
         return;
-    auto newMap = d->directChats;
-    newMap.insert(user, room->id());
-    d->applyDirectChatUpdates(newMap);
+    d->directChats.insert(user, room->id());
+    d->broadcastDirectChatUpdates();
 }
 
-void Connection::removeFromDirectChats(const Room* room, const User* user)
+void Connection::removeFromDirectChats(const QString& roomId, const User* user)
 {
-    Q_ASSERT(room != nullptr);
-    if ((user != nullptr && !d->directChats.contains(user, room->id())) ||
-            d->directChats.key(room->id()) == nullptr)
+    Q_ASSERT(!roomId.isEmpty());
+    if ((user != nullptr && !d->directChats.contains(user, roomId)) ||
+            d->directChats.key(roomId) == nullptr)
         return;
-    DirectChatsMap newMap;
-    for (auto it = d->directChats.begin(); it != d->directChats.end(); ++it)
-    {
-        if (it.value() != room->id() || (user != nullptr && it.key() != user))
-            newMap.insert(it.key(), it.value());
-    }
-    d->applyDirectChatUpdates(newMap);
+    if (user != nullptr)
+        d->directChats.remove(user, roomId);
+    else
+        for (auto it = d->directChats.begin(); it != d->directChats.end();)
+        {
+            if (it.value() == roomId)
+                it = d->directChats.erase(it);
+            else
+                ++it;
+        }
+    d->broadcastDirectChatUpdates();
 }
 
-bool Connection::isDirectChat(const Room* room) const
+bool Connection::isDirectChat(const QString& roomId) const
 {
-    return d->directChats.key(room->id()) != nullptr;
+    return d->directChats.key(roomId) != nullptr;
 }
 
 QMap<QString, User*> Connection::users() const
diff --git a/connection.h b/connection.h
index 4497e200..6a5285f9 100644
--- a/connection.h
+++ b/connection.h
@@ -89,18 +89,30 @@ namespace QMatrixClient
             /** Get the list of rooms with the specified tag */
             QVector<Room*> roomsWithTag(const QString& tagName) const;
 
-            /** Mark the room as a direct chat with the user */
+            /** Mark the room as a direct chat with the user
+             * This function marks \p room as a direct chat with \p user.
+             * Emits the signal synchronously, without waiting to complete
+             * synchronisation with the server.
+             *
+             * \sa directChatsListChanged
+             */
             void addToDirectChats(const Room* room, const User* user);
 
             /** Unmark the room from direct chats
-             * This function removes the room from direct chats either for
+             * This function removes the room id from direct chats either for
              * a specific \p user or for all users if \p user in nullptr.
+             * The room id is used to allow removal of, e.g., ids of forgotten
+             * rooms; a Room object need not exist. Emits the signal
+             * immediately, without waiting to complete synchronisation with
+             * the server.
+             *
+             * \sa directChatsListChanged
              */
-            void removeFromDirectChats(const Room* room,
+            void removeFromDirectChats(const QString& roomId,
                                        const User* user = nullptr);
 
-            /** Check whether the room is a direct chat */
-            bool isDirectChat(const Room* room) const;
+            /** Check whether the room id corresponds to a direct chat */
+            bool isDirectChat(const QString& roomId) const;
 
             QMap<QString, User*> users() const;
 
diff --git a/examples/qmc-example.cpp b/examples/qmc-example.cpp
index 5ea91856..23a1bff1 100644
--- a/examples/qmc-example.cpp
+++ b/examples/qmc-example.cpp
@@ -126,7 +126,8 @@ void QMCTest::addAndRemoveTag()
     if (targetRoom->tags().contains(TestTag))
         targetRoom->removeTag(TestTag);
 
-    QObject::connect(targetRoom, &Room::tagsChanged, targetRoom, [=] {
+    // Connect first because the signal is emitted synchronously.
+    connect(targetRoom, &Room::tagsChanged, targetRoom, [=] {
         cout << "Room " << targetRoom->id().toStdString()
              << ", tag(s) changed:" << endl
              << "  " << targetRoom->tagNames().join(", ").toStdString() << endl;
@@ -138,7 +139,6 @@ void QMCTest::addAndRemoveTag()
             QObject::disconnect(targetRoom, &Room::tagsChanged, nullptr, nullptr);
         }
     });
-    // The reverse order because tagsChanged is emitted synchronously.
     cout << "Adding a tag" << endl;
     targetRoom->addTag(TestTag);
 }
@@ -211,40 +211,31 @@ void QMCTest::checkRedactionOutcome(QString evtIdToRedact,
 
 void QMCTest::markDirectChat()
 {
-    if (c->isDirectChat(targetRoom))
+    if (c->isDirectChat(targetRoom->id()))
     {
         cout << "Warning: the room is already a direct chat,"
                 " only unmarking will be tested" << endl;
         checkDirectChatOutcome();
     }
-    cout << "Marking the room as a direct chat" << endl;
-    c->addToDirectChats(targetRoom, c->user());
+    // Connect first because the signal is emitted synchronously.
     connect(c.data(), &Connection::directChatsListChanged,
             this, &QMCTest::checkDirectChatOutcome);
+    cout << "Marking the room as a direct chat" << endl;
+    c->addToDirectChats(targetRoom, c->user());
 }
 
 void QMCTest::checkDirectChatOutcome()
 {
-    if (!c->isDirectChat(targetRoom))
+    disconnect(c.data(), &Connection::directChatsListChanged, nullptr, nullptr);
+    if (!c->isDirectChat(targetRoom->id()))
     {
-        cout << "Room not (yet?) added to direct chats, waiting" << endl;
+        QMC_CHECK("Direct chat test", false);
         return;
     }
 
     cout << "Room marked as a direct chat, unmarking now" << endl;
-    disconnect(c.data(), &Connection::directChatsListChanged, nullptr, nullptr);
-    c->removeFromDirectChats(targetRoom, c->user());
-    connect(c.data(), &Connection::directChatsListChanged, this, [this] {
-        if (c->isDirectChat(targetRoom))
-        {
-            cout << "Room not (yet?) removed from direct chats, waiting" << endl;
-            return;
-        }
-
-        QMC_CHECK("Direct chat test", !c->isDirectChat(targetRoom));
-        disconnect(c.data(), &Connection::directChatsListChanged,
-                   nullptr, nullptr);
-    });
+    c->removeFromDirectChats(targetRoom->id(), c->user());
+    QMC_CHECK("Direct chat test", !c->isDirectChat(targetRoom->id()));
 }
 
 void QMCTest::finalize()