-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix showing edited message for historical messages #714
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -361,6 +361,8 @@ class Q_DECL_HIDDEN Room::Private { | |
|
||
bool isLocalUser(const User* u) const { return u == q->localUser(); } | ||
|
||
void processRedactionsAndEdits(RoomEvents& events); | ||
|
||
#ifdef Quotient_E2EE_ENABLED | ||
UnorderedMap<QByteArray, QOlmInboundGroupSession> groupSessions; | ||
Omittable<QOlmOutboundGroupSession> currentOutboundMegolmSession = none; | ||
|
@@ -2878,6 +2880,51 @@ inline bool isEditing(const RoomEventPtr& ep) | |
false); | ||
} | ||
|
||
void Room::Private::processRedactionsAndEdits(RoomEvents& events) | ||
{ | ||
// Pre-process redactions and edits so that events that get | ||
// redacted/replaced in the same batch landed in the timeline already | ||
// treated. | ||
// NB: We have to store redacting/replacing events to the timeline too - | ||
// see #220. | ||
auto it = std::find_if(events.begin(), events.end(), isEditing); | ||
for (const auto& eptr : RoomEventsRange(it, events.end())) { | ||
if (auto* r = eventCast<RedactionEvent>(eptr)) { | ||
// Try to find the target in the timeline, then in the batch. | ||
if (processRedaction(*r)) | ||
continue; | ||
if (auto targetIt = std::find_if(events.begin(), events.end(), | ||
[id = r->redactedEvent()](const RoomEventPtr& ep) { | ||
return ep->id() == id; | ||
}); targetIt != events.end()) | ||
*targetIt = makeRedacted(**targetIt, *r); | ||
else | ||
qCDebug(STATE) | ||
<< "Redaction" << r->id() << "ignored: target event" | ||
<< r->redactedEvent() << "is not found"; | ||
// If the target event comes later, it comes already redacted. | ||
} | ||
if (auto* msg = eventCast<RoomMessageEvent>(eptr); | ||
msg && !msg->replacedEvent().isEmpty()) { | ||
if (processReplacement(*msg)) | ||
continue; | ||
auto targetIt = std::find_if(events.begin(), events.end(), | ||
[id = msg->replacedEvent()](const RoomEventPtr& ep) { | ||
return ep->id() == id; | ||
}); | ||
if (targetIt != events.end()) | ||
*targetIt = makeReplaced(**targetIt, *msg); | ||
else // FIXME: hide the replacing event when target arrives later | ||
qCDebug(EVENTS) | ||
<< "Replacing event" << msg->id() | ||
<< "ignored: target event" << msg->replacedEvent() | ||
<< "is not found"; | ||
// Same as with redactions above, the replaced event coming | ||
// later will come already with the new content. | ||
Comment on lines
+2917
to
+2923
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is applicable to events coming with sync but not with history. In case of historical events, I guess we should keep a map of "orphaned" redactions and replacements and resolve them as additional batches arrive. In absence of the parent event, historical redactions are probably a no-op while historical replacements should be shown as they are (and we can even mark them as edited). |
||
} | ||
} | ||
} | ||
|
||
Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) | ||
{ | ||
dropExtraneousEvents(events); | ||
|
@@ -2889,49 +2936,7 @@ Room::Changes Room::Private::addNewMessageEvents(RoomEvents&& events) | |
QElapsedTimer et; | ||
et.start(); | ||
|
||
{ | ||
// Pre-process redactions and edits so that events that get | ||
// redacted/replaced in the same batch landed in the timeline already | ||
// treated. | ||
// NB: We have to store redacting/replacing events to the timeline too - | ||
// see #220. | ||
auto it = std::find_if(events.begin(), events.end(), isEditing); | ||
for (const auto& eptr : RoomEventsRange(it, events.end())) { | ||
if (auto* r = eventCast<RedactionEvent>(eptr)) { | ||
// Try to find the target in the timeline, then in the batch. | ||
if (processRedaction(*r)) | ||
continue; | ||
if (auto targetIt = std::find_if(events.begin(), events.end(), | ||
[id = r->redactedEvent()](const RoomEventPtr& ep) { | ||
return ep->id() == id; | ||
}); targetIt != events.end()) | ||
*targetIt = makeRedacted(**targetIt, *r); | ||
else | ||
qCDebug(STATE) | ||
<< "Redaction" << r->id() << "ignored: target event" | ||
<< r->redactedEvent() << "is not found"; | ||
// If the target event comes later, it comes already redacted. | ||
} | ||
if (auto* msg = eventCast<RoomMessageEvent>(eptr); | ||
msg && !msg->replacedEvent().isEmpty()) { | ||
if (processReplacement(*msg)) | ||
continue; | ||
auto targetIt = std::find_if(events.begin(), it, | ||
[id = msg->replacedEvent()](const RoomEventPtr& ep) { | ||
return ep->id() == id; | ||
}); | ||
if (targetIt != it) | ||
*targetIt = makeReplaced(**targetIt, *msg); | ||
else // FIXME: hide the replacing event when target arrives later | ||
qCDebug(EVENTS) | ||
<< "Replacing event" << msg->id() | ||
<< "ignored: target event" << msg->replacedEvent() | ||
<< "is not found"; | ||
// Same as with redactions above, the replaced event coming | ||
// later will come already with the new content. | ||
} | ||
} | ||
} | ||
processRedactionsAndEdits(events); | ||
|
||
// State changes arrive as a part of timeline; the current room state gets | ||
// updated before merging events to the timeline because that's what | ||
|
@@ -3034,6 +3039,8 @@ void Room::Private::addHistoricalMessageEvents(RoomEvents&& events) | |
|
||
decryptIncomingEvents(events); | ||
|
||
processRedactionsAndEdits(events); | ||
|
||
QElapsedTimer et; | ||
et.start(); | ||
Changes changes {}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Since we're moving this block, can we use std::exchange. Ditto for the first branch too