Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
3 changes: 3 additions & 0 deletions crates/matrix-sdk-ui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ All notable changes to this project will be documented in this file.
([#5055](https://github.com/matrix-org/matrix-rust-sdk/pull/5055))
- `Timeline::mark_as_read()` unsets the unread flag of the room if it was set.
([#5055](https://github.com/matrix-org/matrix-rust-sdk/pull/5055))
- Add new method `Timeline::send_gallery` to allow sending MSC4274-style
galleries.
([#5125](https://github.com/matrix-org/matrix-rust-sdk/pull/5125))

## [0.11.0] - 2025-04-11

Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-ui/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ uniffi = ["dep:uniffi", "matrix-sdk/uniffi", "matrix-sdk-base/uniffi"]
# Add support for encrypted extensible events.
unstable-msc3956 = ["ruma/unstable-msc3956"]

# Add support for inline media galleries via msgtypes
unstable-msc4274 = ["matrix-sdk/unstable-msc4274"]

[dependencies]
as_variant.workspace = true
async-rx.workspace = true
Expand Down
26 changes: 14 additions & 12 deletions crates/matrix-sdk-ui/src/timeline/controller/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,19 +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::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
Member

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

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
Member

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
Member

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

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
Member

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

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

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
Member

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

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
22 changes: 11 additions & 11 deletions crates/matrix-sdk-ui/src/timeline/event_item/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -362,17 +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::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
36 changes: 36 additions & 0 deletions crates/matrix-sdk-ui/src/timeline/futures.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::future::IntoFuture;

use eyeball::SharedObservable;
#[cfg(feature = "unstable-msc4274")]
use matrix_sdk::attachment::GalleryConfig;
use matrix_sdk::{attachment::AttachmentConfig, TransmissionProgress};
use matrix_sdk_base::boxed_into_future;
use mime::Mime;
Expand Down Expand Up @@ -92,3 +94,37 @@ impl<'a> IntoFuture for SendAttachment<'a> {
Box::pin(fut.instrument(tracing_span))
}
}

#[cfg(feature = "unstable-msc4274")]
Comment thread
Johennes marked this conversation as resolved.
pub struct SendGallery<'a> {
timeline: &'a Timeline,
gallery: GalleryConfig,
tracing_span: Span,
}

#[cfg(feature = "unstable-msc4274")]
impl<'a> SendGallery<'a> {
pub(crate) fn new(timeline: &'a Timeline, gallery: GalleryConfig) -> Self {
Self { timeline, gallery, tracing_span: Span::current() }
}
}

#[cfg(feature = "unstable-msc4274")]
impl<'a> IntoFuture for SendGallery<'a> {
type Output = Result<(), Error>;
boxed_into_future!(extra_bounds: 'a);

fn into_future(self) -> Self::IntoFuture {
let Self { timeline, gallery, tracing_span } = self;

let fut = async move {
let send_queue = timeline.room().send_queue();
let fut = send_queue.send_gallery(gallery);
fut.await.map_err(|_| Error::FailedSendingAttachment)?;

Ok(())
};

Box::pin(fut.instrument(tracing_span))
}
}
26 changes: 26 additions & 0 deletions crates/matrix-sdk-ui/src/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ use std::{fs, path::PathBuf, sync::Arc};
use algorithms::rfind_event_by_item_id;
use event_item::TimelineItemHandle;
use eyeball_im::VectorDiff;
#[cfg(feature = "unstable-msc4274")]
use futures::SendGallery;
use futures_core::Stream;
use imbl::Vector;
#[cfg(feature = "unstable-msc4274")]
use matrix_sdk::attachment::GalleryConfig;
use matrix_sdk::{
attachment::AttachmentConfig,
deserialized_responses::TimelineEvent,
Expand Down Expand Up @@ -416,6 +420,28 @@ impl Timeline {
SendAttachment::new(self, source.into(), mime_type, config)
}

/// Sends a media gallery to the room.
///
/// If the encryption feature is enabled, this method will transparently
/// encrypt the room message if the room is encrypted.
///
/// The attachments and their optional thumbnails are stored in the media
/// cache and can be retrieved at any time, by calling
/// [`Media::get_media_content()`] with the `MediaSource` that can be found
/// in the corresponding `TimelineEventItem`, and using a
/// `MediaFormat::File`.
///
/// # Arguments
/// * `gallery` - A configuration object containing details about the
/// gallery like files, thumbnails, etc.
///
/// [`Media::get_media_content()`]: matrix_sdk::Media::get_media_content
#[cfg(feature = "unstable-msc4274")]
#[instrument(skip_all)]
pub fn send_gallery(&self, gallery: GalleryConfig) -> SendGallery<'_> {
SendGallery::new(self, gallery)
}

/// Redact an event given its [`TimelineEventItemId`] and an optional
/// reason.
pub async fn redact(
Expand Down
126 changes: 126 additions & 0 deletions crates/matrix-sdk-ui/tests/integration/timeline/media.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ use assert_matches::assert_matches;
use assert_matches2::assert_let;
use eyeball_im::VectorDiff;
use futures_util::{FutureExt, StreamExt};
#[cfg(feature = "unstable-msc4274")]
use matrix_sdk::attachment::{AttachmentInfo, BaseFileInfo, GalleryConfig, GalleryItemInfo};
use matrix_sdk::{
assert_let_timeout,
attachment::AttachmentConfig,
Expand All @@ -26,6 +28,8 @@ use matrix_sdk::{
};
use matrix_sdk_test::{async_test, event_factory::EventFactory, JoinedRoomBuilder, ALICE};
use matrix_sdk_ui::timeline::{AttachmentSource, EventSendState, RoomExt};
#[cfg(feature = "unstable-msc4274")]
use ruma::events::room::message::GalleryItemType;
use ruma::{
event_id,
events::room::{
Expand Down Expand Up @@ -262,6 +266,128 @@ async fn test_send_attachment_from_bytes() {
assert!(timeline_stream.next().now_or_never().is_none());
}

#[cfg(feature = "unstable-msc4274")]
#[async_test]
async fn test_send_gallery_from_bytes() {
let mock = MatrixMockServer::new().await;
let client = mock.client_builder().build().await;

mock.mock_room_state_encryption().plain().mount().await;

let room_id = room_id!("!a98sd12bjh:example.org");
let room = mock.sync_joined_room(&client, room_id).await;
let timeline = room.timeline().await.unwrap();

let (items, mut timeline_stream) =
timeline.subscribe_filter_map(|item| item.as_event().cloned()).await;

assert!(items.is_empty());

let f = EventFactory::new();
mock.sync_room(
&client,
JoinedRoomBuilder::new(room_id).add_timeline_event(f.text_msg("hello").sender(&ALICE)),
)
.await;

// Sanity check.
assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next());
assert_let!(Some(msg) = item.content().as_message());
assert_eq!(msg.body(), "hello");

// No other updates.
assert!(timeline_stream.next().now_or_never().is_none());
Comment thread
Johennes marked this conversation as resolved.
Outdated

// The data of the file.
let filename = "test.bin";
let data = b"hello world".to_vec();

// Set up mocks for the file upload.
mock.mock_upload()
.respond_with(ResponseTemplate::new(200).set_delay(Duration::from_secs(2)).set_body_json(
json!({
"content_uri": "mxc://sdk.rs/media"
}),
))
.mock_once()
.mount()
.await;

mock.mock_room_send().ok(event_id!("$media")).mock_once().mount().await;

// Queue sending of a gallery.
let gallery =
GalleryConfig::new().caption(Some("caption".to_owned())).add_item(GalleryItemInfo {
filename: filename.to_owned(),
content_type: mime::TEXT_PLAIN,
data,
attachment_info: AttachmentInfo::File(BaseFileInfo { size: None }),
caption: Some("item caption".to_owned()),
formatted_caption: None,
thumbnail: None,
});
timeline.send_gallery(gallery).await.unwrap();

{
assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next());
assert_matches!(item.send_state(), Some(EventSendState::NotSentYet));
assert_let!(Some(msg) = item.content().as_message());

// Body matches gallery caption.
assert_eq!(msg.body(), "caption");

// Message is gallery of expected length
assert_let!(MessageType::Gallery(content) = msg.msgtype());
assert_eq!(1, content.itemtypes.len());
assert_let!(GalleryItemType::File(file) = content.itemtypes.first().unwrap());

// Item has filename and caption
assert_eq!(filename, file.filename());
assert_eq!(Some("item caption"), file.caption());

// The URI refers to the local cache.
assert_let!(MediaSource::Plain(uri) = &file.source);
assert!(uri.to_string().contains("localhost"));
}

// Eventually, the media is updated with the final MXC IDs…
sleep(Duration::from_secs(2)).await;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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


{
assert_let_timeout!(
Some(VectorDiff::Set { index: 1, value: item }) = timeline_stream.next()
);
assert_let!(Some(msg) = item.content().as_message());
assert_matches!(item.send_state(), Some(EventSendState::NotSentYet));

// Message is gallery of expected length
assert_let!(MessageType::Gallery(content) = msg.msgtype());
assert_eq!(1, content.itemtypes.len());
assert_let!(GalleryItemType::File(file) = content.itemtypes.first().unwrap());

// Item has filename and caption
assert_eq!(filename, file.filename());
assert_eq!(Some("item caption"), file.caption());

// The URI now refers to the final MXC URI.
assert_let!(MediaSource::Plain(uri) = &file.source);
assert_eq!(uri.to_string(), "mxc://sdk.rs/media");
}

// And eventually the event itself is sent.
{
assert_let_timeout!(
Some(VectorDiff::Set { index: 1, value: item }) = timeline_stream.next()
);
assert_matches!(item.send_state(), Some(EventSendState::Sent{ event_id }) => {
assert_eq!(event_id, event_id!("$media"));
});
}

// That's all, folks!
assert!(timeline_stream.next().now_or_never().is_none());
}

#[async_test]
async fn test_react_to_local_media() {
let mock = MatrixMockServer::new().await;
Expand Down
Loading