feat(send_queue): Implement sending of MSC4274 galleries by Johennes · Pull Request #4977 · matrix-org/matrix-rust-sdk

Johennes · 2025-04-25T06:45:59Z

This was broken out of #4838 and is a step towards implementing MSC4274.

The entry point for sending galleries via the send queue is a new method RoomSendQueue::send_gallery which is a generalization of RoomSendQueue::send_attachment.
- send_gallery takes as input parameters a GalleryConfig (containing info about the gallery itself, such as its caption) and a vector of GalleryItemInfos (containing info about each image / file / etc. in the gallery).
- send_gallery creates the gallery event content via Room:make_message_event which was renamed from make_attachment_event to reflect the fact that it creates general msgtype events now.
- send_gallery maps the passed item infos into GalleryItemQueueInfos. This additional struct allows grouping all the metadata for a single gallery item together.
Finally send_gallery invokes QueueStorage::push_gallery which is a generalization of QueueStorage::push_media.
- send_gallery pushes upload requests for the media and thumbnails to the queue in a "daisy chain" manner. The first thumbnail (or media if no thumbnail exists) is pushed as a QueuedRequestKind::MediaUpload. The remaining thumbnails and media are pushed as DependentQueuedRequestKind::UploadFileOrThumbnails while chaining each request to the previous one.
- Finally a DependentQueuedRequestKind::FinishGallery is pushed to finalize the gallery upload (analogous to the existing FinishUpload for single media uploads).
The FinishGallery request is handled in QueueStorage::handle_dependent_finish_gallery_upload which was modeled after handle_dependent_finish_upload.
- To be able to map the temporary event source for each gallery item to the final AccumulatedSentMediaInfo, a hash map is used.
- Using the hash map, the gallery event is then updated to use the actual media sources via update_gallery_event_after_upload and a new method Room::make_gallery_item_type
An integration test has been added to demonstrate the functionality.

This is relatively large, unfortunately, but including everything needed to actually send the event made it possible to also add a test for it. It would be nice if the amount of new code could be reduced but I'm struggling a bit to find ways to integrate galleries with the existing media uploads further.

Public API changes documented in changelogs (optional)

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes · 2025-04-25T06:50:49Z

+                let info = assign!(info.map(ImageInfo::from).unwrap_or_default(), {
+                    mimetype: Some(content_type.as_ref().to_owned()),
+                    thumbnail_source,
+                    thumbnail_info
+                });
+                let content = assign!(ImageMessageEventContent::new(body, source), {
+                    info: Some(Box::new(info)),
+                    formatted: formatted_caption,
+                    filename
+                });


These blocks could maybe share the logic with make_attachment_type by extracting a function to create the content for each mime-type? I wasn't sure if it's really worth it because it might not be shorter due to the number of arguments on the call.

Some form of code sharing would be nice, since it's a direct copy-pasto. I think a local macro (defined with macro_rules!) that contains the entire match would be nice here: the only thing that changes is the GalleryItemType vs MessageType, if I'm following correctly?

codecov · 2025-04-25T07:01:36Z

Codecov Report

Attention: Patch coverage is 83.92857% with 27 lines in your changes missing coverage. Please review.

Project coverage is 85.81%. Comparing base (40ffd40) to head (f235e56).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/room/mod.rs	53.65%	19 Missing ⚠️
crates/matrix-sdk/src/send_queue/upload.rs	90.12%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4977      +/-   ##
==========================================
+ Coverage   85.80%   85.81%   +0.01%     
==========================================
  Files         332      332              
  Lines       36195    36211      +16     
==========================================
+ Hits        31056    31076      +20     
+ Misses       5139     5135       -4

☔ 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.

Johennes · 2025-05-19T18:55:34Z

I'm mindful that it's probably just a matter of lack of time but if there's anything I can do to simplify reviewing this PR, I'd be happy to follow any suggestions.

bnjbvr · 2025-05-20T08:29:04Z

Sorry about the long review time. I'll take a look this week, but indeed one large commit with that many changes is a bit overwhelming and hard to review. For PRs that large, in general, in the core team, we tend to do some synchronous video meeting with screen sharing, where the author explains the changes in real-time, giving some kind of "map" of the changes (where to start reading from, where to continue, etc.). If there's any chance you can do any of this in written form, that'd be great, but it is not mandatory :)

One early comment, though:

Room::make_attachment_event was renamed to make_message_event because it is now reused to create gallery events as well

