From eed5a6f127ec3bb1553ac629457f196d8893665a Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Mon, 26 Jul 2021 08:19:29 +0200 Subject: Room::usersAtEventId(): switch from QMultiHash to QHash of QSets While slightly more complex for updating, this allows COW to kick in in the read accessor; using QSet instead of QList also provides better consistency guarantees. For QML both are converted to an Array-like collection since Qt 5.15; Qt 5.12 turns QSet<> in a QVariantList, according to the documentation, which is quite reasonable too. --- lib/room.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 10e827d7..13589ee9 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -121,7 +121,7 @@ public: int notificationCount = 0; members_map_t membersMap; QList usersTyping; - QMultiHash eventIdReadUsers; + QHash> eventIdReadUsers; QList usersInvited; QList membersLeft; int unreadMessages = 0; @@ -627,8 +627,11 @@ Room::Changes Room::Private::setLastReadEvent(User* u, QString eventId) auto& storedId = lastReadEventIds[u]; if (storedId == eventId) return Change::NoChange; - eventIdReadUsers.remove(storedId, u); - eventIdReadUsers.insert(eventId, u); + auto& oldEventReadUsers = eventIdReadUsers[storedId]; + oldEventReadUsers.remove(u); + if (oldEventReadUsers.isEmpty()) + eventIdReadUsers.remove(storedId); + eventIdReadUsers[eventId].insert(u); swap(storedId, eventId); emit q->lastReadEventChanged(u); emit q->readMarkerForUserMoved(u, eventId, storedId); @@ -965,9 +968,9 @@ QString Room::readMarkerEventId() const return d->lastReadEventIds.value(localUser()); } -QList Room::usersAtEventId(const QString& eventId) +QSet Room::usersAtEventId(const QString& eventId) { - return d->eventIdReadUsers.values(eventId); + return d->eventIdReadUsers.value(eventId); } int Room::notificationCount() const { return d->notificationCount; } -- cgit v1.2.3 From 277f43defe3fa55ff32fe53952c6331a81d65a20 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 28 Jul 2021 03:32:02 +0200 Subject: Room: fix the read markers/receipts confusion This turns the design changes laid out in #464 comments to code, as of 0.6.x branch (0.7 API will be introduced separately): - readMarker() now returns the fully read marker, unlike readMarker(User*) that returns a read receipt, even when called for the local user. - Private::setLastReadEvent() -> setLastReadReceipt(), incorporating the "promotion" logic from promoteReadReceipt(). - The above makes promoteReadReceipt() unneeded; the remaining piece of logic that recalculates the number of unread messages is put to its own method - Private::recalculateUnreadCount(). - Private::updateUnreadCount() is only slightly refreshed, continues to use the fully read marker position (as it used to). - Now that read receipts and fully read markers are managed separately, Private::setLastReadReceipt() has got its counterpart, Private::setFullyReadMarker(); both only update their respective markers locally (emitting signals as needed), without interaction with the homeserver. - Private::markMessagesAsRead() now delegates updating the fully read marker to setFullyReadMarker() and on top of that sends the new fully read marker to the homeserver. - Private::serverReadMarker -> fullyReadUntilEventId (to be really clear what it stores). - The hand-written PostReadMarkersJob is replaced with the generated SetReadMarkerJob that does the same thing (and can update the read receipt on top). --- lib/room.cpp | 341 +++++++++++++++++++++++++++++++---------------------------- 1 file changed, 180 insertions(+), 161 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 13589ee9..ec3a9532 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -129,7 +129,7 @@ public: QString firstDisplayedEventId; QString lastDisplayedEventId; QHash lastReadEventIds; - QString serverReadMarker; + QString fullyReadUntilEventId; TagsMap tags; UnorderedMap accountData; QString prevBatch; @@ -290,11 +290,12 @@ public: */ void dropDuplicateEvents(RoomEvents& events) const; - Changes setLastReadEvent(User* u, QString eventId); - void updateUnreadCount(const rev_iter_t& from, const rev_iter_t& to); - Changes promoteReadMarker(User* u, const rev_iter_t& newMarker, bool force = false); - - Changes markMessagesAsRead(rev_iter_t upToMarker); + void setLastReadReceipt(User* u, rev_iter_t newMarker, + QString newEvtId = {}); + Changes setFullyReadMarker(const QString &eventId); + Changes updateUnreadCount(const rev_iter_t& from, const rev_iter_t& to); + Changes recalculateUnreadCount(bool force = false); + void markMessagesAsRead(const rev_iter_t &upToMarker); void getAllMembers(); @@ -622,142 +623,172 @@ void Room::setJoinState(JoinState state) emit joinStateChanged(oldState, state); } -Room::Changes Room::Private::setLastReadEvent(User* u, QString eventId) +void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, + QString newEvtId) { + if (!u) { + Q_ASSERT(u != nullptr); // For Debug builds + qCCritical(MAIN) << "Empty user, skipping read receipt registration"; + return; // For Release builds + } + if (q->memberJoinState(u) != JoinState::Join) { + qCWarning(MAIN) << "Won't record read receipt for non-member" << u->id(); + return; + } + + if (newMarker == timeline.crend() && !newEvtId.isEmpty()) + newMarker = q->findInTimeline(newEvtId); + if (newMarker != timeline.crend()) { + // NB: with reverse iterators, timeline history >= sync edge + if (newMarker >= q->readMarker(u)) + return; + + // Try to auto-promote the read marker over the user's own messages + // (switch to direct iterators for that). + const auto eagerMarker = find_if(newMarker.base(), timeline.cend(), + [=](const TimelineItem& ti) { + return ti->senderId() != u->id(); + }) + - 1; + newEvtId = (*eagerMarker)->id(); + if (eagerMarker != newMarker.base() - 1) // &*(rIt.base() - 1) === &*rIt + qCDebug(EPHEMERAL) << "Auto-promoted read receipt for" << u->id() + << "to" << newEvtId; + } + auto& storedId = lastReadEventIds[u]; - if (storedId == eventId) - return Change::NoChange; + if (storedId == newEvtId) + return; + // Finally make the change auto& oldEventReadUsers = eventIdReadUsers[storedId]; oldEventReadUsers.remove(u); if (oldEventReadUsers.isEmpty()) eventIdReadUsers.remove(storedId); - eventIdReadUsers[eventId].insert(u); - swap(storedId, eventId); + eventIdReadUsers[newEvtId].insert(u); + swap(storedId, newEvtId); // Now newEvtId actually stores the old eventId emit q->lastReadEventChanged(u); - emit q->readMarkerForUserMoved(u, eventId, storedId); - if (isLocalUser(u)) { - if (storedId != serverReadMarker) - connection->callApi(BackgroundRequest, id, - storedId); - emit q->readMarkerMoved(eventId, storedId); - return Change::ReadMarkerChange; - } - return Change::NoChange; + if (!isLocalUser(u)) + emit q->readMarkerForUserMoved(u, newEvtId, storedId); } -void Room::Private::updateUnreadCount(const rev_iter_t& from, - const rev_iter_t& to) +Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, + const rev_iter_t& to) { Q_ASSERT(from >= timeline.crbegin() && from <= timeline.crend()); Q_ASSERT(to >= from && to <= timeline.crend()); - // Catch a special case when the last read event id refers to an event - // that has just arrived. In this case we should recalculate - // unreadMessages and might need to promote the read marker further - // over local-origin messages. - auto readMarker = q->readMarker(); - if (readMarker == timeline.crend() && q->allHistoryLoaded()) - --readMarker; // Read marker not found in the timeline, initialise it - if (readMarker >= from && readMarker < to) { - promoteReadMarker(q->localUser(), readMarker, true); - return; - } - - Q_ASSERT(to <= readMarker); + auto fullyReadMarker = q->readMarker(); + if (fullyReadMarker < from) + return NoChange; // What's arrived is already fully read + + if (fullyReadMarker == timeline.crend() && q->allHistoryLoaded()) + --fullyReadMarker; // No read marker in the whole room, initialise it + if (fullyReadMarker < to) { + // Catch a special case when the last fully read event id refers to an + // event that has just arrived. In this case we should recalculate + // unreadMessages to get an exact number instead of an estimation + // (see https://github.com/quotient-im/libQuotient/wiki/unread_count). + // For the same reason (switching from the estimation to the exact + // number) this branch always emits unreadMessagesChanged() and returns + // UnreadNotifsChange, even if the estimation luckily matched the exact + // result. + return recalculateUnreadCount(true); + } + + // Fully read marker is somewhere beyond the most historical message from + // the arrived batch - add up newly arrived messages to the current counter, + // instead of a complete recalculation. + Q_ASSERT(to <= fullyReadMarker); QElapsedTimer et; et.start(); const auto newUnreadMessages = - count_if(from, to, std::bind(&Room::Private::isEventNotable, this, _1)); + count_if(from, to, + std::bind(&Room::Private::isEventNotable, this, _1)); if (et.nsecsElapsed() > profilerMinNsecs() / 10) qCDebug(PROFILER) << "Counting gained unread messages took" << et; - if (newUnreadMessages > 0) { - // See https://github.com/quotient-im/libQuotient/wiki/unread_count - if (unreadMessages < 0) - unreadMessages = 0; - - unreadMessages += newUnreadMessages; - qCDebug(MESSAGES) << "Room" << q->objectName() << "has gained" - << newUnreadMessages << "unread message(s)," - << (q->readMarker() == timeline.crend() - ? "in total at least" - : "in total") - << unreadMessages << "unread message(s)"; - emit q->unreadMessagesChanged(q); - } -} + if (newUnreadMessages == 0) + return NoChange; -Room::Changes Room::Private::promoteReadMarker(User* u, - const rev_iter_t& newMarker, - bool force) -{ - Q_ASSERT_X(u, __FUNCTION__, "User* should not be nullptr"); - Q_ASSERT(newMarker >= timeline.crbegin() && newMarker <= timeline.crend()); + // See https://github.com/quotient-im/libQuotient/wiki/unread_count + if (unreadMessages < 0) + unreadMessages = 0; + + unreadMessages += newUnreadMessages; + qCDebug(MESSAGES) << "Room" << q->objectName() << "has gained" + << newUnreadMessages << "unread message(s)," + << (q->readMarker() == timeline.crend() + ? "in total at least" + : "in total") + << unreadMessages << "unread message(s)"; + emit q->unreadMessagesChanged(q); + return UnreadNotifsChange; +} + +Room::Changes Room::Private::recalculateUnreadCount(bool force) +{ + // The recalculation logic assumes that the fully read marker points at + // a specific position in the timeline + Q_ASSERT(q->readMarker() != timeline.crend()); + const auto oldUnreadCount = unreadMessages; + QElapsedTimer et; + et.start(); + unreadMessages = + int(count_if(timeline.crbegin(), q->readMarker(), + [this](const auto& ti) { return isEventNotable(ti); })); + if (et.nsecsElapsed() > profilerMinNsecs() / 10) + qCDebug(PROFILER) << "Recounting unread messages took" << et; - const auto prevMarker = q->readMarker(u); - if (!force && prevMarker <= newMarker) // Remember, we deal with reverse - // iterators - return Change::NoChange; + // See https://github.com/quotient-im/libQuotient/wiki/unread_count + if (unreadMessages == 0) + unreadMessages = -1; - Q_ASSERT(newMarker < timeline.crend()); + if (!force && unreadMessages == oldUnreadCount) + return NoChange; - // Try to auto-promote the read marker over the user's own messages - // (switch to direct iterators for that). - auto eagerMarker = - find_if(newMarker.base(), timeline.cend(), [=](const TimelineItem& ti) { - return ti->senderId() != u->id(); - }); + if (unreadMessages == -1) + qCDebug(MESSAGES) + << "Room" << displayname << "has no more unread messages"; + else + qCDebug(MESSAGES) << "Room" << displayname << "still has" + << unreadMessages << "unread message(s)"; + emit q->unreadMessagesChanged(q); + return UnreadNotifsChange; +} - auto changes = setLastReadEvent(u, (*(eagerMarker - 1))->id()); - if (isLocalUser(u)) { - const auto oldUnreadCount = unreadMessages; - QElapsedTimer et; - et.start(); - unreadMessages = - int(count_if(eagerMarker, timeline.cend(), - [this](const auto& ti) { return isEventNotable(ti); })); - if (et.nsecsElapsed() > profilerMinNsecs() / 10) - qCDebug(PROFILER) << "Recounting unread messages took" << et; +Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) +{ + if (fullyReadUntilEventId == eventId) + return NoChange; - // See https://github.com/quotient-im/libQuotient/wiki/unread_count - if (unreadMessages == 0) - unreadMessages = -1; + const auto prevFullyReadId = std::exchange(fullyReadUntilEventId, eventId); + qCDebug(MESSAGES) << "Fully read marker in" << q->objectName() // + << "moved to" << fullyReadUntilEventId; + emit q->readMarkerMoved(prevFullyReadId, fullyReadUntilEventId); - if (force || unreadMessages != oldUnreadCount) { - if (unreadMessages == -1) { - qCDebug(MESSAGES) - << "Room" << displayname << "has no more unread messages"; - } else - qCDebug(MESSAGES) << "Room" << displayname << "still has" - << unreadMessages << "unread message(s)"; - emit q->unreadMessagesChanged(q); - changes |= Change::UnreadNotifsChange; - } + Changes changes = ReadMarkerChange; + if (const auto rm = q->readMarker(); rm != timeline.crend()) { + // Pull read receipt if it's behind + if (auto rr = q->readMarker(q->localUser()); rr > rm) + setLastReadReceipt(q->localUser(), rm); + + changes |= recalculateUnreadCount(); } return changes; } -Room::Changes Room::Private::markMessagesAsRead(rev_iter_t upToMarker) +void Room::Private::markMessagesAsRead(const rev_iter_t &upToMarker) { - const auto prevMarker = q->readMarker(); - auto changes = promoteReadMarker(q->localUser(), upToMarker); - if (prevMarker != upToMarker) - qCDebug(MESSAGES) << "Marked messages as read until" << *q->readMarker(); - - // We shouldn't send read receipts for the local user's own messages - so - // search earlier messages for the latest message not from the local user - // until the previous last-read message, whichever comes first. - for (; upToMarker < prevMarker; ++upToMarker) { - if ((*upToMarker)->senderId() != q->localUser()->id()) { - connection->callApi(BackgroundRequest, - id, QStringLiteral("m.read"), - QUrl::toPercentEncoding( - (*upToMarker)->id())); - break; - } + if (upToMarker < q->readMarker()) { + setFullyReadMarker((*upToMarker)->id()); + // Assuming that if a read receipt was sent on a newer event, it will + // stay there instead of "un-reading" notifications/mentions from + // m.fully_read to m.read + connection->callApi(BackgroundRequest, id, + fullyReadUntilEventId, + fullyReadUntilEventId); } - return changes; } void Room::markMessagesAsRead(QString uptoEventId) @@ -961,11 +992,14 @@ Room::rev_iter_t Room::readMarker(const User* user) const return findInTimeline(d->lastReadEventIds.value(user)); } -Room::rev_iter_t Room::readMarker() const { return readMarker(localUser()); } +Room::rev_iter_t Room::readMarker() const +{ + return findInTimeline(d->fullyReadUntilEventId); +} QString Room::readMarkerEventId() const { - return d->lastReadEventIds.value(localUser()); + return d->fullyReadUntilEventId; } QSet Room::usersAtEventId(const QString& eventId) @@ -2436,24 +2470,21 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) << timeline.back(); // The first event in the just-added batch (referred to by `from`) - // defines whose read marker can possibly be promoted any further over + // defines whose read receipt can possibly be promoted any further over // the same author's events newly arrived. Others will need explicit - // read receipts from the server (or, for the local user, - // markMessagesAsRead() invocation) to promote their read markers over + // read receipts from the server - or, for the local user, calling + // setLastDisplayedEventId() - to promote their read receipts over // the new message events. - if (const auto senderId = (*from)->senderId(); !senderId.isEmpty()) { - auto* const firstWriter = q->user(senderId); - if (q->readMarker(firstWriter) != timeline.crend()) { - roomChanges |= - promoteReadMarker(firstWriter, rev_iter_t(from) - 1); - qCDebug(MESSAGES) - << "Auto-promoted read marker for" << senderId - << "to" << *q->readMarker(firstWriter); - } + auto* const firstWriter = q->user((*from)->senderId()); + setLastReadReceipt(firstWriter, rev_iter_t(from + 1)); + if (firstWriter == q->localUser() && q->readMarker().base() == from) { + // If the local user's message(s) is/are first in the batch + // and the fully read marker was right before it, promote + // the fully read marker to the same event as the read receipt. + roomChanges |= + setFullyReadMarker(lastReadEventIds.value(firstWriter)); } - - updateUnreadCount(timeline.crbegin(), rev_iter_t(from)); - roomChanges |= Change::UnreadNotifsChange; + roomChanges |= updateUnreadCount(timeline.crbegin(), rev_iter_t(from)); } Q_ASSERT(timeline.size() == timelineSize + totalInserted); @@ -2498,8 +2529,11 @@ void Room::Private::addHistoricalMessageEvents(RoomEvents&& events) emit q->updatedEvent(relation.eventId); } } - if (from <= q->readMarker()) - updateUnreadCount(from, timeline.crend()); + updateUnreadCount(from, timeline.crend()); + // When there are no unread messages and the read marker is within the + // known timeline, unreadMessages == -1 + // (see https://github.com/quotient-im/libQuotient/wiki/unread_count). + Q_ASSERT(unreadMessages != 0 || q->readMarker() == timeline.crend()); Q_ASSERT(timeline.size() == timelineSize + insertedSize); if (insertedSize > 9 || et.nsecsElapsed() >= profilerMinNsecs()) @@ -2723,7 +2757,7 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) if (auto* evt = eventCast(event)) { d->usersTyping.clear(); for (const auto& userId : evt->users()) { - auto u = user(userId); + auto* const u = user(userId); if (memberJoinState(u) == JoinState::Join) d->usersTyping.append(u); } @@ -2746,30 +2780,21 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) << p.receipts.size() << "users"; } const auto newMarker = findInTimeline(p.evtId); - if (newMarker != timelineEdge()) { - for (const Receipt& r : p.receipts) { - if (r.userId == connection()->userId()) - continue; // FIXME, #185 - auto u = user(r.userId); - if (memberJoinState(u) == JoinState::Join) - changes |= d->promoteReadMarker(u, newMarker); - } - } else { - qCDebug(EPHEMERAL) << "Event" << p.evtId - << "not found; saving read receipts anyway"; - // If the event is not found (most likely, because it's too old - // and hasn't been fetched from the server yet), but there is - // 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 (r.userId == connection()->userId()) - continue; // FIXME, #185 - auto u = user(r.userId); - if (memberJoinState(u) == JoinState::Join - && readMarker(u) == timelineEdge()) - changes |= d->setLastReadEvent(u, p.evtId); + if (newMarker == historyEdge()) + qCDebug(EPHEMERAL) << "Event of the read receipt(s) is not " + "found; saving them anyway"; + for (const Receipt& r : p.receipts) + if (auto* const u = user(r.userId); + memberJoinState(u) == JoinState::Join) { + // If the event is not found (most likely, because it's + // too old and hasn't been fetched from the server yet) + // but there is a previous marker for a user, keep + // the previous marker because read receipts are not + // supposed to move backwards. Otherwise, blindly + // store the event id for this user and update the read + // marker when/if the event is fetched later on. + d->setLastReadReceipt(u, newMarker, p.evtId); } - } } if (eventsWithReceipts.size() > 3 || totalReceipts > 10 || et.nsecsElapsed() >= profilerMinNsecs()) @@ -2789,15 +2814,9 @@ Room::Changes Room::processAccountDataEvent(EventPtr&& event) changes |= Change::TagsChange; } - if (auto* evt = eventCast(event)) { - auto readEventId = evt->event_id(); - qCDebug(STATE) << "Server-side read marker at" << readEventId; - d->serverReadMarker = readEventId; - const auto newMarker = findInTimeline(readEventId); - changes |= newMarker != timelineEdge() - ? d->markMessagesAsRead(newMarker) - : d->setLastReadEvent(localUser(), readEventId); - } + if (auto* evt = eventCast(event)) + changes |= d->setFullyReadMarker(evt->event_id()); + // For all account data events auto& currentData = d->accountData[event->matrixType()]; // A polymorphic event-specific comparison might be a bit more -- cgit v1.2.3 From 62c829cc8de8a870c08926c41331f2766e766f37 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 29 Jul 2021 21:58:56 +0200 Subject: Quotient::ReadReceipt; deprecated readMarker() It's now possible to get receipts along with their timestamps by calling Room::lastReadReceipt(). Together this new method, fullyReadMarker(), and lastFullyReadEventId() deprecate readMarker() overloads and readMarkerEventId() respectively. lastFullyReadEventId is also a Q_PROPERTY (deprecating readMarkerEventId); readMarkerMoved() signal is deprecated by fullyReadMarkerMoved(), while readMarkerForUserMoved() is deprecated in favour of existing lastReadEventChanged(). --- lib/room.cpp | 79 ++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 45 insertions(+), 34 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index ec3a9532..9a571127 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -128,7 +128,7 @@ public: bool displayed = false; QString firstDisplayedEventId; QString lastDisplayedEventId; - QHash lastReadEventIds; + QHash lastReadReceipts; QString fullyReadUntilEventId; TagsMap tags; UnorderedMap accountData; @@ -291,7 +291,7 @@ public: void dropDuplicateEvents(RoomEvents& events) const; void setLastReadReceipt(User* u, rev_iter_t newMarker, - QString newEvtId = {}); + ReadReceipt newReceipt = {}); Changes setFullyReadMarker(const QString &eventId); Changes updateUnreadCount(const rev_iter_t& from, const rev_iter_t& to); Changes recalculateUnreadCount(bool force = false); @@ -624,7 +624,7 @@ void Room::setJoinState(JoinState state) } void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, - QString newEvtId) + ReadReceipt newReceipt) { if (!u) { Q_ASSERT(u != nullptr); // For Debug builds @@ -636,11 +636,12 @@ void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, return; } - if (newMarker == timeline.crend() && !newEvtId.isEmpty()) - newMarker = q->findInTimeline(newEvtId); + auto& storedReceipt = lastReadReceipts[u]; + if (newMarker == timeline.crend() && !newReceipt.eventId.isEmpty()) + newMarker = q->findInTimeline(newReceipt.eventId); if (newMarker != timeline.crend()) { // NB: with reverse iterators, timeline history >= sync edge - if (newMarker >= q->readMarker(u)) + if (newMarker >= q->findInTimeline(storedReceipt.eventId)) return; // Try to auto-promote the read marker over the user's own messages @@ -650,25 +651,26 @@ void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, return ti->senderId() != u->id(); }) - 1; - newEvtId = (*eagerMarker)->id(); + newReceipt.eventId = (*eagerMarker)->id(); if (eagerMarker != newMarker.base() - 1) // &*(rIt.base() - 1) === &*rIt qCDebug(EPHEMERAL) << "Auto-promoted read receipt for" << u->id() - << "to" << newEvtId; + << "to" << newReceipt.eventId; } - auto& storedId = lastReadEventIds[u]; - if (storedId == newEvtId) + if (storedReceipt == newReceipt) return; // Finally make the change - auto& oldEventReadUsers = eventIdReadUsers[storedId]; + auto& oldEventReadUsers = eventIdReadUsers[storedReceipt.eventId]; oldEventReadUsers.remove(u); if (oldEventReadUsers.isEmpty()) - eventIdReadUsers.remove(storedId); - eventIdReadUsers[newEvtId].insert(u); - swap(storedId, newEvtId); // Now newEvtId actually stores the old eventId + eventIdReadUsers.remove(storedReceipt.eventId); + eventIdReadUsers[newReceipt.eventId].insert(u); + swap(storedReceipt, newReceipt); // Now newReceipt actually stores the old receipt emit q->lastReadEventChanged(u); + // TODO: remove in 0.8 if (!isLocalUser(u)) - emit q->readMarkerForUserMoved(u, newEvtId, storedId); + emit q->readMarkerForUserMoved(u, newReceipt.eventId, + storedReceipt.eventId); } Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, @@ -677,7 +679,7 @@ Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, Q_ASSERT(from >= timeline.crbegin() && from <= timeline.crend()); Q_ASSERT(to >= from && to <= timeline.crend()); - auto fullyReadMarker = q->readMarker(); + auto fullyReadMarker = q->fullyReadMarker(); if (fullyReadMarker < from) return NoChange; // What's arrived is already fully read @@ -718,7 +720,7 @@ Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, unreadMessages += newUnreadMessages; qCDebug(MESSAGES) << "Room" << q->objectName() << "has gained" << newUnreadMessages << "unread message(s)," - << (q->readMarker() == timeline.crend() + << (q->fullyReadMarker() == timeline.crend() ? "in total at least" : "in total") << unreadMessages << "unread message(s)"; @@ -730,12 +732,12 @@ Room::Changes Room::Private::recalculateUnreadCount(bool force) { // The recalculation logic assumes that the fully read marker points at // a specific position in the timeline - Q_ASSERT(q->readMarker() != timeline.crend()); + Q_ASSERT(q->fullyReadMarker() != timeline.crend()); const auto oldUnreadCount = unreadMessages; QElapsedTimer et; et.start(); unreadMessages = - int(count_if(timeline.crbegin(), q->readMarker(), + int(count_if(timeline.crbegin(), q->fullyReadMarker(), [this](const auto& ti) { return isEventNotable(ti); })); if (et.nsecsElapsed() > profilerMinNsecs() / 10) qCDebug(PROFILER) << "Recounting unread messages took" << et; @@ -765,22 +767,22 @@ Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) const auto prevFullyReadId = std::exchange(fullyReadUntilEventId, eventId); qCDebug(MESSAGES) << "Fully read marker in" << q->objectName() // << "moved to" << fullyReadUntilEventId; + emit q->fullyReadMarkerMoved(prevFullyReadId, fullyReadUntilEventId); + // TODO: Remove in 0.8 emit q->readMarkerMoved(prevFullyReadId, fullyReadUntilEventId); Changes changes = ReadMarkerChange; - if (const auto rm = q->readMarker(); rm != timeline.crend()) { + if (const auto rm = q->fullyReadMarker(); rm != timeline.crend()) { // Pull read receipt if it's behind - if (auto rr = q->readMarker(q->localUser()); rr > rm) - setLastReadReceipt(q->localUser(), rm); - - changes |= recalculateUnreadCount(); + setLastReadReceipt(q->localUser(), rm); + changes |= recalculateUnreadCount(); // TODO: updateUnreadCount()? } return changes; } void Room::Private::markMessagesAsRead(const rev_iter_t &upToMarker) { - if (upToMarker < q->readMarker()) { + if (upToMarker < q->fullyReadMarker()) { setFullyReadMarker((*upToMarker)->id()); // Assuming that if a read receipt was sent on a newer event, it will // stay there instead of "un-reading" notifications/mentions from @@ -989,17 +991,23 @@ void Room::setLastDisplayedEvent(TimelineItem::index_t index) Room::rev_iter_t Room::readMarker(const User* user) const { Q_ASSERT(user); - return findInTimeline(d->lastReadEventIds.value(user)); + return findInTimeline(lastReadReceipt(user->id()).eventId); } -Room::rev_iter_t Room::readMarker() const +Room::rev_iter_t Room::readMarker() const { return fullyReadMarker(); } + +QString Room::readMarkerEventId() const { return lastFullyReadEventId(); } + +ReadReceipt Room::lastReadReceipt(const QString& userId) const { - return findInTimeline(d->fullyReadUntilEventId); + return d->lastReadReceipts.value(user(userId)); } -QString Room::readMarkerEventId() const +QString Room::lastFullyReadEventId() const { return d->fullyReadUntilEventId; } + +Room::rev_iter_t Room::fullyReadMarker() const { - return d->fullyReadUntilEventId; + return findInTimeline(d->fullyReadUntilEventId); } QSet Room::usersAtEventId(const QString& eventId) @@ -2477,12 +2485,14 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) // the new message events. auto* const firstWriter = q->user((*from)->senderId()); setLastReadReceipt(firstWriter, rev_iter_t(from + 1)); - if (firstWriter == q->localUser() && q->readMarker().base() == from) { + if (firstWriter == q->localUser() + && q->fullyReadMarker().base() == from) // + { // If the local user's message(s) is/are first in the batch // and the fully read marker was right before it, promote // the fully read marker to the same event as the read receipt. roomChanges |= - setFullyReadMarker(lastReadEventIds.value(firstWriter)); + setFullyReadMarker(lastReadReceipts.value(firstWriter).eventId); } roomChanges |= updateUnreadCount(timeline.crbegin(), rev_iter_t(from)); } @@ -2533,7 +2543,7 @@ void Room::Private::addHistoricalMessageEvents(RoomEvents&& events) // When there are no unread messages and the read marker is within the // known timeline, unreadMessages == -1 // (see https://github.com/quotient-im/libQuotient/wiki/unread_count). - Q_ASSERT(unreadMessages != 0 || q->readMarker() == timeline.crend()); + Q_ASSERT(unreadMessages != 0 || q->fullyReadMarker() == timeline.crend()); Q_ASSERT(timeline.size() == timelineSize + insertedSize); if (insertedSize > 9 || et.nsecsElapsed() >= profilerMinNsecs()) @@ -2793,7 +2803,8 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) // supposed to move backwards. Otherwise, blindly // store the event id for this user and update the read // marker when/if the event is fetched later on. - d->setLastReadReceipt(u, newMarker, p.evtId); + d->setLastReadReceipt(u, newMarker, + { p.evtId, r.timestamp }); } } if (eventsWithReceipts.size() > 3 || totalReceipts > 10 -- cgit v1.2.3 From 2975b435f1ae198187183b883b5e666c7659e8fc Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Fri, 30 Jul 2021 23:25:01 +0200 Subject: Room::setReadReceipt() --- lib/room.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index c7f29b6d..649b1ed2 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -765,6 +765,15 @@ Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) return changes; } +void Room::setReadReceipt(const QString& atEventId) +{ + d->setLastReadReceipt(localUser(), historyEdge(), + { atEventId, QDateTime::currentDateTime() }); + connection()->callApi(BackgroundRequest, id(), + QStringLiteral("m.read"), + QUrl::toPercentEncoding(atEventId)); +} + void Room::Private::markMessagesAsRead(const rev_iter_t &upToMarker) { if (upToMarker < q->fullyReadMarker()) { -- cgit v1.2.3 From 8093ea85347588431c0ddbf6e178a4db5f51b909 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 1 Aug 2021 17:19:23 +0200 Subject: Enhanced logging for read receipts --- lib/room.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 649b1ed2..e7d4e137 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -617,7 +617,8 @@ void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, return; // For Release builds } if (q->memberJoinState(u) != JoinState::Join) { - qCWarning(MAIN) << "Won't record read receipt for non-member" << u->id(); + qCWarning(EPHEMERAL) + << "Won't record read receipt for non-member" << u->id(); return; } @@ -626,8 +627,11 @@ void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, newMarker = q->findInTimeline(newReceipt.eventId); if (newMarker != timeline.crend()) { // NB: with reverse iterators, timeline history >= sync edge - if (newMarker >= q->findInTimeline(storedReceipt.eventId)) + if (newMarker >= q->findInTimeline(storedReceipt.eventId)) { + qCDebug(EPHEMERAL) << "The new read receipt for" << u->id() + << "is at or behind the old one, skipping"; return; + } // Try to auto-promote the read marker over the user's own messages // (switch to direct iterators for that). @@ -651,6 +655,8 @@ void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, eventIdReadUsers.remove(storedReceipt.eventId); eventIdReadUsers[newReceipt.eventId].insert(u); swap(storedReceipt, newReceipt); // Now newReceipt actually stores the old receipt + qCDebug(EPHEMERAL) << "The new read receipt for" << u->id() << "is at" + << storedReceipt.eventId; emit q->lastReadEventChanged(u); // TODO: remove in 0.8 if (!isLocalUser(u)) -- cgit v1.2.3 From c79dcb673b6f2888faa4b62cd53689fde9f7ea10 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 1 Aug 2021 17:22:42 +0200 Subject: Room::toJson(): save the last local user's read receipt Read receipts are entangled with counting unread messages, and saving them also helps in not sending receipts for too old events. Other users' read receipts are still treated as truly ephemeral. --- lib/room.cpp | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index e7d4e137..1c84ece0 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -3003,6 +3003,32 @@ QJsonObject Room::Private::toJson() const { QStringLiteral("events"), accountDataEvents } }); } + if (const auto& readReceiptEventId = + lastReadReceipts.value(q->localUser()).eventId; + !readReceiptEventId.isEmpty()) // + { + // Okay, that's a mouthful; but basically, it's simply placing an m.read + // event in the 'ephemeral' section of the cached sync payload. + // See also receiptevent.* and m.read example in the spec. + // Only the local user's read receipt is saved - others' are really + // considered ephemeral but this one is useful in understanding where + // the user is in the timeline before any history is loaded. + result.insert( + QStringLiteral("ephemeral"), + QJsonObject { + { QStringLiteral("events"), + QJsonArray { QJsonObject { + { TypeKey, ReceiptEvent::matrixTypeId() }, + { ContentKey, + QJsonObject { + { readReceiptEventId, + QJsonObject { + { QStringLiteral("m.read"), + QJsonObject { + { connection->userId(), + QJsonObject {} } } } } } } } } } } }); + } + QJsonObject unreadNotifObj { { SyncRoomData::UnreadCountKey, unreadMessages } }; -- cgit v1.2.3 From 4f08c88d234119c2a76874ebd2b1433b81992427 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Sun, 1 Aug 2021 17:47:56 +0200 Subject: Room::setDisplayed(): Don't reset unread counters Closes #489. --- lib/room.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 1c84ece0..8462902f 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -924,11 +924,8 @@ void Room::setDisplayed(bool displayed) d->displayed = displayed; emit displayedChanged(displayed); - if (displayed) { - resetHighlightCount(); - resetNotificationCount(); + if (displayed) d->getAllMembers(); - } } QString Room::firstDisplayedEventId() const { return d->firstDisplayedEventId; } -- cgit v1.2.3 From 5c0346f3a700e6af31463490b7af3382b86e09d5 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Wed, 8 Sep 2021 05:47:10 +0200 Subject: Room: actually initialise read marker when needed This fixes the `q->readMarker() != historyEdge()` assertion failure occuring in recalculateUnreadCount() when new events from sync arrive to a room with no read marker and all history loaded. --- lib/room.cpp | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index a2b99039..ea8df286 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -686,23 +686,24 @@ Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, if (fullyReadMarker < from) return NoChange; // What's arrived is already fully read + // If there's no read marker in the whole room, initialise it if (fullyReadMarker == historyEdge() && q->allHistoryLoaded()) - --fullyReadMarker; // No read marker in the whole room, initialise it - if (fullyReadMarker < to) { - // Catch a special case when the last fully read event id refers to an - // event that has just arrived. In this case we should recalculate - // unreadMessages to get an exact number instead of an estimation - // (see https://github.com/quotient-im/libQuotient/wiki/unread_count). - // For the same reason (switching from the estimation to the exact - // number) this branch always emits unreadMessagesChanged() and returns - // UnreadNotifsChange, even if the estimation luckily matched the exact - // result. + return setFullyReadMarker(timeline.front()->id()); + + // Catch a special case when the last fully read event id refers to an + // event that has just arrived. In this case we should recalculate + // unreadMessages to get an exact number instead of an estimation + // (see https://github.com/quotient-im/libQuotient/wiki/unread_count). + // For the same reason (switching from the estimation to the exact + // number) this branch always emits unreadMessagesChanged() and returns + // UnreadNotifsChange, even if the estimation luckily matched the exact + // result. + if (fullyReadMarker < to) return recalculateUnreadCount(true); - } - // Fully read marker is somewhere beyond the most historical message from - // the arrived batch - add up newly arrived messages to the current counter, - // instead of a complete recalculation. + // At this point the fully read marker is somewhere beyond the "oldest" + // message from the arrived batch - add up newly arrived messages to + // the current counter, instead of a complete recalculation. Q_ASSERT(to <= fullyReadMarker); QElapsedTimer et; @@ -769,7 +770,7 @@ Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) const auto prevFullyReadId = std::exchange(fullyReadUntilEventId, eventId); qCDebug(MESSAGES) << "Fully read marker in" << q->objectName() // - << "moved to" << fullyReadUntilEventId; + << "set to" << fullyReadUntilEventId; emit q->fullyReadMarkerMoved(prevFullyReadId, fullyReadUntilEventId); // TODO: Remove in 0.8 emit q->readMarkerMoved(prevFullyReadId, fullyReadUntilEventId); -- cgit v1.2.3 From 29195491c25bcba50b78a30c779db07913f87d3a Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 11 Nov 2021 13:01:58 +0100 Subject: Update comments around read receipts code --- lib/room.cpp | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index ceb8d111..4d11878f 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -2498,12 +2498,6 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) << totalInserted << "new events; the last event is now" << timeline.back(); - // The first event in the just-added batch (referred to by `from`) - // defines whose read receipt can possibly be promoted any further over - // the same author's events newly arrived. Others will need explicit - // read receipts from the server - or, for the local user, calling - // setLastDisplayedEventId() - to promote their read receipts over - // the new message events. auto* const firstWriter = q->user((*from)->senderId()); setLastReadReceipt(firstWriter, rev_iter_t(from + 1)); if (firstWriter == q->localUser() @@ -2813,15 +2807,14 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) if (newMarker == historyEdge()) qCDebug(EPHEMERAL) << "Event of the read receipt(s) is not " "found; saving them anyway"; + // If the event is not found (most likely, because it's too old and + // hasn't been fetched from the server yet) but there is a previous + // marker for a user, keep the previous marker because read receipts + // are not supposed to move backwards. Otherwise, blindly store + // the event id for this user and update the read marker when/if + // the event is fetched later on. for (const Receipt& r : p.receipts) if (isMember(r.userId)) { - // If the event is not found (most likely, because it's - // too old and hasn't been fetched from the server yet) - // but there is a previous marker for a user, keep - // the previous marker because read receipts are not - // supposed to move backwards. Otherwise, blindly - // store the event id for this user and update the read - // marker when/if the event is fetched later on. d->setLastReadReceipt(user(r.userId), newMarker, { p.evtId, r.timestamp }); } -- cgit v1.2.3 From f2bf3f203965c51824e8681427798f7a09784ce3 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Thu, 11 Nov 2021 22:18:18 +0100 Subject: Make ReceiptEvent constructible from content Makes the Room::P::toJson() code more readable. --- lib/room.cpp | 27 +++++++-------------------- 1 file changed, 7 insertions(+), 20 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 4d11878f..58950aac 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -2813,7 +2813,7 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) // are not supposed to move backwards. Otherwise, blindly store // the event id for this user and update the read marker when/if // the event is fetched later on. - for (const Receipt& r : p.receipts) + for (const auto& r : p.receipts) if (isMember(r.userId)) { d->setLastReadReceipt(user(r.userId), newMarker, { p.evtId, r.timestamp }); @@ -3021,30 +3021,17 @@ QJsonObject Room::Private::toJson() const { QStringLiteral("events"), accountDataEvents } }); } - if (const auto& readReceiptEventId = - lastReadReceipts.value(q->localUser()).eventId; - !readReceiptEventId.isEmpty()) // + if (const auto& readReceipt = q->lastReadReceipt(connection->userId()); + !readReceipt.eventId.isEmpty()) // { - // Okay, that's a mouthful; but basically, it's simply placing an m.read - // event in the 'ephemeral' section of the cached sync payload. - // See also receiptevent.* and m.read example in the spec. - // Only the local user's read receipt is saved - others' are really - // considered ephemeral but this one is useful in understanding where - // the user is in the timeline before any history is loaded. result.insert( QStringLiteral("ephemeral"), QJsonObject { { QStringLiteral("events"), - QJsonArray { QJsonObject { - { TypeKey, ReceiptEvent::matrixTypeId() }, - { ContentKey, - QJsonObject { - { readReceiptEventId, - QJsonObject { - { QStringLiteral("m.read"), - QJsonObject { - { connection->userId(), - QJsonObject {} } } } } } } } } } } }); + QJsonArray { ReceiptEvent({ { readReceipt.eventId, + { { connection->userId(), + readReceipt.timestamp } } } }) + .fullJson() } } }); } QJsonObject unreadNotifObj { { SyncRoomData::UnreadCountKey, -- cgit v1.2.3 From edb63528e6f3048045f70eb6a48412917bdbea0b Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Mon, 15 Nov 2021 21:37:04 +0100 Subject: Bind read receipts to userIds, not to User* values This reduces the surface interacting with the User class that eventually will be split into LocalUser (most part) and RoomMember (a tiny wrapper around the member data in a given room, used almost everywhere in Room where User currently is). Also: dropped a log message when the new receipt is at or behind the old one as it causes a lot of noise in the logs. --- lib/room.cpp | 163 +++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 90 insertions(+), 73 deletions(-) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index 58950aac..ee76a85c 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -120,14 +120,14 @@ public: int notificationCount = 0; members_map_t membersMap; QList usersTyping; - QHash> eventIdReadUsers; + QHash> eventIdReadUsers; QList usersInvited; QList membersLeft; int unreadMessages = 0; bool displayed = false; QString firstDisplayedEventId; QString lastDisplayedEventId; - QHash lastReadReceipts; + QHash lastReadReceipts; QString fullyReadUntilEventId; TagsMap tags; UnorderedMap accountData; @@ -291,7 +291,7 @@ public: */ void dropDuplicateEvents(RoomEvents& events) const; - void setLastReadReceipt(User* u, rev_iter_t newMarker, + bool setLastReadReceipt(const QString& userId, rev_iter_t newMarker, ReadReceipt newReceipt = {}); Changes setFullyReadMarker(const QString &eventId); Changes updateUnreadCount(const rev_iter_t& from, const rev_iter_t& to); @@ -619,60 +619,62 @@ void Room::setJoinState(JoinState state) emit joinStateChanged(oldState, state); } -void Room::Private::setLastReadReceipt(User* u, rev_iter_t newMarker, +bool Room::Private::setLastReadReceipt(const QString& userId, + rev_iter_t newMarker, ReadReceipt newReceipt) { - if (!u) { - Q_ASSERT(u != nullptr); // For Debug builds - qCCritical(MAIN) << "Empty user, skipping read receipt registration"; - return; // For Release builds - } - if (!q->isMember(u->id())) { - qCWarning(EPHEMERAL) - << "Won't record read receipt for non-member" << u->id(); - return; - } - - auto& storedReceipt = lastReadReceipts[u]; - if (newMarker == timeline.crend() && !newReceipt.eventId.isEmpty()) + if (newMarker == historyEdge() && !newReceipt.eventId.isEmpty()) newMarker = q->findInTimeline(newReceipt.eventId); - if (newMarker != timeline.crend()) { - // NB: with reverse iterators, timeline history >= sync edge - if (newMarker >= q->findInTimeline(storedReceipt.eventId)) { - qCDebug(EPHEMERAL) << "The new read receipt for" << u->id() - << "is at or behind the old one, skipping"; - return; - } - + if (newMarker != historyEdge()) { // Try to auto-promote the read marker over the user's own messages // (switch to direct iterators for that). - const auto eagerMarker = find_if(newMarker.base(), timeline.cend(), + const auto eagerMarker = find_if(newMarker.base(), syncEdge(), [=](const TimelineItem& ti) { - return ti->senderId() != u->id(); - }) - - 1; - newReceipt.eventId = (*eagerMarker)->id(); - if (eagerMarker != newMarker.base() - 1) // &*(rIt.base() - 1) === &*rIt - qCDebug(EPHEMERAL) << "Auto-promoted read receipt for" << u->id() + return ti->senderId() != userId; + }); + // eagerMarker is now just after the desired event for newMarker + if (eagerMarker != newMarker.base()) { + qCDebug(EPHEMERAL) << "Auto-promoted read receipt for" << userId << "to" << newReceipt.eventId; - } + newMarker = rev_iter_t(eagerMarker); + } + // Fill newReceipt with the event (and, if needed, timestamp) from + // eagerMarker + newReceipt.eventId = (eagerMarker - 1)->event()->id(); + if (newReceipt.timestamp.isNull()) + newReceipt.timestamp = QDateTime::currentDateTime(); + } + auto& storedReceipt = + lastReadReceipts[userId]; // clazy:exclude=detaching-member + const auto prevEventId = storedReceipt.eventId; + // NB: with reverse iterators, timeline history >= sync edge + if (newMarker >= q->findInTimeline(prevEventId)) + return false; - if (storedReceipt == newReceipt) - return; // Finally make the change - auto& oldEventReadUsers = eventIdReadUsers[storedReceipt.eventId]; - oldEventReadUsers.remove(u); - if (oldEventReadUsers.isEmpty()) - eventIdReadUsers.remove(storedReceipt.eventId); - eventIdReadUsers[newReceipt.eventId].insert(u); - swap(storedReceipt, newReceipt); // Now newReceipt actually stores the old receipt - qCDebug(EPHEMERAL) << "The new read receipt for" << u->id() << "is at" + + auto oldEventReadUsersIt = + eventIdReadUsers.find(prevEventId); // clazy:exclude=detaching-member + if (oldEventReadUsersIt != eventIdReadUsers.end()) { + oldEventReadUsersIt->remove(userId); + if (oldEventReadUsersIt->isEmpty()) + eventIdReadUsers.erase(oldEventReadUsersIt); + } + eventIdReadUsers[newReceipt.eventId].insert(userId); + storedReceipt = move(newReceipt); + qCDebug(EPHEMERAL) << "The new read receipt for" << userId << "is now at" << storedReceipt.eventId; - emit q->lastReadEventChanged(u); + + // TODO: use Room::member() when it becomes a thing and only emit signals + // for actual members, not just any user + const auto member = q->user(userId); + Q_ASSERT(member != nullptr); + emit q->lastReadEventChanged(member); // TODO: remove in 0.8 - if (!isLocalUser(u)) - emit q->readMarkerForUserMoved(u, newReceipt.eventId, + if (!isLocalUser(member)) + emit q->readMarkerForUserMoved(member, prevEventId, storedReceipt.eventId); + return true; } Room::Changes Room::Private::updateUnreadCount(const rev_iter_t& from, @@ -777,7 +779,7 @@ Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) Changes changes = ReadMarkerChange; if (const auto rm = q->fullyReadMarker(); rm != historyEdge()) { // Pull read receipt if it's behind - setLastReadReceipt(q->localUser(), rm); + setLastReadReceipt(connection->userId(), rm); changes |= recalculateUnreadCount(); // TODO: updateUnreadCount()? } return changes; @@ -785,8 +787,13 @@ Room::Changes Room::Private::setFullyReadMarker(const QString& eventId) void Room::setReadReceipt(const QString& atEventId) { - d->setLastReadReceipt(localUser(), historyEdge(), - { atEventId, QDateTime::currentDateTime() }); + if (!d->setLastReadReceipt(localUser()->id(), historyEdge(), + { atEventId, QDateTime::currentDateTime() })) { + qCDebug(EPHEMERAL) << "The new read receipt for" << localUser()->id() + << "in" << objectName() + << "is at or behind the old one, skipping"; + return; + } connection()->callApi(BackgroundRequest, id(), QStringLiteral("m.read"), QUrl::toPercentEncoding(atEventId)); @@ -1004,7 +1011,7 @@ QString Room::readMarkerEventId() const { return lastFullyReadEventId(); } ReadReceipt Room::lastReadReceipt(const QString& userId) const { - return d->lastReadReceipts.value(user(userId)); + return d->lastReadReceipts.value(userId); } QString Room::lastFullyReadEventId() const { return d->fullyReadUntilEventId; } @@ -1014,11 +1021,21 @@ Room::rev_iter_t Room::fullyReadMarker() const return findInTimeline(d->fullyReadUntilEventId); } -QSet Room::usersAtEventId(const QString& eventId) +QSet Room::userIdsAtEvent(const QString& eventId) { return d->eventIdReadUsers.value(eventId); } +QSet Room::usersAtEventId(const QString& eventId) +{ + const auto& userIds = d->eventIdReadUsers.value(eventId); + QSet users; + users.reserve(userIds.size()); + for (const auto& uId : userIds) + users.insert(user(uId)); + return users; +} + int Room::notificationCount() const { return d->notificationCount; } void Room::resetNotificationCount() @@ -2498,17 +2515,16 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) << totalInserted << "new events; the last event is now" << timeline.back(); - auto* const firstWriter = q->user((*from)->senderId()); - setLastReadReceipt(firstWriter, rev_iter_t(from + 1)); - if (firstWriter == q->localUser() + const auto& firstWriterId = (*from)->senderId(); + setLastReadReceipt(firstWriterId, rev_iter_t(from + 1)); + // If the local user's message(s) is/are first in the batch + // and the fully read marker was right before it, promote + // the fully read marker to the same event as the read receipt. + if (firstWriterId == connection->userId() && q->fullyReadMarker().base() == from) // - { - // If the local user's message(s) is/are first in the batch - // and the fully read marker was right before it, promote - // the fully read marker to the same event as the read receipt. roomChanges |= - setFullyReadMarker(lastReadReceipts.value(firstWriter).eventId); - } + setFullyReadMarker(q->lastReadReceipt(firstWriterId).eventId); + roomChanges |= updateUnreadCount(timeline.crbegin(), rev_iter_t(from)); } @@ -2781,6 +2797,7 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) et.start(); if (auto* evt = eventCast(event)) { d->usersTyping.clear(); + d->usersTyping.reserve(evt->users().size()); // Assume all are members for (const auto& userId : evt->users()) if (isMember(userId)) d->usersTyping.append(user(userId)); @@ -2795,29 +2812,29 @@ Room::Changes Room::processEphemeralEvent(EventPtr&& event) const auto& eventsWithReceipts = evt->eventsWithReceipts(); for (const auto& p : eventsWithReceipts) { totalReceipts += p.receipts.size(); - { - if (p.receipts.size() == 1) - qCDebug(EPHEMERAL) << "Marking" << p.evtId << "as read for" - << p.receipts[0].userId; - else - qCDebug(EPHEMERAL) << "Marking" << p.evtId << "as read for" - << p.receipts.size() << "users"; - } const auto newMarker = findInTimeline(p.evtId); if (newMarker == historyEdge()) qCDebug(EPHEMERAL) << "Event of the read receipt(s) is not " - "found; saving them anyway"; + "found; saving anyway"; // If the event is not found (most likely, because it's too old and // hasn't been fetched from the server yet) but there is a previous // marker for a user, keep the previous marker because read receipts // are not supposed to move backwards. Otherwise, blindly store // the event id for this user and update the read marker when/if // the event is fetched later on. - for (const auto& r : p.receipts) - if (isMember(r.userId)) { - d->setLastReadReceipt(user(r.userId), newMarker, - { p.evtId, r.timestamp }); - } + const auto updatedCount = std::count_if( + p.receipts.cbegin(), p.receipts.cend(), + [this, &newMarker, &evtId = p.evtId](const auto& r) -> bool { + return d->setLastReadReceipt(r.userId, newMarker, + { evtId, r.timestamp }); + }); + + if (p.receipts.size() > 1) + qCDebug(EPHEMERAL) << p.evtId << "marked as read for" + << updatedCount << "user(s)"; + if (updatedCount < p.receipts.size()) + qCDebug(EPHEMERAL) << p.receipts.size() - updatedCount + << "receipts were skipped"; } if (eventsWithReceipts.size() > 3 || totalReceipts > 10 || et.nsecsElapsed() >= profilerMinNsecs()) -- cgit v1.2.3 From b472fc355dc5ce70391ca2b9bc8da35b973ae3a3 Mon Sep 17 00:00:00 2001 From: Alexey Rusakov Date: Mon, 8 Nov 2021 21:03:37 +0100 Subject: Room: lastLocalReadReceipt(), localReadReceiptMarker() To simplify retrieval of the local m.read receipt and the marker for it. --- lib/room.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'lib/room.cpp') diff --git a/lib/room.cpp b/lib/room.cpp index ee76a85c..273a5753 100644 --- a/lib/room.cpp +++ b/lib/room.cpp @@ -1014,6 +1014,16 @@ ReadReceipt Room::lastReadReceipt(const QString& userId) const return d->lastReadReceipts.value(userId); } +ReadReceipt Room::lastLocalReadReceipt() const +{ + return d->lastReadReceipts.value(localUser()->id()); +} + +Room::rev_iter_t Room::localReadReceiptMarker() const +{ + return findInTimeline(lastLocalReadReceipt().eventId); +} + QString Room::lastFullyReadEventId() const { return d->fullyReadUntilEventId; } Room::rev_iter_t Room::fullyReadMarker() const -- cgit v1.2.3