feat(timeline): Expose method to send galleries in matrix-sdk-ui by Johennes · Pull Request #5125 · matrix-org/matrix-rust-sdk

Johennes · 2025-05-28T16:52:45Z

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.

Public API changes documented in changelogs (optional)

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

codecov · 2025-05-28T17:16:42Z

Codecov Report

Attention: Patch coverage is 20.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 85.88%. Comparing base (edabb23) to head (22f1991).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/futures.rs	0.00%	10 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/controller/mod.rs	50.00%	1 Missing ⚠️
...rates/matrix-sdk-ui/src/timeline/event_item/mod.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5125      +/-   ##
==========================================
- Coverage   85.90%   85.88%   -0.02%     
==========================================
  Files         335      335              
  Lines       36511    36522      +11     
==========================================
+ Hits        31364    31367       +3     
- Misses       5147     5155       +8

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

bnjbvr

Nice, thank you! Approving because my comments are mostly nits. Soo, is this the last patch, or maybe we need a bit more to display and send galleries at the FFI layer?

bnjbvr · 2025-06-02T13:42:51Z

+    }
+
+    // Eventually, the media is updated with the final MXC IDs…
+    sleep(Duration::from_secs(2)).await;


Do we need the sleep, considering that below we're using assert_let_timeout! (which can be configured with a higher timeout than the default)?

Interestingly I have to use 3 seconds inside assert_let_timeout to replace the 2 seconds from sleep but this still feels better because the code is shorter.

Oh, looks like the sleepdelay was just increased from 2 to 4 seconds in 8a5d4f0.

bnjbvr · 2025-06-02T13:45:40Z

+                                | MessageType::Video(_)
+                                | MessageType::VerificationRequest(_) => true,
+                                #[cfg(feature = "unstable-msc4274")]
+                                MessageType::Gallery(_) => false,


I think this should be true, so as to display galleries by default. Wouldn't this make the test you've added fail, since it'd filter out the gallery event from rendering into an item? Maybe that's why CI's failing now.

Oh, right! 🤦‍♂️

Have switched it around. It didn't seem to affect the test, however, which is mildly concerning. 🤔

The test is likely not running; investigating.

Hah:

CI didn't run the test because the feature flag isn't ever enabled for the UI crate. I think we could decide that we support galleries by default in the timeline, so we could get rid of it now. Alternatively, we could enable the feature just a bit ahead of time in the FFI layer Cargo.toml, so that it's enabled workspace-wide, and included in testing. I think the latter would be great, since the FFI layer likely will want it anyways.

@stefanceriu and I noticed a bug that local echoes never get filtered out based on content, and this is what's happening here, since the current is sending a gallery. Can you add, in this test or another one, a gallery message received via sync from another user, so we can make sure that the filtering actually works? (and temporarily revert the patch that switched this from false to true, to make sure it does filter? no need to add a specific test for the filtering)

Oh! 🤯

Ok, I hope I got all of this right. The test now does fail for me also locally (with the erroneous false). When I switch back to true, it succeeds.

Does it? Looks like it's the sending of the media gallery that fails, not the filtering out…

Oh, you're right. 🤦‍♂️ Ok, will have to look into what's going on there now. 🤔

Looks like the media config requests need mocking after merging in main. Added those and switched back to false so the CI should now fail timing out waiting for the sent gallery.

Thanks, it seems to be failing as intended now: no item is pushed, while we expected one 🥳 Good job making the test meaningful and winning over CI!

Lets hope it actually turns green now. 😅🤞

Johennes · 2025-06-02T18:02:38Z

Soo, is this the last patch, or maybe we need a bit more to display and send galleries at the FFI layer?

Yes indeed, there's one more PR needed to expose galleries on the FFI layer. I just tried to keep this one here as small as possible because the other one will probably have some lengthy mapping to FFI types again. 🙈

bnjbvr

Requesting changes, because of the latest in-thread comment i've posted, to be super explicit. Let's test this in CI! :)

This reverts commit c960200.

This reverts commit f873500.

This reverts commit e9dfb45.

bnjbvr · 2025-06-03T13:35:07Z

Thanks @Johennes, and great job as usual!

Addendum to #5125 to allow sending galleries from the FFI crate. This is the final PR for galleries (apart from possible enhancements to report the upload status in #5008). This is somewhat lengthy again, apologies. Most of the changes are just wrappers and type mapping, however. So I was hoping that it's relatively straightforward to review in bulk. Happy to try and elaborate on the changes or break them up into smaller commits if that helps, however. --------- Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>

Johennes added 3 commits May 28, 2025 18:50

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

a5594c1

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

Add changelog

5b0b56f

Add missing cfg attributes

7b6c0f0

Johennes marked this pull request as ready for review May 28, 2025 17:21

Johennes requested a review from a team as a code owner May 28, 2025 17:21

Johennes requested review from poljar and removed request for a team May 28, 2025 17:21

bnjbvr requested review from bnjbvr and removed request for poljar June 2, 2025 08:34

bnjbvr approved these changes Jun 2, 2025

View reviewed changes

Johennes added 5 commits June 2, 2025 21:01

Merge branch 'main' into johannes/msc4274-step3

793d147

Introduce galleries module to prevent excessive use of cfg

2ff4345

Switch to assert_pending

f9f3f49

Pass duration into assert_let_timeout instead of using sleep

84e36c4

Don't hide galleries in default event filter

c960200

bnjbvr requested changes Jun 3, 2025

View reviewed changes

Johennes added 3 commits June 3, 2025 12:54

Send a gallery event in integration test to exercise filtering

59b92df

Enable matrix-sdk-ui/unstable-msc4274 from matrix-sdk-ffi

434b896

Revert "Don't hide galleries in default event filter"

f873500

This reverts commit c960200.

Johennes commented Jun 3, 2025

View reviewed changes

Comment thread testing/matrix-sdk-test/Cargo.toml

Johennes added 5 commits June 3, 2025 13:15

Merge branch 'main' into johannes/msc4274-step3

2b0db6c

Revert "Revert "Don't hide galleries in default event filter""

80e2014

This reverts commit f873500.

Mock media config requests

8328039

Temporarily switch back to filtering out gallery events

e9dfb45

Revert "Temporarily switch back to filtering out gallery events"

22f1991

This reverts commit e9dfb45.

Johennes requested a review from bnjbvr June 3, 2025 12:31

bnjbvr approved these changes Jun 3, 2025

View reviewed changes

bnjbvr merged commit ee06965 into matrix-org:main Jun 3, 2025
43 checks passed

Johennes mentioned this pull request Jun 3, 2025

feat(ffi): Expose method for sending galleries #5163

Merged

1 task

bxdxnn mentioned this pull request Apr 8, 2026

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

Open

11 tasks

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

bnjbvr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Johennes commented Jun 2, 2025

Uh oh!

bnjbvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnjbvr commented Jun 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Johennes commented May 28, 2025 •

edited

Loading

codecov Bot commented May 28, 2025 •

edited

Loading

bnjbvr left a comment •

edited

Loading

Johennes Jun 2, 2025 •

edited

Loading