From e2148e46bc990f66d306369802e96cd2e8fcffdb Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Mon, 21 Jul 2025 18:06:31 +0200 Subject: [PATCH] feat!(timeline): infer the reply type automatically when sending an attachment --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 30 +++-- crates/matrix-sdk-ui/CHANGELOG.md | 8 ++ .../src/timeline/controller/mod.rs | 5 + crates/matrix-sdk-ui/src/timeline/futures.rs | 34 +++-- crates/matrix-sdk-ui/src/timeline/mod.rs | 125 +++++++++++------- .../tests/integration/timeline/media.rs | 39 +++--- .../tests/integration/timeline/thread.rs | 5 +- crates/matrix-sdk/src/attachment.rs | 30 ++++- 8 files changed, 177 insertions(+), 99 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 06e2cd192..50a68d2d3 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -20,8 +20,7 @@ use eyeball_im::VectorDiff; use futures_util::pin_mut; use matrix_sdk::{ attachment::{ - AttachmentConfig, AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo, - BaseVideoInfo, Thumbnail, + AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo, BaseVideoInfo, Thumbnail, }, deserialized_responses::{ShieldState as SdkShieldState, ShieldStateCode}, event_cache::RoomPaginationStatus, @@ -35,7 +34,7 @@ use matrix_sdk_common::{ stream::StreamExt, }; use matrix_sdk_ui::timeline::{ - self, AttachmentSource, EventItemOrigin, Profile, TimelineDetails, + self, AttachmentConfig, AttachmentSource, EventItemOrigin, Profile, TimelineDetails, TimelineUniqueId as SdkTimelineUniqueId, }; use mime::Mime; @@ -111,19 +110,26 @@ impl Timeline { let mime_str = mime_type.as_ref().ok_or(RoomError::InvalidAttachmentMimeType)?; let mime_type = mime_str.parse::().map_err(|_| RoomError::InvalidAttachmentMimeType)?; + let replied_to_event_id = params + .in_reply_to + .map(EventId::parse) + .transpose() + .map_err(|_| RoomError::InvalidRepliedToEventId)?; let formatted_caption = formatted_body_from( params.caption.as_deref(), params.formatted_caption.map(Into::into), ); - let attachment_config = AttachmentConfig::new() - .thumbnail(thumbnail) - .info(attachment_info) - .caption(params.caption) - .formatted_caption(formatted_caption) - .mentions(params.mentions.map(Into::into)) - .reply(params.reply_params.map(|p| p.try_into()).transpose()?); + let attachment_config = AttachmentConfig { + info: Some(attachment_info), + thumbnail, + caption: params.caption, + formatted_caption, + mentions: params.mentions.map(Into::into), + replied_to: replied_to_event_id, + ..Default::default() + }; let handle = SendAttachmentJoinHandle::new(get_runtime_handle().spawn(async move { let mut request = @@ -205,8 +211,8 @@ pub struct UploadParameters { formatted_caption: Option, /// Optional intentional mentions to be sent with the media. mentions: Option, - /// Optional parameters for sending the media as (threaded) reply. - reply_params: Option, + /// Optional Event ID to reply to. + in_reply_to: Option, /// Should the media be sent with the send queue, or synchronously? /// /// Watching progress only works with the synchronous method, at the moment. diff --git a/crates/matrix-sdk-ui/CHANGELOG.md b/crates/matrix-sdk-ui/CHANGELOG.md index f40bb5120..141548b0a 100644 --- a/crates/matrix-sdk-ui/CHANGELOG.md +++ b/crates/matrix-sdk-ui/CHANGELOG.md @@ -8,6 +8,14 @@ All notable changes to this project will be documented in this file. ### Features +- [**breaking**] [`Timeline::send_attachment()`] now automatically fills in the thread + relationship, based on the timeline focus. As a result, there's a new + `matrix_sdk_ui::timeline::AttachmentConfig` type in town, that has a simplified optional parameter + `replied_to` of type `OwnedEventId` instead of the `Reply` type and that must be used in place of + `matrix_sdk::attachment::AttachmentConfig`. The proper way to start a thread with a media + attachment is now thus to create a threaded-focused timeline, and then use + `Timeline::send_attachment()`. + ([5427](https://github.com/matrix-org/matrix-rust-sdk/pull/5427)) - [**breaking**] [`Timeline::send_reply()`] now automatically fills in the thread relationship, based on the timeline focus. As a result, it only takes an `OwnedEventId` parameter, instead of the `Reply` type. The proper way to start a thread is now thus to create a threaded-focused diff --git a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs index 98fcede14..9a642dc56 100644 --- a/crates/matrix-sdk-ui/src/timeline/controller/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/controller/mod.rs @@ -591,6 +591,11 @@ impl TimelineController { matches!(&*self.focus, TimelineFocusKind::Live { .. }) } + /// Is this timeline focused on a thread? + pub(super) fn is_threaded(&self) -> bool { + matches!(&*self.focus, TimelineFocusKind::Thread { .. }) + } + pub(super) fn thread_root(&self) -> Option { as_variant!(&*self.focus, TimelineFocusKind::Thread { root_event_id } => root_event_id.clone()) } diff --git a/crates/matrix-sdk-ui/src/timeline/futures.rs b/crates/matrix-sdk-ui/src/timeline/futures.rs index 164de00b5..05cffc736 100644 --- a/crates/matrix-sdk-ui/src/timeline/futures.rs +++ b/crates/matrix-sdk-ui/src/timeline/futures.rs @@ -1,12 +1,13 @@ use std::future::IntoFuture; use eyeball::SharedObservable; -use matrix_sdk::{TransmissionProgress, attachment::AttachmentConfig}; +use matrix_sdk::TransmissionProgress; use matrix_sdk_base::boxed_into_future; use mime::Mime; use tracing::{Instrument as _, Span}; use super::{AttachmentSource, Error, Timeline}; +use crate::timeline::AttachmentConfig; pub struct SendAttachment<'a> { timeline: &'a Timeline, @@ -72,17 +73,32 @@ impl<'a> IntoFuture for SendAttachment<'a> { let fut = async move { let (data, filename) = source.try_into_bytes_and_filename()?; + let reply = timeline.infer_reply(config.replied_to).await; + let sdk_config = matrix_sdk::attachment::AttachmentConfig { + txn_id: config.txn_id, + info: config.info, + thumbnail: config.thumbnail, + caption: config.caption, + formatted_caption: config.formatted_caption, + mentions: config.mentions, + reply, + }; + if use_send_queue { - let send_queue = timeline.room().send_queue(); - let fut = send_queue.send_attachment(filename, mime_type, data, config); - fut.await.map_err(|_| Error::FailedSendingAttachment)?; - } else { - let fut = timeline + timeline .room() - .send_attachment(filename, &mime_type, data, config) + .send_queue() + .send_attachment(filename, mime_type, data, sdk_config) + .await + .map_err(|_| Error::FailedSendingAttachment)?; + } else { + timeline + .room() + .send_attachment(filename, &mime_type, data, sdk_config) .with_send_progress_observable(send_progress) - .store_in_cache(); - fut.await.map_err(|_| Error::FailedSendingAttachment)?; + .store_in_cache() + .await + .map_err(|_| Error::FailedSendingAttachment)?; } Ok(()) diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 0e545c4e6..6f9f5f344 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -25,11 +25,9 @@ use eyeball_im::VectorDiff; use futures::SendGallery; use futures_core::Stream; use imbl::Vector; -#[cfg(feature = "unstable-msc4274")] -use matrix_sdk::attachment::{AttachmentInfo, Thumbnail}; use matrix_sdk::{ Result, - attachment::AttachmentConfig, + attachment::{AttachmentInfo, Thumbnail}, deserialized_responses::TimelineEvent, event_cache::{EventCacheDropHandles, RoomEventCache}, executor::JoinHandle, @@ -43,24 +41,19 @@ use matrix_sdk::{ use mime::Mime; use pinned_events_loader::PinnedEventsRoom; use ruma::{ - EventId, OwnedEventId, UserId, + EventId, OwnedEventId, OwnedTransactionId, UserId, api::client::receipt::create_receipt::v3::ReceiptType, events::{ - AnyMessageLikeEventContent, AnySyncTimelineEvent, + AnyMessageLikeEventContent, AnySyncTimelineEvent, Mentions, poll::unstable_start::{NewUnstablePollStartEventContent, UnstablePollStartEventContent}, receipt::{Receipt, ReceiptThread}, room::{ - message::{ReplyWithinThread, RoomMessageEventContentWithoutRelation}, + message::{FormattedBody, ReplyWithinThread, RoomMessageEventContentWithoutRelation}, pinned_events::RoomPinnedEventsEventContent, }, }, room_version_rules::RoomVersionRules, }; -#[cfg(feature = "unstable-msc4274")] -use ruma::{ - OwnedTransactionId, - events::{Mentions, room::message::FormattedBody}, -}; use subscriber::TimelineWithDropHandle; use thiserror::Error; use tracing::{instrument, trace, warn}; @@ -176,6 +169,22 @@ pub enum DateDividerMode { Monthly, } +/// Configuration for sending an attachment. +/// +/// Like [`matrix_sdk::attachment::AttachmentConfig`], but instead of the +/// `reply` field, there's only a `replied_to` event id; it's the timeline +/// deciding to fill the rest of the reply parameters. +#[derive(Debug, Default)] +pub struct AttachmentConfig { + pub txn_id: Option, + pub info: Option, + pub thumbnail: Option, + pub caption: Option, + pub formatted_caption: Option, + pub mentions: Option, + pub replied_to: Option, +} + impl Timeline { /// Returns the room for this timeline. pub fn room(&self) -> &Room { @@ -290,44 +299,21 @@ impl Timeline { // thread relation ourselves. if let AnyMessageLikeEventContent::RoomMessage(ref room_msg_content) = content && room_msg_content.relates_to.is_none() - && let Some(thread_root) = self.controller.thread_root() + && self.controller.is_threaded() { - // The latest event id is used for the reply-to fallback, for clients which - // don't handle threads. It should be correctly set to the latest - // event in the thread, which the timeline instance might or might - // not know about; in this case, we do a best effort of filling it, and resort - // to using the thread root if we don't know about any event. - // - // Note: we could trigger a back-pagination if the timeline is empty, and wait - // for the results, if the timeline is too often empty. - let latest_event_id = self - .controller - .items() + let reply = self + .infer_reply(None) .await - .iter() - .rev() - .find_map(|item| { - if let TimelineItemKind::Event(event) = item.kind() { - event.event_id().map(ToOwned::to_owned) - } else { - None - } - }) - .unwrap_or(thread_root); - + .expect("a reply will always be set for threaded timelines"); let content = self .room() .make_reply_event( // Note: this `.into()` gets rid of the relation, but we've checked previously // that the `relates_to` field wasn't set. room_msg_content.clone().into(), - Reply { - event_id: latest_event_id, - enforce_thread: EnforceThread::Threaded(ReplyWithinThread::No), - }, + reply, ) .await?; - Ok(self.room().send_queue().send(content.into()).await?) } else { // Otherwise, we send the message as is. @@ -362,19 +348,62 @@ impl Timeline { content: RoomMessageEventContentWithoutRelation, replied_to: OwnedEventId, ) -> Result<(), Error> { - let enforce_thread = if self.controller.thread_root().is_some() { - EnforceThread::Threaded(ReplyWithinThread::Yes) - } else { - EnforceThread::MaybeThreaded - }; - let content = self - .room() - .make_reply_event(content, Reply { event_id: replied_to, enforce_thread }) - .await?; + let reply = self + .infer_reply(Some(replied_to)) + .await + .expect("the reply will always be set because we provided a replied-to event id"); + let content = self.room().make_reply_event(content, reply).await?; self.send(content.into()).await?; Ok(()) } + /// Given a message or media to send, and an optional `replied_to` event, + /// automatically fills the [`Reply`] information based on the current + /// timeline focus. + pub(crate) async fn infer_reply(&self, replied_to: Option) -> Option { + // If there's a replied-to event id, the reply is pretty straightforward, and we + // should only infer the `EnforceThread` based on the current focus. + if let Some(replied_to) = replied_to { + let enforce_thread = if self.controller.is_threaded() { + EnforceThread::Threaded(ReplyWithinThread::Yes) + } else { + EnforceThread::MaybeThreaded + }; + return Some(Reply { event_id: replied_to, enforce_thread }); + } + + let thread_root = self.controller.thread_root()?; + + // The latest event id is used for the reply-to fallback, for clients which + // don't handle threads. It should be correctly set to the latest + // event in the thread, which the timeline instance might or might + // not know about; in this case, we do a best effort of filling it, and resort + // to using the thread root if we don't know about any event. + // + // Note: we could trigger a back-pagination if the timeline is empty, and wait + // for the results, if the timeline is too often empty. + + let latest_event_id = self + .controller + .items() + .await + .iter() + .rev() + .find_map(|item| { + if let TimelineItemKind::Event(event) = item.kind() { + event.event_id().map(ToOwned::to_owned) + } else { + None + } + }) + .unwrap_or(thread_root); + + Some(Reply { + event_id: latest_event_id, + enforce_thread: EnforceThread::Threaded(ReplyWithinThread::No), + }) + } + /// Edit an event given its [`TimelineEventItemId`] and some new content. /// /// Only supports events for which [`EventTimelineItem::is_editable()`] diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/media.rs b/crates/matrix-sdk-ui/tests/integration/timeline/media.rs index c5c43529b..4790ef8b3 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/media.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/media.rs @@ -20,14 +20,11 @@ use eyeball_im::VectorDiff; use futures_util::StreamExt; #[cfg(feature = "unstable-msc4274")] use matrix_sdk::attachment::{AttachmentInfo, BaseFileInfo}; -use matrix_sdk::{ - assert_let_timeout, - attachment::AttachmentConfig, - room::reply::{EnforceThread, Reply}, - test_utils::mocks::MatrixMockServer, -}; +use matrix_sdk::{assert_let_timeout, test_utils::mocks::MatrixMockServer}; use matrix_sdk_test::{ALICE, JoinedRoomBuilder, async_test, event_factory::EventFactory}; -use matrix_sdk_ui::timeline::{AttachmentSource, EventSendState, RoomExt}; +use matrix_sdk_ui::timeline::{ + AttachmentConfig, AttachmentSource, EventSendState, RoomExt, TimelineFocus, +}; #[cfg(feature = "unstable-msc4274")] use matrix_sdk_ui::timeline::{GalleryConfig, GalleryItemInfo}; #[cfg(feature = "unstable-msc4274")] @@ -36,10 +33,7 @@ use ruma::events::room::message::GalleryItemType; use ruma::owned_mxc_uri; use ruma::{ event_id, - events::room::{ - MediaSource, - message::{MessageType, ReplyWithinThread}, - }, + events::room::{MediaSource, message::MessageType}, room_id, }; use serde_json::json; @@ -115,12 +109,19 @@ 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())).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(); + // Queue sending of an attachment in the thread. + let thread_timeline = room + .timeline_builder() + .with_focus(TimelineFocus::Thread { root_event_id: event_id.to_owned() }) + .build() + .await + .unwrap(); + let config = AttachmentConfig { caption: Some("caption".to_owned()), ..Default::default() }; + thread_timeline + .send_attachment(&file_path, mime::TEXT_PLAIN, config) + .use_send_queue() + .await + .unwrap(); { assert_let_timeout!(Some(VectorDiff::PushBack { value: item }) = timeline_stream.next()); @@ -222,7 +223,7 @@ async fn test_send_attachment_from_bytes() { 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 { caption: Some("caption".to_owned()), ..Default::default() }; timeline.send_attachment(source, mime::TEXT_PLAIN, config).use_send_queue().await.unwrap(); { @@ -422,7 +423,7 @@ async fn test_react_to_local_media() { let (_tmp_dir, file_path) = create_temporary_file("test.bin"); // Queue sending of an attachment (no captions). - let config = AttachmentConfig::new(); + let config = AttachmentConfig::default(); timeline.send_attachment(&file_path, mime::TEXT_PLAIN, config).use_send_queue().await.unwrap(); let item_id = { diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/thread.rs b/crates/matrix-sdk-ui/tests/integration/timeline/thread.rs index e0a9dc784..6c00e9150 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/thread.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/thread.rs @@ -868,10 +868,7 @@ async fn test_thread_timeline_can_send_edit() { timeline .send( RoomMessageEventContent::text_plain("bonjour monde") - .make_replacement( - ReplacementMetadata::new(threaded_event_id.to_owned(), None), - None, - ) + .make_replacement(ReplacementMetadata::new(threaded_event_id.to_owned(), None)) .into(), ) .await diff --git a/crates/matrix-sdk/src/attachment.rs b/crates/matrix-sdk/src/attachment.rs index c8c405c64..20712b5d5 100644 --- a/crates/matrix-sdk/src/attachment.rs +++ b/crates/matrix-sdk/src/attachment.rs @@ -184,13 +184,29 @@ impl Thumbnail { /// Configuration for sending an attachment. #[derive(Debug, Default)] pub struct AttachmentConfig { - pub(crate) txn_id: Option, - pub(crate) info: Option, - pub(crate) thumbnail: Option, - pub(crate) caption: Option, - pub(crate) formatted_caption: Option, - pub(crate) mentions: Option, - pub(crate) reply: Option, + /// A fixed transaction id to be used for sending this attachment. + /// + /// Otherwise, a random one will be generated. + pub txn_id: Option, + + /// Type-specific metadata about the attachment. + pub info: Option, + + /// An optional thumbnail to send with the attachment. + pub thumbnail: Option, + + /// An optional caption for the attachment. + pub caption: Option, + + /// An optional formatted caption for the attachment. + pub formatted_caption: Option, + + /// Intentional mentions to be included in the media event. + pub mentions: Option, + + /// Reply parameters for the attachment (replied-to event and thread-related + /// metadata). + pub reply: Option, } impl AttachmentConfig {