From 821fa8fa995377594e4611a21593a44660867292 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 17 Oct 2024 12:15:45 +0200 Subject: [PATCH] refactor(timeline): don't return a bool in `Timeline::edit` See previous commit for explanations. This makes for a simpler API anyways. Changelog: `Timeline::edit` doesn't return a bool anymore to indicate it couldn't manage the edit in some cases, but will return errors indicating what the cause of the error is. --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 4 +- crates/matrix-sdk-ui/src/timeline/error.rs | 16 +++++++- crates/matrix-sdk-ui/src/timeline/mod.rs | 38 +++++++++++-------- .../tests/integration/timeline/edit.rs | 26 ++++++------- .../src/tests/timeline.rs | 6 +-- 5 files changed, 52 insertions(+), 38 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index ca8503f01be..cc7a3b285df 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -500,8 +500,8 @@ impl Timeline { .edit(&event_or_transaction_id.clone().try_into()?, new_content.clone().try_into()?) .await { - Ok(true) => Ok(()), - Ok(false) | Err(timeline::Error::EventNotInTimeline(_)) => { + Ok(()) => Ok(()), + Err(timeline::Error::EventNotInTimeline(_)) => { // If we couldn't edit, assume it was an (remote) event that wasn't in the // timeline, and try to edit it via the room itself. let event_id = match event_or_transaction_id { diff --git a/crates/matrix-sdk-ui/src/timeline/error.rs b/crates/matrix-sdk-ui/src/timeline/error.rs index fc3d0fdf611..53149ee4a83 100644 --- a/crates/matrix-sdk-ui/src/timeline/error.rs +++ b/crates/matrix-sdk-ui/src/timeline/error.rs @@ -14,7 +14,6 @@ use matrix_sdk::{ event_cache::{paginator::PaginatorError, EventCacheError}, - room::edit::EditError, send_queue::RoomSendQueueError, HttpError, }; @@ -79,6 +78,21 @@ pub enum Error { RedactError(#[from] RedactError), } +#[derive(Error, Debug)] +pub enum EditError { + /// The content types have changed. + #[error("the new content type ({new}) doesn't match that of the previous content ({original}")] + ContentMismatch { original: String, new: String }, + + /// The local echo we tried to edit has been lost. + #[error("Invalid state: the local echo we tried to abort has been lost.")] + InvalidLocalEchoState, + + /// An error happened at a lower level. + #[error(transparent)] + RoomError(#[from] matrix_sdk::room::edit::EditError), +} + #[derive(Error, Debug)] pub enum RedactError { /// Local event to redact wasn't found for transaction id diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 1a3aa497e6e..812dc0ae8f6 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -449,20 +449,12 @@ impl Timeline { /// /// Only supports events for which [`EventTimelineItem::is_editable()`] /// returns `true`. - /// - /// # Returns - /// - /// - Returns `Ok(true)` if the edit was added to the send queue. - /// - Returns `Ok(false)` if the edit targets an item that has no local nor - /// matching remote item. - /// - Returns an error if there was an issue sending the redaction event, or - /// interacting with the sending queue. #[instrument(skip(self, new_content))] pub async fn edit( &self, item_id: &TimelineEventItemId, new_content: EditedContent, - ) -> Result { + ) -> Result<(), Error> { let items = self.items().await; let Some((_pos, item)) = rfind_event_by_item_id(&items, item_id) else { return Err(Error::EventNotInTimeline(item_id.clone())); @@ -470,9 +462,13 @@ impl Timeline { match item.handle() { TimelineItemHandle::Remote(event_id) => { - let content = self.room().make_edit_event(event_id, new_content).await?; + let content = self + .room() + .make_edit_event(event_id, new_content) + .await + .map_err(EditError::RoomError)?; self.send(content).await?; - Ok(true) + Ok(()) } TimelineItemHandle::Local(handle) => { @@ -482,8 +478,11 @@ impl Timeline { if matches!(item.content, TimelineItemContent::Message(_)) { AnyMessageLikeEventContent::RoomMessage(message.into()) } else { - warn!("New content (m.room.message) doesn't match previous event content."); - return Ok(false); + return Err(EditError::ContentMismatch { + original: item.content.debug_string().to_owned(), + new: "a message".to_owned(), + } + .into()); } } EditedContent::PollStart { new_content, .. } => { @@ -494,13 +493,20 @@ impl Timeline { ), ) } else { - warn!("New content (poll start) doesn't match previous event content."); - return Ok(false); + return Err(EditError::ContentMismatch { + original: item.content.debug_string().to_owned(), + new: "a poll".to_owned(), + } + .into()); } } }; - Ok(handle.edit(new_content).await.map_err(RoomSendQueueError::StorageError)?) + if !handle.edit(new_content).await.map_err(RoomSendQueueError::StorageError)? { + return Err(EditError::InvalidLocalEchoState.into()); + } + + Ok(()) } } } diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs index f3c22913253..3d848cc2116 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs @@ -30,7 +30,8 @@ use matrix_sdk_test::{ }; use matrix_sdk_ui::{ timeline::{ - Error, EventSendState, RoomExt, TimelineDetails, TimelineEventItemId, TimelineItemContent, + EditError, Error, EventSendState, RoomExt, TimelineDetails, TimelineEventItemId, + TimelineItemContent, }, Timeline, }; @@ -235,8 +236,8 @@ async fn test_edit_local_echo() { .mount(&server) .await; - // Let's edit the local echo. - let did_edit = timeline + // Editing the local echo works, since it was in the failed state. + timeline .edit( &item.identifier(), EditedContent::RoomMessage(RoomMessageEventContent::text_plain("hello, world").into()), @@ -244,9 +245,6 @@ async fn test_edit_local_echo() { .await .unwrap(); - // We could edit the local echo, since it was in the failed state. - assert!(did_edit); - // Observe local echo being replaced. assert_let!(Some(VectorDiff::Set { index: 1, value: item }) = timeline_stream.next().await); @@ -410,7 +408,7 @@ async fn test_send_reply_edit() { .mount(&server) .await; - let edited = timeline + timeline .edit( &reply_item.identifier(), EditedContent::RoomMessage(RoomMessageEventContentWithoutRelation::text_plain( @@ -419,7 +417,6 @@ async fn test_send_reply_edit() { ) .await .unwrap(); - assert!(edited); // Let the send queue handle the event. yield_now().await; @@ -521,7 +518,7 @@ async fn test_edit_to_replied_updates_reply() { .await; // If I edit the first message,… - let edited = timeline + timeline .edit( &replied_to_item.identifier(), EditedContent::RoomMessage(RoomMessageEventContentWithoutRelation::text_plain( @@ -530,7 +527,6 @@ async fn test_edit_to_replied_updates_reply() { ) .await .unwrap(); - assert!(edited); yield_now().await; // let the send queue handle the edit. @@ -822,10 +818,10 @@ async fn test_edit_local_echo_with_unsupported_content() { }; // Let's edit the local echo (message) with an unsupported type (poll start). - let did_edit = timeline.edit(&item.identifier(), poll_start_content).await.unwrap(); + let edit_err = timeline.edit(&item.identifier(), poll_start_content).await.unwrap_err(); // We couldn't edit the local echo, since their content types didn't match - assert!(!did_edit); + assert_matches!(edit_err, Error::EditError(EditError::ContentMismatch { .. })); timeline .send(AnyMessageLikeEventContent::UnstablePollStart(UnstablePollStartEventContent::New( @@ -840,7 +836,7 @@ async fn test_edit_local_echo_with_unsupported_content() { assert_matches!(item.send_state(), Some(EventSendState::NotSentYet)); // Let's edit the local echo (poll start) with an unsupported type (message). - let did_edit = timeline + let edit_err = timeline .edit( &item.identifier(), EditedContent::RoomMessage(RoomMessageEventContentWithoutRelation::text_plain( @@ -848,10 +844,10 @@ async fn test_edit_local_echo_with_unsupported_content() { )), ) .await - .unwrap(); + .unwrap_err(); // We couldn't edit the local echo, since their content types didn't match - assert!(!did_edit); + assert_matches!(edit_err, Error::EditError(EditError::ContentMismatch { .. })); } struct PendingEditHelper { diff --git a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs index 039d593873f..531ca7a1d21 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/timeline.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/timeline.rs @@ -289,7 +289,8 @@ async fn test_stale_local_echo_time_abort_edit() { } // Now do a crime: try to edit the local echo. - let did_edit = timeline + // The edit works on the local echo and applies to the remote echo \o/. + timeline .edit( &local_echo.identifier(), EditedContent::RoomMessage(RoomMessageEventContent::text_plain("bonjour").into()), @@ -297,9 +298,6 @@ async fn test_stale_local_echo_time_abort_edit() { .await .unwrap(); - // The edit works on the local echo and applies to the remote echo \o/. - assert!(did_edit); - let vector_diff = timeout(Duration::from_secs(5), stream.next()).await.unwrap().unwrap(); let remote_echo = assert_matches!(vector_diff, VectorDiff::Set { index: 0, value } => value); assert!(!remote_echo.is_local_echo());