Skip to content

feat!(timeline): allow sending media as (thread) replies #4852

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.

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
merged 12 commits into from
Apr 2, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions bindings/matrix-sdk-ffi/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,8 @@ pub enum RoomError {
TimelineUnavailable,
#[error("Invalid thumbnail data")]
InvalidThumbnailData,
#[error("Invalid replied to event ID")]
InvalidRepliedToEventId,
#[error("Failed sending attachment")]
FailedSendingAttachment,
}
Expand Down
38 changes: 35 additions & 3 deletions bindings/matrix-sdk-ffi/src/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use futures_util::{pin_mut, StreamExt as _};
use matrix_sdk::{
attachment::{
AttachmentConfig, AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo,
BaseVideoInfo, Thumbnail,
BaseVideoInfo, Reply, Thumbnail,
},
deserialized_responses::{ShieldState as SdkShieldState, ShieldStateCode},
event_cache::RoomPaginationStatus,
Expand Down Expand Up @@ -116,12 +116,31 @@ impl Timeline {
params.formatted_caption.map(Into::into),
);

let reply = if let Some(reply_params) = params.reply_params {
let event_id = EventId::parse(reply_params.event_id)
.map_err(|_| RoomError::InvalidRepliedToEventId)?;
let enforce_thread = if reply_params.enforce_thread {
EnforceThread::Threaded(if reply_params.reply_within_thread {
ReplyWithinThread::Yes
} else {
ReplyWithinThread::No
})
} else {
EnforceThread::MaybeThreaded
};

Some(Reply { event_id, enforce_thread })
} else {
None
};

let attachment_config = AttachmentConfig::new()
.thumbnail(thumbnail)
.info(attachment_info)
.caption(params.caption)
.formatted_caption(formatted_caption)
.mentions(params.mentions.map(Into::into));
.mentions(params.mentions.map(Into::into))
.reply(reply);

let handle = SendAttachmentJoinHandle::new(get_runtime_handle().spawn(async move {
let mut request =
Expand Down Expand Up @@ -201,14 +220,27 @@ pub struct UploadParameters {
caption: Option<String>,
/// Optional HTML-formatted caption, for clients that support it.
formatted_caption: Option<FormattedBody>,
// Optional intentional mentions to be sent with the media.
/// Optional intentional mentions to be sent with the media.
mentions: Option<Mentions>,
/// Optional parameters for sending the media as (threaded) reply.
reply_params: Option<ReplyParameters>,
Comment on lines +225 to +226
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this here to not have to duplicate the send_* methods for the various media types. Maybe we could also use this to merge send_reply and send_thread_reply?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that would make sense (maybe as a followup PR, to not have this one drag for too long?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separate PR makes sense. 👍

/// Should the media be sent with the send queue, or synchronously?
///
/// Watching progress only works with the synchronous method, at the moment.
use_send_queue: bool,
}

#[derive(uniffi::Record)]
pub struct ReplyParameters {
/// The ID of the event to reply to.
event_id: String,
/// Whether to enforce a thread relation.
enforce_thread: bool,
/// If enforcing a threaded relation, whether the message is a reply on a
/// thread.
reply_within_thread: bool,
}

#[matrix_sdk_ffi_macros::export]
impl Timeline {
pub async fn add_listener(&self, listener: Box<dyn TimelineListener>) -> Arc<TaskHandle> {
Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-ui/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ All notable changes to this project will be documented in this file.
[`Timeline::send_reply()`] now takes an event ID rather than a `RepliedToInfo`.
`Timeline::replied_to_info_from_event_id` has been made private in `matrix_sdk`.
([4842](https://github.com/matrix-org/matrix-rust-sdk/pull/4842))
- Allow sending media as (thread) replies. The reply behaviour can be configured
through new fields on [`AttachmentConfig`].
([4852](https://github.com/matrix-org/matrix-rust-sdk/pull/4852))

### Refactor

Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-ui/src/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ impl Timeline {
enforce_thread: EnforceThread,
) -> Result<(), Error> {
let content = self.room().make_reply_event(content, &event_id, enforce_thread).await?;
self.send(content).await?;
self.send(content.into()).await?;
Ok(())
}

Expand Down
23 changes: 19 additions & 4 deletions crates/matrix-sdk-ui/tests/integration/timeline/media.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,19 @@ use assert_matches2::assert_let;
use eyeball_im::VectorDiff;
use futures_util::{FutureExt, StreamExt};
use matrix_sdk::{
assert_let_timeout, attachment::AttachmentConfig, test_utils::mocks::MatrixMockServer,
assert_let_timeout,
attachment::{AttachmentConfig, Reply},
room::reply::EnforceThread,
test_utils::mocks::MatrixMockServer,
};
use matrix_sdk_test::{async_test, event_factory::EventFactory, JoinedRoomBuilder, ALICE};
use matrix_sdk_ui::timeline::{AttachmentSource, EventSendState, RoomExt};
use ruma::{
event_id,
events::room::{message::MessageType, MediaSource},
events::room::{
message::{MessageType, ReplyWithinThread},
MediaSource,
},
room_id,
};
use serde_json::json;
Expand Down Expand Up @@ -67,10 +73,12 @@ async fn test_send_attachment_from_file() {

assert!(items.is_empty());

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

Expand Down Expand Up @@ -99,7 +107,10 @@ async fn test_send_attachment_from_file() {
mock.mock_room_send().ok(event_id!("$media")).mock_once().mount().await;

// Queue sending of an attachment.
let config = AttachmentConfig::new().caption(Some("caption".to_owned()));
let config = AttachmentConfig::new().caption(Some("caption".to_owned())).reply(Some(Reply {
event_id: event_id.to_owned(),
enforce_thread: EnforceThread::Threaded(ReplyWithinThread::No),
}));
timeline.send_attachment(&file_path, mime::TEXT_PLAIN, config).use_send_queue().await.unwrap();

{
Expand All @@ -115,6 +126,10 @@ async fn test_send_attachment_from_file() {
assert_let!(MessageType::File(file) = msg.msgtype());
assert_let!(MediaSource::Plain(uri) = &file.source);
assert!(uri.to_string().contains("localhost"));

// The message should be considered part of the thread.
let aggregated = item.content().as_msglike().unwrap();
assert!(aggregated.is_threaded());
}

// Eventually, the media is updated with the final MXC IDs…
Expand Down
24 changes: 23 additions & 1 deletion crates/matrix-sdk/src/attachment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ use ruma::{
},
Mentions,
},
OwnedTransactionId, TransactionId, UInt,
OwnedEventId, OwnedTransactionId, TransactionId, UInt,
};

use crate::room::reply::EnforceThread;

/// Base metadata about an image.
#[derive(Debug, Clone, Default)]
pub struct BaseImageInfo {
Expand Down Expand Up @@ -179,6 +181,15 @@ impl Thumbnail {
}
}

/// Information needed to reply to an event.
#[derive(Debug)]
pub struct Reply {
/// The event ID of the event to reply to.
pub event_id: OwnedEventId,
/// Whether to enforce a thread relation.
pub enforce_thread: EnforceThread,
}

/// Configuration for sending an attachment.
#[derive(Debug, Default)]
pub struct AttachmentConfig {
Expand All @@ -188,6 +199,7 @@ pub struct AttachmentConfig {
pub(crate) caption: Option<String>,
pub(crate) formatted_caption: Option<FormattedBody>,
pub(crate) mentions: Option<Mentions>,
pub(crate) reply: Option<Reply>,
}

impl AttachmentConfig {
Expand Down Expand Up @@ -262,4 +274,14 @@ impl AttachmentConfig {
self.mentions = mentions;
self
}

/// Set the reply information of the message.
///
/// # Arguments
///
/// * `reply` - The reply information of the message
pub fn reply(mut self, reply: Option<Reply>) -> Self {
self.reply = reply;
self
}
}
9 changes: 8 additions & 1 deletion crates/matrix-sdk/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ use serde_json::Error as JsonError;
use thiserror::Error;
use url::ParseError as UrlParseError;

use crate::{event_cache::EventCacheError, media::MediaError, store_locks::LockStoreError};
use crate::{
event_cache::EventCacheError, media::MediaError, room::reply::ReplyError,
store_locks::LockStoreError,
};

/// Result type of the matrix-sdk.
pub type Result<T, E = Error> = std::result::Result<T, E>;
Expand Down Expand Up @@ -381,6 +384,10 @@ pub enum Error {
/// An error happened during handling of a media subrequest.
#[error(transparent)]
Media(#[from] MediaError),

/// An error happened while attempting to reply to an event.
#[error(transparent)]
ReplyError(#[from] ReplyError),
}

#[rustfmt::skip] // stop rustfmt breaking the `<code>` in docs across multiple lines
Expand Down
48 changes: 30 additions & 18 deletions crates/matrix-sdk/src/room/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ pub use self::{
#[cfg(doc)]
use crate::event_cache::EventCache;
use crate::{
attachment::{AttachmentConfig, AttachmentInfo},
attachment::{AttachmentConfig, AttachmentInfo, Reply},
client::WeakClient,
config::RequestConfig,
error::{BeaconError, WrongRoomState},
Expand Down Expand Up @@ -2142,18 +2142,21 @@ impl Room {
}
}

let content = Self::make_attachment_event(
self.make_attachment_type(
content_type,
filename,
media_source,
config.caption,
config.formatted_caption,
config.info,
thumbnail,
),
mentions,
);
let content = self
.make_attachment_event(
self.make_attachment_type(
content_type,
filename,
media_source,
config.caption,
config.formatted_caption,
config.info,
thumbnail,
),
mentions,
config.reply,
)
.await?;

let mut fut = self.send(content);
if let Some(txn_id) = txn_id {
Expand Down Expand Up @@ -2254,17 +2257,26 @@ impl Room {
}
}

/// Creates the [`RoomMessageEventContent`] based on the message type and
/// mentions.
pub(crate) fn make_attachment_event(
/// Creates the [`RoomMessageEventContent`] based on the message type,
/// mentions and reply information.
pub(crate) async fn make_attachment_event(
&self,
msg_type: MessageType,
mentions: Option<Mentions>,
) -> RoomMessageEventContent {
reply: Option<Reply>,
) -> Result<RoomMessageEventContent> {
let mut content = RoomMessageEventContent::new(msg_type);
if let Some(mentions) = mentions {
content = content.add_mentions(mentions);
}
content
if let Some(reply) = reply {
// Since we just created the event, there is no relation attached to it. Thus,
// it is safe to add the reply relation without overriding anything.
content = self
.make_reply_event(content.into(), &reply.event_id, reply.enforce_thread)
.await?;
}
Ok(content)
}

/// Update the power levels of a select set of users of this room.
Expand Down
Loading
Loading