feat(timeline): report progress for media uploads

[Johennes]

This is meant to only contain the timeline changes broken out from #5008 (together with #5453 which this depends on).

• Public API changes documented in changelogs (optional)

Signed-off-by:

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.93%. Comparing base (5ad477a) to head (c23d92d).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	70.58%	5 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5454   +/-   ##
=======================================
  Coverage   88.93%   88.93%           
=======================================
  Files         334      334           
  Lines       92420    92433   +13     
  Branches    92420    92433   +13     
=======================================
+ Hits        82196    82208   +12     
- Misses       6378     6379    +1     
  Partials     3846     3846           

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

[codspeed-hq]

CodSpeed Performance Report

Merging #5454 will improve performances by 21.81%

_{Comparing Johennes:johannes/timeline-media-upload-progress (c23d92d) with main (033c6bd)}

Summary

⚡ 1 improvements
✅ 30 untouched benchmarks

Benchmarks breakdown

	Benchmark	BASE	HEAD	Change
⚡	Restore session [SQLite][clear]	773.2 ms	634.8 ms	+21.81%

[Johennes]

I have addressed all comments from #5008, except for:

I do realize only now that it's a bit weird that message events also get a progress optional value here, despite this option being never set. This suggests that some variant EventSendState::Uploading that we could use for medias only, maybe?
[...]
Based on other comments: we might prefer to have a new variant NotUploadedYet, for medias. The progress field isn't used, for non-media uploads.

This is turning out a little more involved than expected, sadly.

There are places like here when a timeline item is added where I would have to peek into the content and figure out if it's a media event or not. There's also the retry handling here where I only have the transaction ID to figure out if it's a media upload transaction or not. It feels like working in a new enum variant there might introduce quite a bit of complexity.

Alternatively, we could only use NotUploadedYet once the upload actually starts and stick to NotSentYet otherwise. That might make things even more confusing than right now though.

I currently feel like keeping the progress optional in NotSentYet might do the least damage?

[bnjbvr]

We've discussed this with the Rust SDK core team, and we think this is a nice API. We might experiment with a new EventSendState::Uploading state later, but since this is unclear whether it'll be good or not, I won't request it now.

My final comments are mostly cosmetic improvements, so a last round and we should be good to go. Thanks for this PR!

[bnjbvr]

Since this isn't serialized: maybe we could make this enum with one variant a plain struct, instead? MediaUploadProgress or something like that?

[bnjbvr]

Can we precise it's only available for media uploads, at the moment?

[bnjbvr]

Let's let the code breath a bit, and simplify the as_variant! to do as little as possible:

Suggested change

	// If the local echo had an upload progress, retain it.
	let progress = as_variant!(&prev_local_item.send_state,
	EventSendState::NotSentYet { progress } => progress.clone())
	.flatten();
	// If the local echo had an upload progress, retain it.
	let progress = as_variant!(&prev_local_item.send_state, EventSendState::NotSentYet)
	.and_then(|progress| progress.cloned());

[bnjbvr]

Can we precise it's only available for media uploads, please?

[bnjbvr]

ditto, since this is an enum with a single variant, and that's not ser/de, let's use a struct instead