Fix showing edited message for historical messages #714
Conversation
|
CC @redstrate |
|
Kudos, SonarCloud Quality Gate passed! |
TobiasFella
force-pushed
the
tobias/historical-edits
branch
from
0d504df to
604929f
Compare
TobiasFella
force-pushed
the
tobias/historical-edits
branch
from
604929f to
b3795d9
Compare
|
The reason why only a subrange is checked was simple optimisation: redacts and edits always come after the events they change. In case of historical messages it's meant to be the reverse: historical events from |
| return ep->id() == id; | ||
| }); | ||
| if (targetIt != events.end()) | ||
| *targetIt = makeReplaced(**targetIt, *msg); |
There was a problem hiding this comment.
nit: Since we're moving this block, can we use std::exchange. Ditto for the first branch too
|
Thinking further of the whole thing - what this doesn't cover is the case when an edit or a redaction applies to an event that resides in an even older, not yet received batch. I still wonder if #713 should be reported as the homeserver issue, rather. Doesn't mean I'm unwilling to cover up the problem on the client side, given that the issue already emerged. I would certainly add a warning in the logs for that situation, so that at least knowledgeable people would take heed. |
I need to re-read this part of the spec again, but shouldn't that not matter? I would think if we the original message event, you must have the edits that came before it since you shouldn't be able to edit messages before they're sent. Maybe if the client is in a middle of a timeline, and they only see the message and some edits but not future ones. I'm not sure if that's possible in NeoChat/Quotient currently. +1 to reporting this as a homeserver issue though, but we could at least do the best we can client-side. |
There was a problem hiding this comment.
Revisiting this, and having re-read the spec again - I agree that aggregations (and de-aggregations, but that part is out of scope here for now) are hard (c) and both servers and clients have to do their best effort in tackling them. That being said, if we assume that aggregations have to be processed on the client side too then we have to do some extra stuff, namely two things:
- Make sure the library processes "orphaned" child events well (see below).
- Make sure the library processes aggregations in the right order - I'm not quite sure what will happen if, say, an event and its two replacements come in a single historical batch: I think the PR code as it is will display the older replacement instead of the newer one (bearing in mind that
/messages?dir=breturns events in the reverse-chronological order;moveEventsToTimeline()takes care of re-reversing the order butprocessRedactionsAndEdits()apparently doesn't).
| 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. |
There was a problem hiding this comment.
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).
Fixes #713
This is mostly just extracting the code from
Room::Private::addNewMessageEventsin order to be able to reuse it inRoom::Private::addHistoricalMessageEvents. Note that there is one change: the range of events being searched for the replaced event is changed from(events.begin(), it)to(events.begin(), events.end())- Apparently this is required when handling historical messages; I don't know the exact reason, but don't see how it could cause problems.