From 84695adbb65a76e39421318d143f72e1a477dceb Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Fri, 31 May 2024 12:40:59 +0200 Subject: [PATCH] timeline: mark local echoes as not editable Also reunify two methods that did the same thing, with slightly different semantics, and test the one that wasn't tested at all. Note that `is_editable()` was already exposed to the FFI and used in EX apps. --- crates/matrix-sdk-ui/src/timeline/error.rs | 3 ++ .../src/timeline/event_item/mod.rs | 33 +++++++++++-------- crates/matrix-sdk-ui/src/timeline/mod.rs | 28 ++++++++++++---- .../tests/integration/timeline/edit.rs | 8 ++--- .../tests/integration/timeline/queue.rs | 12 +++++-- 5 files changed, 56 insertions(+), 28 deletions(-) diff --git a/crates/matrix-sdk-ui/src/timeline/error.rs b/crates/matrix-sdk-ui/src/timeline/error.rs index 2b35711fab1..3782f70b0f8 100644 --- a/crates/matrix-sdk-ui/src/timeline/error.rs +++ b/crates/matrix-sdk-ui/src/timeline/error.rs @@ -113,6 +113,7 @@ impl UnsupportedEditItem { pub(super) const MISSING_EVENT_ID: Self = Self(UnsupportedEditItemInner::MissingEventId); pub(super) const NOT_ROOM_MESSAGE: Self = Self(UnsupportedEditItemInner::NotRoomMessage); pub(super) const NOT_POLL_EVENT: Self = Self(UnsupportedEditItemInner::NotPollEvent); + pub(super) const NOT_OWN_EVENT: Self = Self(UnsupportedEditItemInner::NotOwnEvent); } #[cfg(not(tarpaulin_include))] @@ -130,6 +131,8 @@ enum UnsupportedEditItemInner { NotRoomMessage, #[error("tried to edit a non-poll event")] NotPollEvent, + #[error("tried to edit another user's event")] + NotOwnEvent, } #[derive(Debug, Error)] diff --git a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs index e2d826a0ce3..5c222a2d862 100644 --- a/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/event_item/mod.rs @@ -279,17 +279,31 @@ impl EventTimelineItem { } } - /// Flag indicating this timeline item can be edited by current user. + /// Flag indicating this timeline item can be edited by the current user. pub fn is_editable(&self) -> bool { + // This must be in sync with the early returns of `Timeline::edit` + // + if !self.is_own() { + // In theory could work, but it's hard to compute locally. + return false; + } + + if self.event_id().is_none() { + // Local echoes without an event id (not sent yet) can't be edited. + return false; + } + match self.content() { TimelineItemContent::Message(message) => { - self.is_own() - && matches!(message.msgtype(), MessageType::Text(_) | MessageType::Emote(_)) + matches!(message.msgtype(), MessageType::Text(_) | MessageType::Emote(_)) } TimelineItemContent::Poll(poll) => { - self.is_own() && poll.response_data.is_empty() && poll.end_event_timestamp.is_none() + poll.response_data.is_empty() && poll.end_event_timestamp.is_none() + } + _ => { + // Other timeline items can't be edited at the moment. + false } - _ => false, } } @@ -321,15 +335,6 @@ impl EventTimelineItem { } } - /// Check whether this item can be edited. - /// - /// Please also check whether the `sender` of this event is the client's - /// current user before presenting an edit button in the UI. - pub fn can_be_edited(&self) -> bool { - // This must be in sync with the early returns of `Timeline::edit` - self.event_id().is_some() && self.content().as_message().is_some() - } - /// Get the raw JSON representation of the initial event (the one that /// caused this timeline item to be created). /// diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index 1449aedbe0e..55edb298f4e 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -60,6 +60,7 @@ use tracing::{error, instrument, trace, warn}; use self::{ error::{RedactEventError, SendEventError}, + event_item::EventTimelineItemKind, futures::SendAttachment, }; @@ -361,7 +362,7 @@ impl Timeline { /// Send an edit to the given event. /// /// Currently only supports `m.room.message` events whose event ID is known. - /// Please check [`EventTimelineItem::can_be_edited`] before calling this. + /// Please check [`EventTimelineItem::is_editable`] before calling this. /// /// # Arguments /// @@ -374,11 +375,14 @@ impl Timeline { new_content: RoomMessageEventContentWithoutRelation, edit_item: &EventTimelineItem, ) -> Result<(), SendEventError> { - // Early returns here must be in sync with - // `EventTimelineItem::can_be_edited` + // Early returns here must be in sync with `EventTimelineItem::is_editable`. + if !edit_item.is_own() { + return Err(UnsupportedEditItem::NOT_OWN_EVENT.into()); + } let Some(event_id) = edit_item.event_id() else { return Err(UnsupportedEditItem::MISSING_EVENT_ID.into()); }; + let TimelineItemContent::Message(original_content) = edit_item.content() else { return Err(UnsupportedEditItem::NOT_ROOM_MESSAGE.into()); }; @@ -419,19 +423,29 @@ impl Timeline { poll: UnstablePollStartContentBlock, edit_item: &EventTimelineItem, ) -> Result<(), SendEventError> { + // TODO: refactor this function into [`Self::edit`], there's no good reason to + // keep a separate function for this. + + // Early returns here must be in sync with `EventTimelineItem::is_editable`. + if !edit_item.is_own() { + return Err(UnsupportedEditItem::NOT_OWN_EVENT.into()); + } let Some(event_id) = edit_item.event_id() else { return Err(UnsupportedEditItem::MISSING_EVENT_ID.into()); }; + let TimelineItemContent::Poll(_) = edit_item.content() else { return Err(UnsupportedEditItem::NOT_POLL_EVENT.into()); }; - let replacement_poll = ReplacementUnstablePollStartEventContent::plain_text( + let content = ReplacementUnstablePollStartEventContent::plain_text( fallback_text, poll, event_id.into(), ); - self.send(UnstablePollStartEventContent::from(replacement_poll).into()).await?; + + self.send(UnstablePollStartEventContent::from(content).into()).await?; + Ok(()) } @@ -560,7 +574,7 @@ impl Timeline { reason: Option<&str>, ) -> Result { match &event.kind { - event_item::EventTimelineItemKind::Local(local) => { + EventTimelineItemKind::Local(local) => { if let Some(handle) = local.abort_handle.clone() { Ok(handle.abort().await) } else { @@ -570,7 +584,7 @@ impl Timeline { } } - event_item::EventTimelineItemKind::Remote(remote) => { + EventTimelineItemKind::Remote(remote) => { self.room() .redact(&remote.event_id, reason, None) .await diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs index a3edc5fc5a3..ed131459594 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/edit.rs @@ -189,7 +189,7 @@ async fn test_send_edit() { assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); let hello_world_message = hello_world_item.content().as_message().unwrap(); assert!(!hello_world_message.is_edited()); - assert!(hello_world_item.can_be_edited()); + assert!(hello_world_item.is_editable()); mock_encryption_state(&server, false).await; Mock::given(method("PUT")) @@ -269,14 +269,14 @@ async fn test_send_reply_edit() { let _response = client.sync_once(sync_settings.clone()).await.unwrap(); server.reset().await; - // 'Hello, World!' message + // 'Hello, World!' message. assert_next_matches!(timeline_stream, VectorDiff::PushBack { .. }); - // Reply message + // Reply message. let reply_item = assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => value); let reply_message = reply_item.content().as_message().unwrap(); assert!(!reply_message.is_edited()); - assert!(reply_item.can_be_edited()); + assert!(reply_item.is_editable()); let in_reply_to = reply_message.in_reply_to().unwrap(); assert_eq!(in_reply_to.event_id, fst_event_id); assert_matches!(in_reply_to.event, TimelineDetails::Ready(_)); diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs b/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs index c0fdc69858c..50f15df21c0 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/queue.rs @@ -87,25 +87,31 @@ async fn test_message_order() { // Local echoes are available after the sending queue has processed these. assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { + assert!(!value.is_editable(), "local echo for first can't be edited"); assert_eq!(value.content().as_message().unwrap().body(), "First!"); }); assert_next_matches!(timeline_stream, VectorDiff::PushBack { value } => { + assert!(!value.is_editable(), "local echo for second can't be edited"); assert_eq!(value.content().as_message().unwrap().body(), "Second."); }); - // Wait 200ms for the first msg, 100ms for the second, 200ms for overhead + // Wait 200ms for the first msg, 100ms for the second, 200ms for overhead. sleep(Duration::from_millis(500)).await; - // The first item should be updated first + // The first item should be updated first. assert_next_matches!(timeline_stream, VectorDiff::Set { index: 0, value } => { + assert!(value.is_editable(), "remote echo of first can be edited"); assert_eq!(value.content().as_message().unwrap().body(), "First!"); assert_eq!(value.event_id().unwrap(), "$PyHxV5mYzjetBUT3qZq7V95GOzxb02EP"); }); - // Then the second one + + // Then the second one. assert_next_matches!(timeline_stream, VectorDiff::Set { index: 1, value } => { + assert!(value.is_editable(), "remote echo of second can be edited"); assert_eq!(value.content().as_message().unwrap().body(), "Second."); assert_eq!(value.event_id().unwrap(), "$5E2kLK/Sg342bgBU9ceEIEPYpbFaqJpZ"); }); + assert_pending!(timeline_stream); }