Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bnjbvr merged 16 commits into from
Jun 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a5594c1
feat(timeline): Expose method to send galleries in matrix-sdk-ui
Johennes May 28, 2025
5b0b56f
Add changelog
Johennes May 28, 2025
7b6c0f0
Add missing cfg attributes
Johennes May 28, 2025
793d147
Merge branch 'main' into johannes/msc4274-step3
Johennes Jun 2, 2025
2ff4345
Introduce galleries module to prevent excessive use of cfg
Johennes Jun 2, 2025
f9f3f49
Switch to assert_pending
Johennes Jun 2, 2025
84e36c4
Pass duration into assert_let_timeout instead of using sleep
Johennes Jun 2, 2025
c960200
Don't hide galleries in default event filter
Johennes Jun 2, 2025
59b92df
Send a gallery event in integration test to exercise filtering
Johennes Jun 3, 2025
434b896
Enable matrix-sdk-ui/unstable-msc4274 from matrix-sdk-ffi
Johennes Jun 3, 2025
f873500
Revert "Don't hide galleries in default event filter"
Johennes Jun 3, 2025
2b0db6c
Merge branch 'main' into johannes/msc4274-step3
Johennes Jun 3, 2025
80e2014
Revert "Revert "Don't hide galleries in default event filter""
Johennes Jun 3, 2025
8328039
Mock media config requests
Johennes Jun 3, 2025
e9dfb45
Temporarily switch back to filtering out gallery events
Johennes Jun 3, 2025
22f1991
Revert "Temporarily switch back to filtering out gallery events"
Johennes Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions crates/matrix-sdk-ui/src/timeline/controller/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,20 +232,21 @@ pub fn default_event_filter(event: &AnySyncTimelineEvent, room_version: &RoomVer
return false;
}

matches!(
content.msgtype,
match content.msgtype {
MessageType::Audio(_)
| MessageType::Emote(_)
| MessageType::File(_)
| MessageType::Gallery(_)
| MessageType::Image(_)
| MessageType::Location(_)
| MessageType::Notice(_)
| MessageType::ServerNotice(_)
| MessageType::Text(_)
| MessageType::Video(_)
| MessageType::VerificationRequest(_)
)
| MessageType::Emote(_)
| MessageType::File(_)
| MessageType::Image(_)
| MessageType::Location(_)
| MessageType::Notice(_)
| MessageType::ServerNotice(_)
| MessageType::Text(_)
| MessageType::Video(_)
| MessageType::VerificationRequest(_) => true,
#[cfg(feature = "unstable-msc4274")]
MessageType::Gallery(_) => false,
Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@Johennes Johennes Jun 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right! 🤦‍♂️

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

Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is likely not running; investigating.

Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@Johennes Johennes Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Johennes Johennes Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Johennes Johennes Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@bnjbvr bnjbvr Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor Author

@Johennes Johennes Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets hope it actually turns green now. 😅🤞

_ => false,
}
}

AnyMessageLikeEventContent::Sticker(_)
Expand Down
23 changes: 11 additions & 12 deletions crates/matrix-sdk-ui/src/timeline/event_item/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,18 +362,17 @@ impl EventTimelineItem {

match self.content() {
TimelineItemContent::MsgLike(msglike) => match &msglike.kind {
MsgLikeKind::Message(message) => {
matches!(
message.msgtype(),
MessageType::Text(_)
| MessageType::Emote(_)
| MessageType::Audio(_)
| MessageType::File(_)
| MessageType::Gallery(_)
| MessageType::Image(_)
| MessageType::Video(_)
)
}
MsgLikeKind::Message(message) => match message.msgtype() {
MessageType::Text(_)
| MessageType::Emote(_)
| MessageType::Audio(_)
| MessageType::File(_)
| MessageType::Image(_)
| MessageType::Video(_) => true,
#[cfg(feature = "unstable-msc4274")]
MessageType::Gallery(_) => true,
_ => false,
},
MsgLikeKind::Poll(poll) => {
poll.response_data.is_empty() && poll.end_event_timestamp.is_none()
}
Expand Down
Loading