I think this could be named make_media_event (to distinguish from the general use case of text messages, as messages is so general). In retrospect I wonder if it could have been in its own separate commit (although it wouldn't have changed the overall size of the other commit that much :)).

Johennes · 2025-05-20T14:27:04Z

Sorry about the long review time. I'll take a look this week, but indeed one large commit with that many changes is a bit overwhelming and hard to review. For PRs that large, in general, in the core team, we tend to do some synchronous video meeting with screen sharing, where the author explains the changes in real-time, giving some kind of "map" of the changes (where to start reading from, where to continue, etc.). If there's any chance you can do any of this in written form, that'd be great, but it is not mandatory :)

No worries at all. I'm really grateful to get input on this hefty change at all. ❤️

I have expanded the bullets in the PR description to describe the code path in some more detail. I hope this is helpful.

One early comment, though:

Room::make_attachment_event was renamed to make_message_event because it is now reused to create gallery events as well

I think this could be named make_media_event (to distinguish from the general use case of text messages, as messages is so general).

Good call. I have copied your comment into the diff and will address it together with any other feedback you might have later.

In retrospect I wonder if it could have been in its own separate commit (although it wouldn't have changed the overall size of the other commit that much :)).

Yes, you're probably right. I will try to pay more attention to canonicalizing commits in future.

bnjbvr

Looks great! The test is quite comprehensive, that's nice.

A first batch of comments; I think we might be able to reduce the code duplication with the existing send_attachment functionality in various places, so let's try it out. If it turns out to be more trouble (e.g. require doing too much data/parameters shuffling, etc.), in some places, please don't hesitate to not do it.

I'd like to take another look after that, but it looks like this is on the right track. Thanks!

Johennes · 2025-05-23T19:27:35Z

Thanks a ton for the review. 🙏

I've addressed most comment but will have to return to this next week for the refactoring of send_gallery and push_gallery.

Johennes · 2025-05-26T18:25:40Z

Ok, I think I've addressed everything now.

bnjbvr

Thanks, looking great! I didn't expect some of the refactorings to happen that cleanly, that's super nice; thanks for making separate commits for those, that really helped.

What a journey! A few more stylistics comments, and I think we can merge this then 🥳

bnjbvr · 2025-05-27T06:51:36Z

+        match itemtype {
+            GalleryItemType::Audio(event) => match sent.get(&event.source.unique_key()) {


Can we invert the position of the match sent.get(...) and match itemtype? Then, we don't need to repeat it in each match arm.

Hm, I think this doesn't work because event used in the inner match is only set through the outer match? We could maybe add an fn source(&self): Option<MediaSource> on GalleryItemType. That would probably have to go into Ruma though?

Ah you're right; let's defer!

bnjbvr · 2025-05-27T09:54:18Z

Thanks a bunch for getting this through the finish line 🥳 Excited to see it in action soon!

This was broken out of #4838 and is a step towards implementing matrix-org/matrix-spec-proposals#4274. Building upon #4977, a new method `Timeline::send_gallery` in matrix-sdk-ui for sending galleries. - [x] Public API changes documented in changelogs (optional) --------- Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

feat(send_queue): Implement sending of MSC4274 galleries

0c3161b

Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes force-pushed the johannes/msc4274-step2 branch from ba8aa4b to 0c3161b Compare April 25, 2025 06:46

Johennes commented Apr 25, 2025

View reviewed changes

Johennes marked this pull request as ready for review April 25, 2025 07:16

Johennes requested a review from a team as a code owner April 25, 2025 07:16

Johennes requested review from poljar and removed request for a team April 25, 2025 07:16

bnjbvr requested review from bnjbvr and removed request for poljar May 12, 2025 11:38

Johennes commented May 20, 2025

View reviewed changes

Comment thread crates/matrix-sdk/src/room/mod.rs Outdated

bnjbvr requested changes May 22, 2025

View reviewed changes

Johennes added 15 commits May 23, 2025 13:47

Merge branch 'main' into johannes/msc4274-step2

d749ebe

Use an OwnedTransactionId to avoid cloning

9a62767

Add missing trailing periods in comments

58626d2

Move client and cache store outside of loop

932dbf9

Prevent sending empty galleries

c6731ca

Remove superfluous type annotations

47fe934

Pre-allocate capacity for item vectors

49c9576

Extract macro for MessageType / GalleryItemType

421f5bd

Remove superfluous self

b5f4828

Rename make_message_event to make_media_event

7149b73

Bundle items with GalleryConfig

0e58e50

Avoid type annotation and pre-allocate vector

fc4aa7e

Avoid unnecessary type annotation

e47706e

Use let / else to reduce indentation

4028bb4

Relocate and rephrase comment for FinishGalleryItemInfo

3acc0e8

Extract shared function to update cache keys

7e7a38c

Johennes added 5 commits May 26, 2025 20:10

Extract method to push thumbnail & media requests

aaa392c

Move pushing the dependent media request out of the if / else

26601a4

Avoid unwrapping last_upload_file_txn

b5b6723

Extract method for caching

9a491b6

Merge branch 'main' into johannes/msc4274-step2

6b2b3f0

Johennes requested a review from bnjbvr May 26, 2025 18:25

bnjbvr approved these changes May 27, 2025

View reviewed changes

Johennes added 3 commits May 27, 2025 10:46

Rename config parameter to gallery

946e05a

Remove superfluous blank line

662d6c7

Reduce indentation by returning early

f235e56

bnjbvr merged commit 10668f2 into matrix-org:main May 27, 2025
43 checks passed

Johennes mentioned this pull request May 28, 2025

feat(timeline): Expose method to send galleries in matrix-sdk-ui #5125

Merged

1 task

bxdxnn mentioned this pull request Apr 8, 2026

Support gallery messages element-hq/element-x-android#6519

Open

11 tasks

		match itemtype {
		GalleryItemType::Audio(event) => match sent.get(&event.source.unique_key()) {

Conversation

Johennes commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Johennes Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

bnjbvr May 22, 2025

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Johennes commented May 19, 2025

Uh oh!

bnjbvr commented May 20, 2025

Uh oh!

Uh oh!

Johennes commented May 20, 2025

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Johennes commented May 23, 2025

Uh oh!

Johennes commented May 26, 2025

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Johennes May 27, 2025

Choose a reason for hiding this comment

Uh oh!

bnjbvr May 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bnjbvr commented May 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Johennes commented Apr 25, 2025 •

edited

Loading

codecov Bot commented Apr 25, 2025 •

edited

Loading