From f2c569440e8457d9e97ca31cf7c8a53b27e76482 Mon Sep 17 00:00:00 2001 From: Jonas Platte Date: Mon, 25 Sep 2023 15:47:36 +0200 Subject: [PATCH] ui: Remove transaction ID parameter on Timeline methods There is no reason for it to be configurable in the high-level API, since the timeline manages local echoes automatically. --- bindings/matrix-sdk-ffi/src/room.rs | 43 ++++------------ crates/matrix-sdk-ui/src/timeline/mod.rs | 9 ++-- .../tests/integration/room_list_service.rs | 8 +-- .../tests/integration/timeline/echo.rs | 49 ++++++++++--------- .../tests/integration/timeline/queue.rs | 29 ++++++----- .../tests/integration/timeline/replies.rs | 1 - .../src/tests/reactions.rs | 2 +- 7 files changed, 59 insertions(+), 82 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room.rs b/bindings/matrix-sdk-ffi/src/room.rs index 0052ef772..4f44c5af9 100644 --- a/bindings/matrix-sdk-ffi/src/room.rs +++ b/bindings/matrix-sdk-ffi/src/room.rs @@ -419,7 +419,7 @@ impl Room { }) } - pub fn send(&self, msg: Arc, txn_id: Option) { + pub fn send(&self, msg: Arc) { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), None => { @@ -429,12 +429,7 @@ impl Room { }; RUNTIME.spawn(async move { - timeline - .send( - (*msg).to_owned().with_relation(None).into(), - txn_id.as_deref().map(Into::into), - ) - .await; + timeline.send((*msg).to_owned().with_relation(None).into()).await; }); } @@ -444,7 +439,6 @@ impl Room { answers: Vec, max_selections: u8, poll_kind: PollKind, - txn_id: Option, ) -> Result<(), ClientError> { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), @@ -477,7 +471,7 @@ impl Room { AnyMessageLikeEventContent::UnstablePollStart(poll_start_event_content.into()); RUNTIME.spawn(async move { - timeline.send(event_content, txn_id.as_deref().map(Into::into)).await; + timeline.send(event_content).await; }); Ok(()) @@ -487,7 +481,6 @@ impl Room { &self, poll_start_id: String, answers: Vec, - txn_id: Option, ) -> Result<(), ClientError> { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), @@ -504,18 +497,13 @@ impl Room { AnyMessageLikeEventContent::UnstablePollResponse(poll_response_event_content); RUNTIME.spawn(async move { - timeline.send(event_content, txn_id.as_deref().map(Into::into)).await; + timeline.send(event_content).await; }); Ok(()) } - pub fn end_poll( - &self, - poll_start_id: String, - text: String, - txn_id: Option, - ) -> Result<(), ClientError> { + pub fn end_poll(&self, poll_start_id: String, text: String) -> Result<(), ClientError> { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), None => { @@ -529,7 +517,7 @@ impl Room { let event_content = AnyMessageLikeEventContent::UnstablePollEnd(poll_end_event_content); RUNTIME.spawn(async move { - timeline.send(event_content, txn_id.as_deref().map(Into::into)).await; + timeline.send(event_content).await; }); Ok(()) @@ -539,7 +527,6 @@ impl Room { &self, msg: Arc, reply_item: Arc, - txn_id: Option, ) -> Result<(), ClientError> { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), @@ -553,7 +540,6 @@ impl Room { &reply_item.0, ForwardThread::Yes, AddMentions::No, - txn_id.as_deref().map(Into::into), ) .await?; anyhow::Ok(()) @@ -566,7 +552,6 @@ impl Room { &self, new_msg: Arc, original_event_id: String, - txn_id: Option, ) -> Result<(), ClientError> { let timeline = match &*RUNTIME.block_on(self.timeline.read()) { Some(t) => Arc::clone(t), @@ -579,7 +564,7 @@ impl Room { ))); RUNTIME.spawn(async move { - timeline.send(edited_content.into(), txn_id.as_deref().map(Into::into)).await; + timeline.send(edited_content.into()).await; }); Ok(()) } @@ -591,18 +576,11 @@ impl Room { /// * `event_id` - The ID of the event to redact /// /// * `reason` - The reason for the event being redacted (optional). - /// - /// * `txn_id` - A unique ID that can be attached to this event as /// its transaction ID (optional). If not given one is created. - pub fn redact( - &self, - event_id: String, - reason: Option, - txn_id: Option, - ) -> Result<(), ClientError> { + pub fn redact(&self, event_id: String, reason: Option) -> Result<(), ClientError> { RUNTIME.block_on(async move { let event_id = EventId::parse(event_id)?; - self.inner.redact(&event_id, reason.as_deref(), txn_id.map(Into::into)).await?; + self.inner.redact(&event_id, reason.as_deref(), None).await?; Ok(()) }) } @@ -911,7 +889,6 @@ impl Room { description: Option, zoom_level: Option, asset_type: Option, - txn_id: Option, ) { let mut location_event_message_content = LocationMessageEventContent::new(body, geo_uri.clone()); @@ -929,7 +906,7 @@ impl Room { let room_message_event_content = RoomMessageEventContentWithoutRelation::new( MessageType::Location(location_event_message_content), ); - self.send(Arc::new(room_message_event_content), txn_id) + self.send(Arc::new(room_message_event_content)) } pub fn cancel_send(&self, txn_id: String) { diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 2b3234c78..b77de34cf 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -354,8 +354,8 @@ impl Timeline { /// [`MessageLikeUnsigned`]: ruma::events::MessageLikeUnsigned /// [`SyncMessageLikeEvent`]: ruma::events::SyncMessageLikeEvent #[instrument(skip(self, content), fields(room_id = ?self.room().room_id()))] - pub async fn send(&self, content: AnyMessageLikeEventContent, txn_id: Option<&TransactionId>) { - let txn_id = txn_id.map_or_else(TransactionId::new, ToOwned::to_owned); + pub async fn send(&self, content: AnyMessageLikeEventContent) { + let txn_id = TransactionId::new(); self.inner.handle_local_event(txn_id.clone(), content.clone()).await; if self.msg_sender.send(LocalMessage { content, txn_id }).await.is_err() { error!("Internal error: timeline message receiver is closed"); @@ -383,14 +383,13 @@ impl Timeline { /// propagated according to user intent, `No` otherwise /// /// * `txn_id` - Optional transaction ID, usually `None` - #[instrument(skip(self, content, reply_item, txn_id))] + #[instrument(skip(self, content, reply_item))] pub async fn send_reply( &self, content: RoomMessageEventContent, reply_item: &EventTimelineItem, forward_thread: ForwardThread, add_mentions: AddMentions, - txn_id: Option<&TransactionId>, ) -> Result<(), UnsupportedReplyItem> { // Error returns here must be in sync with // `EventTimelineItem::can_be_replied_to` @@ -425,7 +424,7 @@ impl Timeline { } }; - self.send(content.into(), txn_id).await; + self.send(content.into()).await; Ok(()) } diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index f5561e8ec..99daa4e80 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -23,7 +23,7 @@ use ruma::{ api::client::sync::sync_events::{v4::RoomSubscription, UnreadNotificationsCount}, assign, event_id, events::{room::message::RoomMessageEventContent, StateEventType}, - mxc_uri, room_id, uint, TransactionId, + mxc_uri, room_id, uint, }; use serde_json::json; use stream_assert::{assert_next_matches, assert_pending}; @@ -2517,9 +2517,7 @@ async fn test_room_latest_event() -> Result<(), Error> { ); // Insert a local event in the `Timeline`. - let txn_id: &TransactionId = "foobar-txn-id".into(); - - timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into(), Some(txn_id)).await; + timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into()).await; // The latest event of the `Timeline` is a local event. assert_matches!( @@ -2527,7 +2525,6 @@ async fn test_room_latest_event() -> Result<(), Error> { Some(timeline_event) => { assert!(timeline_event.is_local_echo()); assert_eq!(timeline_event.event_id(), None); - assert_eq!(timeline_event.transaction_id(), Some(txn_id)); } ); @@ -2537,7 +2534,6 @@ async fn test_room_latest_event() -> Result<(), Error> { Some(event) => { assert!(event.is_local_echo()); assert_eq!(event.event_id(), None); - assert_eq!(event.transaction_id(), Some(txn_id)); } ); diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs index 9b56a2829..8ddc863fd 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/echo.rs @@ -25,7 +25,7 @@ use matrix_sdk_ui::timeline::{ use ruma::{ event_id, events::room::message::{MessageType, RoomMessageEventContent}, - room_id, uint, TransactionId, + room_id, uint, }; use serde_json::json; use stream_assert::assert_next_matches; @@ -53,15 +53,15 @@ async fn echo() { let timeline = Arc::new(room.timeline().await); let (_, mut timeline_stream) = timeline.subscribe().await; - let event_id = event_id!("$wWgymRfo7ri1uQx0NXO40vLJ"); - let txn_id: &TransactionId = "my-txn-id".into(); - mock_encryption_state(&server, false).await; Mock::given(method("PUT")) .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) .and(header("authorization", "Bearer 1234")) - .respond_with(ResponseTemplate::new(200).set_body_json(&json!({ "event_id": event_id }))) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(&json!({ "event_id": "$wWgymRfo7ri1uQx0NXO40vLJ" })), + ) .mount(&server) .await; @@ -69,15 +69,14 @@ async fn echo() { let timeline = timeline.clone(); #[allow(unknown_lints, clippy::redundant_async_block)] // false positive let send_hdl = spawn(async move { - timeline - .send(RoomMessageEventContent::text_plain("Hello, World!").into(), Some(txn_id)) - .await + timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into()).await }); let _day_divider = assert_matches!(timeline_stream.next().await, Some(VectorDiff::PushBack { value }) => value); let local_echo = assert_matches!(timeline_stream.next().await, Some(VectorDiff::PushBack { value }) => value); let item = local_echo.as_event().unwrap(); assert_matches!(item.send_state(), Some(EventSendState::NotSentYet)); + let txn_id = item.transaction_id().unwrap(); let msg = assert_matches!(item.content(), TimelineItemContent::Message(msg) => msg); let text = assert_matches!(msg.msgtype(), MessageType::Text(text) => text); @@ -151,10 +150,7 @@ async fn retry_failed() { let (_, mut timeline_stream) = timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; - let event_id = event_id!("$wWgymRfo7ri1uQx0NXO40vLJ"); - let txn_id: &TransactionId = "my-txn-id".into(); - - timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into(), Some(txn_id)).await; + timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into()).await; // First, local echo is added assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { @@ -162,18 +158,25 @@ async fn retry_failed() { }); // Sending fails, the mock server has no matching route yet - assert_matches!(timeline_stream.next().await, Some(VectorDiff::Set { index: 0, value }) => { - assert_matches!(value.send_state(), Some(EventSendState::SendingFailed { .. })); - }); + let txn_id = assert_matches!( + timeline_stream.next().await, + Some(VectorDiff::Set { index: 0, value }) => { + assert_matches!(value.send_state(), Some(EventSendState::SendingFailed { .. })); + value.transaction_id().unwrap().to_owned() + } + ); Mock::given(method("PUT")) .and(path_regex(r"^/_matrix/client/r0/rooms/.*/send/.*")) .and(header("authorization", "Bearer 1234")) - .respond_with(ResponseTemplate::new(200).set_body_json(&json!({ "event_id": event_id }))) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(&json!({ "event_id": "$wWgymRfo7ri1uQx0NXO40vLJ" })), + ) .mount(&server) .await; - timeline.retry_send(txn_id).await.unwrap(); + timeline.retry_send(&txn_id).await.unwrap(); // After mocking the endpoint and retrying, it first transitions back out of // the error state @@ -206,7 +209,6 @@ async fn dedup_by_event_id_late() { let (_, mut timeline_stream) = timeline.subscribe().await; let event_id = event_id!("$wWgymRfo7ri1uQx0NXO40vLJ"); - let txn_id: &TransactionId = "my-txn-id".into(); mock_encryption_state(&server, false).await; @@ -223,7 +225,7 @@ async fn dedup_by_event_id_late() { .mount(&server) .await; - timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into(), Some(txn_id)).await; + timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into()).await; assert_matches!(timeline_stream.next().await, Some(VectorDiff::PushBack { .. })); // day divider let local_echo = assert_matches!(timeline_stream.next().await, Some(VectorDiff::PushBack { value }) => value); @@ -276,13 +278,12 @@ async fn cancel_failed() { let (_, mut timeline_stream) = timeline.subscribe_filter_map(|item| item.as_event().cloned()).await; - let txn_id: &TransactionId = "my-txn-id".into(); - - timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into(), Some(txn_id)).await; + timeline.send(RoomMessageEventContent::text_plain("Hello, World!").into()).await; // Local echo is added (immediately) - assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { + let txn_id = assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { assert_matches!(value.send_state(), Some(EventSendState::NotSentYet)); + value.transaction_id().unwrap().to_owned() }); // Sending fails, the mock server has no matching route @@ -291,7 +292,7 @@ async fn cancel_failed() { }); // Discard, assert the local echo is found - assert!(timeline.cancel_send(txn_id).await); + assert!(timeline.cancel_send(&txn_id).await); // Observable local echo being removed assert_matches!(timeline_stream.next().await, Some(VectorDiff::Remove { index: 0 })); diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs b/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs index 52a2f62c0..e05c43382 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs @@ -76,8 +76,8 @@ async fn message_order() { .mount(&server) .await; - timeline.send(RoomMessageEventContent::text_plain("First!").into(), None).await; - timeline.send(RoomMessageEventContent::text_plain("Second.").into(), None).await; + timeline.send(RoomMessageEventContent::text_plain("First!").into()).await; + timeline.send(RoomMessageEventContent::text_plain("Second.").into()).await; // Local echoes are available as soon as `timeline.send` returns assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { @@ -125,8 +125,8 @@ async fn retry_order() { // Send two messages without mocking the server response. // It will respond with a 404, resulting in a failed-to-send state. - timeline.send(RoomMessageEventContent::text_plain("First!").into(), Some("1".into())).await; - timeline.send(RoomMessageEventContent::text_plain("Second.").into(), Some("2".into())).await; + timeline.send(RoomMessageEventContent::text_plain("First!").into()).await; + timeline.send(RoomMessageEventContent::text_plain("Second.").into()).await; // Local echoes are available as soon as `timeline.send` returns assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { @@ -138,12 +138,17 @@ async fn retry_order() { // Local echoes are updated with the failed send state as soon as // the 404 response is received - assert_matches!(timeline_stream.next().await, Some(VectorDiff::Set { index: 0, value }) => { - assert_matches!(value.send_state().unwrap(), EventSendState::SendingFailed { .. }); - }); + let txn_id_1 = assert_matches!( + timeline_stream.next().await, + Some(VectorDiff::Set { index: 0, value }) => { + assert_matches!(value.send_state().unwrap(), EventSendState::SendingFailed { .. }); + value.transaction_id().unwrap().to_owned() + } + ); // The second one is cancelled without an extra delay - assert_next_matches!(timeline_stream, VectorDiff::Set { index: 1, value } => { + let txn_id_2 = assert_next_matches!(timeline_stream, VectorDiff::Set { index: 1, value } => { assert_matches!(value.send_state().unwrap(), EventSendState::Cancelled); + value.transaction_id().unwrap().to_owned() }); // Response for first message takes 100ms to respond @@ -172,8 +177,8 @@ async fn retry_order() { .await; // Retry the second message first - timeline.retry_send("2".into()).await.unwrap(); - timeline.retry_send("1".into()).await.unwrap(); + timeline.retry_send(&txn_id_2).await.unwrap(); + timeline.retry_send(&txn_id_1).await.unwrap(); // Both items are immediately updated and moved to the bottom in the order // of the function calls to indicate they are being sent @@ -229,7 +234,7 @@ async fn clear_with_echoes() { { let (_, mut timeline_stream) = timeline.subscribe().await; - timeline.send(RoomMessageEventContent::text_plain("Send failure").into(), None).await; + timeline.send(RoomMessageEventContent::text_plain("Send failure").into()).await; // Wait for the first message to fail. Don't use time, but listen for the first // timeline item diff to get back signalling the error. @@ -250,7 +255,7 @@ async fn clear_with_echoes() { .await; // (this one) - timeline.send(RoomMessageEventContent::text_plain("Pending").into(), None).await; + timeline.send(RoomMessageEventContent::text_plain("Pending").into()).await; // Another message comes in. sync_builder.add_joined_room( diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs index 654ffbc93..06c3e7bca 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/replies.rs @@ -306,7 +306,6 @@ async fn send_reply() { &hello_world_item, ForwardThread::Yes, AddMentions::No, - None, ) .await .unwrap(); diff --git a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs index 10ac4cb39..6c5cf2129 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/reactions.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/reactions.rs @@ -49,7 +49,7 @@ async fn test_toggling_reaction() -> Result<()> { let (_items, mut stream) = timeline.subscribe().await; // Send message - timeline.send(RoomMessageEventContent::text_plain("hi!").into(), None).await; + timeline.send(RoomMessageEventContent::text_plain("hi!").into()).await; // Sync until the remote echo arrives let event_id = loop {