chore(ui): Unify the logic for timeline item insertions

[Hywan]

(Extracted from #3512)

This patch tries to unify the code to insert timeline item at position Start and End. We can clearly have the same code for these TimelineItemPositions. It doesn't make sense to have separate implementations, especially knowing that End had duplicated events check while Start had not.

This code only and strictly code moving, modulo the position handling, but nothing has changed apart from that.

Note: this is going to be useful when (i) event deduplication will be removed, and (ii) when TimelineItemPosition::At will be added!

---

• Address [meta] EventCache storage #3280

[codecov]

Codecov Report

Attention: Patch coverage is 70.21277% with 14 lines in your changes missing coverage. Please review.

Project coverage is 85.06%. Comparing base (75d7d07) to head (979b6b0).
Report is 27 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/event_handler.rs	70.21%	14 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4331   +/-   ##
=======================================
  Coverage   85.05%   85.06%           
=======================================
  Files         275      275           
  Lines       30309    30316    +7     
=======================================
+ Hits        25780    25788    +8     
+ Misses       4529     4528    -1     

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

[bnjbvr]

(spoiler alert: we don't need must_push_front, since inserting at insert_idx = 0 means pushing front…)

[Hywan]

(spoiler alert: turns out, we need it :-D)

Imagine we have insert_index = 0. Should we push front or back? Front!, according to you. But it's gonna break many tests that assume that the first added event, even with position = End, must be pushed back. So by only looking at insert_index, in the case it's zero, we don't know whether we have to push it front or back. Hence the must_push_front that adds a clue.

[Hywan]

Alternatively, we can insert inside the match position. A bit more repetitive code, but it's much much clearer. Let me try that in the last commit.

[bnjbvr]

I still think what I suggested would work, if you both kept only the insert_idx and reverted the order of the final push statements as it were back in the original form:

• in the case of inserting at the front, insert_idx would be 0, and when the list is empty that would result in a push_back; when it's not, it would be a push front
• in the case of inserting at the back, we compute the proper insert_idx anyways.

And I mean, it's OK to change the test expectations when they lead to the same outcome ^^ push_back or push_front on an empty array yields to the same outcome. But OK, the new code works too.

[poljar]

The commit message looks a bit rough grammar-wise as well, perhaps something like:

chore(ui): Unify the logic for timeline item insertions

This patch unifies the logic for inserting timeline items at Start and End positions. Both TimelineItemPositions can share the same implementation, making separate logic unnecessary. Previously, End included a duplicated events check as well, while Start did not, leading to inconsistency.

The changes strictly involve moving and refactoring, with no functional modifications.

[poljar]

Err not that happy that this patch is adding an unreachable call, which we previously avoided. But ok.

[Hywan]

Yeah… it's unfortunate this is needed, but the upper match-arm makes this really unreachable. A larger refactoring would be needed to tackle that I think.

Poljar did approve it, and all feedback from Ben have been addressed