refactor(timeline): finish the aggregations refactoring

[bnjbvr]

This uses the Aggregations manager to take care of redactions and edits, allowing us to get rid of the pending_edits in the TimelineMetadata \o/ Aggregations are now handled in a single manner, which is nice for consistency.

For redactions, this means we now stash a redaction even if we've seen it before the event it redacts, making sure we won't display something that should've been redacted.

For edits, we now do "the right thing", and will look at the position of each edit event we know about, and use the most "recent" one, if we've found any. This will help handling edits within threads as well (as such, helps with #4869), instead of having us implement some more complicated logic for those.

Part of #4823.

[bnjbvr]

Now rebased and ready for review! Even though there are many, the changes ought to be digestible in a commit per commit read-through.

[codecov]

Codecov Report

Attention: Patch coverage is 84.30233% with 54 lines in your changes missing coverage. Please review.

Project coverage is 85.94%. Comparing base (e3bcd4d) to head (2adfa00).
Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
...rix-sdk-ui/src/timeline/controller/aggregations.rs	82.60%	28 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	88.97%	14 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	65.62%	11 Missing ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5046      +/-   ##
==========================================
+ Coverage   85.85%   85.94%   +0.09%     
==========================================
  Files         325      325              
  Lines       35937    35941       +4     
==========================================
+ Hits        30852    30889      +37     
+ Misses       5085     5052      -33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Hywan]

It's a big PR with many changes. I've spotted a couple of optimisations. Approving ahead of time to avoid another round a review. The main purpose of this patch is reached I think. Good job!

[Hywan]

I suspect it is possible to rewrite this part without the clone.

Right now, the code does the following:

• clone all reactions
• find a specific reaction or create it if it doesn't exist, insert a new sender for this reaction
• if this reaction for this sender was present, previous_reaction equals Some(…)
• we check the condition to compute is_same
• if is_same is true, we return LeftItemIntact, otherwise we get the reactions as a mutable, and update it.

The proposal is the following:

• get the reactions as a mutable directly
• check if the specific reaction exists with entry without calling insert
• if it returns None, we can early return with LeftItemIntact, otherwise compute is_same and then insert if needed

I believe that get (or entry) + insert later is cheaper than a clone here. Something like this:

let Some(mut reactions) = event.content_mut().reactions_mut() else { … }

let reaction_info = ReactionInfo { timestamp: *timestamp, status: reaction_status.clone() };

match reactions.entry(key.clone()) {
    Entry::Vacant(entry) => {
        entry.insert(sender.clone(), reaction_info);
        ApplyAggregationResult::UpdatedItem
    }
    Entry::Occupied(entry) => {
        let mut prev = entry.get_mut();
        let is_same = prev.timestmp == *timestamp && matches!(…);
        if is_same {
            ApplyAggregationResult::LeftItemIntact
        } else {
            prev.insert(sender.clone(), reaction_info);
            ApplyAggregationResult::UpdatedItem
        }
    }
}

Something like this. Thoughts?

[bnjbvr]

It's possible to do this, but there's one change in the code excerpt you posted that would be needed:

let Some(mut reactions) = event.to_mut().content_mut().reactions_mut() else { … }

This to_mut() eagerly marks the Cow as written to, so eagerly clones the underlying content if it was borrowed. In that case, we might be cloning even though the final item isn't actually mutated. So we'd need to do another dance:

• on the read-only version of reaction, check first if there's a similar reaction
• if not, do the mutable transition and add it.

I've done that locally; it's a bit more code, but it's OK and should avoid a few useless clones in some edge cases.

[Hywan]

Has it been addressed?

[bnjbvr]

It can be done, I'll open a followup PR to keep changes tidy.

[Hywan]

Should we rename replies to remote_replies? Not convinced, just asking.

[bnjbvr]

It's unclear in remote_replies which side is remote; in this case, it's both the original and the reply which must have been remote-echoed, and I think this information is more clearly described from the code comments + types used (only OwnedEventId, not TimelineEventItemId anywhere). If you don't mind too much, I'll keep this name.