Skip to content

Commit

Permalink
timeline: mark local echoes as not editable
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bnjbvr committed May 31, 2024
1 parent 4b4e440 commit 84695ad
Show file tree
Hide file tree
Showing 5 changed files with 56 additions and 28 deletions.
3 changes: 3 additions & 0 deletions crates/matrix-sdk-ui/src/timeline/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Expand All @@ -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)]
Expand Down
33 changes: 19 additions & 14 deletions crates/matrix-sdk-ui/src/timeline/event_item/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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).
///
Expand Down
28 changes: 21 additions & 7 deletions crates/matrix-sdk-ui/src/timeline/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ use tracing::{error, instrument, trace, warn};

use self::{
error::{RedactEventError, SendEventError},
event_item::EventTimelineItemKind,
futures::SendAttachment,
};

Expand Down Expand Up @@ -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
///
Expand All @@ -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());
};
Expand Down Expand Up @@ -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(())
}

Expand Down Expand Up @@ -560,7 +574,7 @@ impl Timeline {
reason: Option<&str>,
) -> Result<bool, RedactEventError> {
match &event.kind {
event_item::EventTimelineItemKind::Local(local) => {
EventTimelineItemKind::Local(local) => {
if let Some(handle) = local.abort_handle.clone() {
Ok(handle.abort().await)
} else {
Expand All @@ -570,7 +584,7 @@ impl Timeline {
}
}

event_item::EventTimelineItemKind::Remote(remote) => {
EventTimelineItemKind::Remote(remote) => {
self.room()
.redact(&remote.event_id, reason, None)
.await
Expand Down
8 changes: 4 additions & 4 deletions crates/matrix-sdk-ui/tests/integration/timeline/edit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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(_));
Expand Down
12 changes: 9 additions & 3 deletions crates/matrix-sdk-ui/tests/integration/timeline/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down

0 comments on commit 84695ad

Please sign in to comment.