diff --git a/bindings/matrix-sdk-ffi/CHANGELOG.md b/bindings/matrix-sdk-ffi/CHANGELOG.md index e9e0b9acff6..6e76c12956d 100644 --- a/bindings/matrix-sdk-ffi/CHANGELOG.md +++ b/bindings/matrix-sdk-ffi/CHANGELOG.md @@ -6,6 +6,13 @@ All notable changes to this project will be documented in this file. ## [Unreleased] - ReleaseDate +### Bug Fixes + +- [**breaking**] `LatestEventValue::Local { is_sending: bool }` is replaced + by [`state: LatestEventValueLocalState`] to represent 3 states: `IsSending`, + `HasBeenSent` and `CannotBeSent`. + ([#5968](https://github.com/matrix-org/matrix-rust-sdk/pull/5968/)) + ### Features - Add `SpaceService::get_space_room` to get a space given its id from the space graph if available. diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 5f28a68bf2a..db1be1ee5a9 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -31,8 +31,9 @@ use matrix_sdk_common::{ }; use matrix_sdk_ui::timeline::{ self, AttachmentConfig, AttachmentSource, EventItemOrigin, - LatestEventValue as UiLatestEventValue, MediaUploadProgress as SdkMediaUploadProgress, Profile, - TimelineDetails, TimelineUniqueId as SdkTimelineUniqueId, + LatestEventValue as UiLatestEventValue, LatestEventValueLocalState, + MediaUploadProgress as SdkMediaUploadProgress, Profile, TimelineDetails, + TimelineUniqueId as SdkTimelineUniqueId, }; use mime::Mime; use reply::{EmbeddedEventDetails, InReplyToDetails}; @@ -1301,7 +1302,7 @@ impl LazyTimelineItemProvider { } /// Mimic the [`UiLatestEventValue`] type. -#[derive(Clone, uniffi::Enum)] +#[derive(uniffi::Enum)] pub enum LatestEventValue { None, Remote { @@ -1316,7 +1317,7 @@ pub enum LatestEventValue { sender: String, profile: ProfileDetails, content: TimelineItemContent, - is_sending: bool, + state: LatestEventValueLocalState, }, } @@ -1333,13 +1334,13 @@ impl From for LatestEventValue { content: content.into(), } } - UiLatestEventValue::Local { timestamp, sender, profile, content, is_sending } => { + UiLatestEventValue::Local { timestamp, sender, profile, content, state } => { Self::Local { timestamp: timestamp.into(), sender: sender.to_string(), profile: profile.into(), content: content.into(), - is_sending, + state, } } } diff --git a/crates/matrix-sdk-base/CHANGELOG.md b/crates/matrix-sdk-base/CHANGELOG.md index 7b59caff160..45f960d7987 100644 --- a/crates/matrix-sdk-base/CHANGELOG.md +++ b/crates/matrix-sdk-base/CHANGELOG.md @@ -6,6 +6,12 @@ All notable changes to this project will be documented in this file. ## [Unreleased] - ReleaseDate +### Bug Fixes + +- [**breaking**] New `LatestEventValue::LocalHasBeenSent` variant to represent + a local event that has been sent successfully. + ([#5968](https://github.com/matrix-org/matrix-rust-sdk/pull/5968)) + ### Refactor - [**breaking**] The `message-ids` feature has been removed. It was already a no-op and has now diff --git a/crates/matrix-sdk-base/src/latest_event.rs b/crates/matrix-sdk-base/src/latest_event.rs index 1ecf18375a8..67a48cac879 100644 --- a/crates/matrix-sdk-base/src/latest_event.rs +++ b/crates/matrix-sdk-base/src/latest_event.rs @@ -19,6 +19,10 @@ pub enum LatestEventValue { /// The latest event represents a local event that is sending. LocalIsSending(LocalLatestEventValue), + /// The latest event represents a local event that has been sent + /// successfully. It should come quickly as a [`Self::Remote`]. + LocalHasBeenSent(LocalLatestEventValue), + /// The latest event represents a local event that cannot be sent, either /// because a previous local event, or this local event cannot be sent. LocalCannotBeSent(LocalLatestEventValue), @@ -28,19 +32,21 @@ impl LatestEventValue { /// Get the timestamp of the [`LatestEventValue`]. /// /// If it's [`None`], it returns `None`. If it's [`Remote`], it returns the - /// [`TimelineEvent::timestamp`]. If it's [`LocalIsSending`] or - /// [`LocalCannotBeSent`], it returns the + /// [`TimelineEvent::timestamp`]. If it's [`LocalIsSending`], + /// [`LocalHasBeenSent`] or [`LocalCannotBeSent`], it returns the /// [`LocalLatestEventValue::timestamp`] value. /// /// [`None`]: LatestEventValue::None /// [`Remote`]: LatestEventValue::Remote /// [`LocalIsSending`]: LatestEventValue::LocalIsSending + /// [`LocalHasBeenSent`]: LatestEventValue::LocalHasBeenSent /// [`LocalCannotBeSent`]: LatestEventValue::LocalCannotBeSent pub fn timestamp(&self) -> Option { match self { Self::None => None, Self::Remote(remote_latest_event_value) => remote_latest_event_value.timestamp(), Self::LocalIsSending(LocalLatestEventValue { timestamp, .. }) + | Self::LocalHasBeenSent(LocalLatestEventValue { timestamp, .. }) | Self::LocalCannotBeSent(LocalLatestEventValue { timestamp, .. }) => Some(*timestamp), } } @@ -52,11 +58,25 @@ impl LatestEventValue { /// [`LocalCannotBeSent`]: LatestEventValue::LocalCannotBeSent pub fn is_local(&self) -> bool { match self { - Self::LocalIsSending(_) | Self::LocalCannotBeSent(_) => true, + Self::LocalIsSending(_) | Self::LocalHasBeenSent(_) | Self::LocalCannotBeSent(_) => { + true + } Self::None | Self::Remote(_) => false, } } + /// Check whether the [`LatestEventValue`] represents an unsent event, i.e. + /// is [`LocalIsSending`] nor [`LocalCannotBeSent`]. + /// + /// [`LocalIsSending`]: LatestEventValue::LocalIsSending + /// [`LocalCannotBeSent`]: LatestEventValue::LocalCannotBeSent + pub fn is_unsent(&self) -> bool { + match self { + Self::LocalIsSending(_) | Self::LocalCannotBeSent(_) => true, + Self::LocalHasBeenSent(_) | Self::Remote(_) | Self::None => false, + } + } + /// Check whether the [`LatestEventValue`] is not set, i.e. [`None`]. /// /// [`None`]: LatestEventValue::None @@ -132,6 +152,19 @@ mod tests_latest_event_value { assert_eq!(value.timestamp(), Some(MilliSecondsSinceUnixEpoch(uint!(42)))); } + #[test] + fn test_timestamp_with_local_has_been_sent() { + let value = LatestEventValue::LocalHasBeenSent(LocalLatestEventValue { + timestamp: MilliSecondsSinceUnixEpoch(uint!(42)), + content: SerializableEventContent::new(&AnyMessageLikeEventContent::RoomMessage( + RoomMessageEventContent::text_plain("raclette"), + )) + .unwrap(), + }); + + assert_eq!(value.timestamp(), Some(MilliSecondsSinceUnixEpoch(uint!(42)))); + } + #[test] fn test_timestamp_with_local_cannot_be_sent() { let value = LatestEventValue::LocalCannotBeSent(LocalLatestEventValue { diff --git a/crates/matrix-sdk-base/src/room/latest_event.rs b/crates/matrix-sdk-base/src/room/latest_event.rs index 99c211ee9d5..1f791027a5e 100644 --- a/crates/matrix-sdk-base/src/room/latest_event.rs +++ b/crates/matrix-sdk-base/src/room/latest_event.rs @@ -31,8 +31,8 @@ impl Room { self.info.read().latest_event_value.timestamp() } - /// Return the value of [`LatestEventValue::is_local`]. - pub fn latest_event_is_local(&self) -> bool { - self.info.read().latest_event_value.is_local() + /// Return the value of [`LatestEventValue::is_unsent`]. + pub fn latest_event_is_unsent(&self) -> bool { + self.info.read().latest_event_value.is_unsent() } } diff --git a/crates/matrix-sdk-ui/CHANGELOG.md b/crates/matrix-sdk-ui/CHANGELOG.md index 08401adec76..77312d575c6 100644 --- a/crates/matrix-sdk-ui/CHANGELOG.md +++ b/crates/matrix-sdk-ui/CHANGELOG.md @@ -6,15 +6,23 @@ All notable changes to this project will be documented in this file. ## [Unreleased] - ReleaseDate +### Bug Fixes + +- [**breaking**] `LatestEventValue::Local { is_sending: bool }` is replaced + by [`state: LatestEventValueLocalState`] to represent 3 states: `IsSending`, + `HasBeenSent` and `CannotBeSent`. + ([#5968](https://github.com/matrix-org/matrix-rust-sdk/pull/5968/)) + ### Features -- Add `SpaceService::get_space_room` to get a space given its id from the space graph if available. -[#5944](https://github.com/matrix-org/matrix-rust-sdk/pull/5944) +- Add `SpaceService::get_space_room` to get a space + given its id from the space graph if available. + ([#5944](https://github.com/matrix-org/matrix-rust-sdk/pull/5944)) - [**breaking**]: The new Latest Event API replaces the old API. All the `new_` prefixes have been removed. The following methods are removed: `EventTimelineItem::from_latest_event`, and `Timeline::latest_event`. See the documentation of `matrix_sdk::latest_event` to learn about the new API. - [#5624](https://github.com/matrix-org/matrix-rust-sdk/pull/5624/) + ([#5624](https://github.com/matrix-org/matrix-rust-sdk/pull/5624/)) - `Room::load_event_with_relations` now also calls `/relations` to fetch related events when falling back to network mode after a cache miss. ([#5930](https://github.com/matrix-org/matrix-rust-sdk/pull/5930)) diff --git a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs index 07ab387ffb7..cb1103af97f 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/room_list.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/room_list.rs @@ -423,8 +423,8 @@ pub struct RoomListItem { /// Cache of `Room::latest_event_timestamp`. pub(super) cached_latest_event_timestamp: Option, - /// Cache of `Room::latest_event_is_local`. - pub(super) cached_latest_event_is_local: bool, + /// Cache of `Room::latest_event_is_unsent`. + pub(super) cached_latest_event_is_unsent: bool, /// Cache of `Room::recency_stamp`. pub(super) cached_recency_stamp: Option, @@ -448,7 +448,7 @@ impl RoomListItem { /// Refresh the cached data. pub(super) fn refresh_cached_data(&mut self) { self.cached_latest_event_timestamp = self.inner.latest_event_timestamp(); - self.cached_latest_event_is_local = self.inner.latest_event_is_local(); + self.cached_latest_event_is_unsent = self.inner.latest_event_is_unsent(); self.cached_recency_stamp = self.inner.recency_stamp(); self.cached_display_name = self.inner.cached_display_name().map(|name| name.to_string()); self.cached_is_space = self.inner.is_space(); @@ -459,7 +459,7 @@ impl RoomListItem { impl From for RoomListItem { fn from(inner: Room) -> Self { let cached_latest_event_timestamp = inner.latest_event_timestamp(); - let cached_latest_event_is_local = inner.latest_event_is_local(); + let cached_latest_event_is_unsent = inner.latest_event_is_unsent(); let cached_recency_stamp = inner.recency_stamp(); let cached_display_name = inner.cached_display_name().map(|name| name.to_string()); let cached_is_space = inner.is_space(); @@ -468,7 +468,7 @@ impl From for RoomListItem { Self { inner, cached_latest_event_timestamp, - cached_latest_event_is_local, + cached_latest_event_is_unsent, cached_recency_stamp, cached_display_name, cached_is_space, diff --git a/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs b/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs index 4edd76b658c..ab544c6f84c 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/sorters/latest_event.rs @@ -16,14 +16,15 @@ use std::cmp::Ordering; use super::{RoomListItem, Sorter}; -fn cmp(are_latest_events_locals: F, left: &RoomListItem, right: &RoomListItem) -> Ordering +fn cmp(are_latest_events_unsent: F, left: &RoomListItem, right: &RoomListItem) -> Ordering where F: Fn(&RoomListItem, &RoomListItem) -> (bool, bool), { - // We want local latest event to come first. When there is a remote latest event - // or no latest event, we don't want to sort them. + // We want latest events representing unsent events to come firsts. When + // there is a remote latest event or no latest event, we don't want to sort + // them. // NOTE: This is the same as a.cmp(b).reverse() for booleans. - match are_latest_events_locals(left, right) { + match are_latest_events_unsent(left, right) { // `false` and `false`, i.e.: // - `None` == `None`. // - `None` == `Remote`. @@ -48,22 +49,15 @@ where } /// Create a new sorter that will sort two [`RoomListItem`] by their latest -/// events' state: latest events representing a local event -/// ([`LatestEventValue::LocalIsSending`] or -/// [`LatestEventValue::LocalCannotBeSent`]) come first, and latest event -/// representing a remote event ([`LatestEventValue::Remote`]) come last. -/// -/// [`LatestEventValue::LocalIsSending`]: matrix_sdk_base::latest_event::LatestEventValue::LocalIsSending -/// [`LatestEventValue::LocalCannotBeSent`]: matrix_sdk_base::latest_event::LatestEventValue::LocalCannotBeSent -/// [`LatestEventValue::Remote`]: matrix_sdk_base::latest_event::LatestEventValue::Remote +/// events' state: latest events representing unsent events come firsts. pub fn new_sorter() -> impl Sorter { let latest_events = |left: &RoomListItem, right: &RoomListItem| { - // Be careful. This method is called **a lot** in the context of a sorter. Using - // `Room::latest_event` would be dramatic as it returns a clone of the - // `LatestEventValue`. It's better to use the more specific method - // `Room::latest_event_is_local`, where the value is cached in this module's - // `Room` type. - (left.cached_latest_event_is_local, right.cached_latest_event_is_local) + // Be careful. This method is called **a lot** in the context of a + // sorter. Using `Room::latest_event` would be dramatic as it returns a + // clone of the `LatestEventValue`. It's better to use the more specific + // method `Room::latest_event_is_unsent`, where the value is cached + // in `RoomListItem`. + (left.cached_latest_event_is_unsent, right.cached_latest_event_is_unsent) }; move |left, right| -> Ordering { cmp(latest_events, left, right) } @@ -119,6 +113,16 @@ mod tests { }) } + fn local_has_been_sent() -> LatestEventValue { + LatestEventValue::LocalHasBeenSent(LocalLatestEventValue { + timestamp: MilliSecondsSinceUnixEpoch(uint!(42)), + content: SerializableEventContent::new(&AnyMessageLikeEventContent::RoomMessage( + RoomMessageEventContent::text_plain("raclette"), + )) + .unwrap(), + }) + } + fn local_cannot_be_sent() -> LatestEventValue { LatestEventValue::LocalCannotBeSent(LocalLatestEventValue { timestamp: MilliSecondsSinceUnixEpoch(uint!(42)), @@ -129,6 +133,10 @@ mod tests { }) } + fn pair(left: LatestEventValue, right: LatestEventValue) -> (bool, bool) { + (left.is_unsent(), right.is_unsent()) + } + #[async_test] async fn test_none_or_remote_and_none_or_remote() { let (client, server) = logged_in_client_with_server().await; @@ -138,34 +146,22 @@ mod tests { // `None` and `None`. { - assert_eq!( - cmp(|_, _| (none().is_local(), none().is_local()), &room_a, &room_b), - Ordering::Equal - ); + assert_eq!(cmp(|_, _| pair(none(), none()), &room_a, &room_b), Ordering::Equal); } // `None` and `Remote`. { - assert_eq!( - cmp(|_, _| (none().is_local(), remote().is_local()), &room_a, &room_b), - Ordering::Equal - ); + assert_eq!(cmp(|_, _| pair(none(), remote()), &room_a, &room_b), Ordering::Equal); } // `Remote` and `None`. { - assert_eq!( - cmp(|_, _| (remote().is_local(), none().is_local()), &room_a, &room_b), - Ordering::Equal - ); + assert_eq!(cmp(|_, _| pair(remote(), none()), &room_a, &room_b), Ordering::Equal); } // `Remote` and `None`. { - assert_eq!( - cmp(|_, _| (remote().is_local(), remote().is_local()), &room_a, &room_b), - Ordering::Equal - ); + assert_eq!(cmp(|_, _| pair(remote(), remote()), &room_a, &room_b), Ordering::Equal); } } @@ -179,15 +175,15 @@ mod tests { // `None` and `Local*`. { assert_eq!( - cmp(|_, _| (none().is_local(), local_is_sending().is_local()), &room_a, &room_b), + cmp(|_, _| pair(none(), local_is_sending()), &room_a, &room_b), Ordering::Greater ); assert_eq!( - cmp( - |_, _| (none().is_local(), local_cannot_be_sent().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(none(), local_has_been_sent()), &room_a, &room_b), + Ordering::Equal + ); + assert_eq!( + cmp(|_, _| pair(none(), local_cannot_be_sent()), &room_a, &room_b), Ordering::Greater ); } @@ -195,15 +191,15 @@ mod tests { // `Remote` and `Local*`. { assert_eq!( - cmp(|_, _| (remote().is_local(), local_is_sending().is_local()), &room_a, &room_b), + cmp(|_, _| pair(remote(), local_is_sending()), &room_a, &room_b), Ordering::Greater ); assert_eq!( - cmp( - |_, _| (remote().is_local(), local_cannot_be_sent().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(remote(), local_has_been_sent()), &room_a, &room_b), + Ordering::Equal + ); + assert_eq!( + cmp(|_, _| pair(remote(), local_cannot_be_sent()), &room_a, &room_b), Ordering::Greater ); } @@ -219,15 +215,15 @@ mod tests { // `Local*` and `None`. { assert_eq!( - cmp(|_, _| (local_is_sending().is_local(), none().is_local()), &room_a, &room_b), + cmp(|_, _| pair(local_is_sending(), none()), &room_a, &room_b), Ordering::Less ); assert_eq!( - cmp( - |_, _| (local_cannot_be_sent().is_local(), none().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_has_been_sent(), none()), &room_a, &room_b), + Ordering::Equal + ); + assert_eq!( + cmp(|_, _| pair(local_cannot_be_sent(), none()), &room_a, &room_b), Ordering::Less ); } @@ -235,15 +231,15 @@ mod tests { // `Local*` and `Remote`. { assert_eq!( - cmp(|_, _| (local_is_sending().is_local(), remote().is_local()), &room_a, &room_b), + cmp(|_, _| pair(local_is_sending(), remote()), &room_a, &room_b), Ordering::Less ); assert_eq!( - cmp( - |_, _| (local_cannot_be_sent().is_local(), remote().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_has_been_sent(), remote()), &room_a, &room_b), + Ordering::Equal + ); + assert_eq!( + cmp(|_, _| pair(local_cannot_be_sent(), remote()), &room_a, &room_b), Ordering::Less ); } @@ -259,35 +255,41 @@ mod tests { // `Local*` and `Local*`. { assert_eq!( - cmp( - |_, _| (local_is_sending().is_local(), local_is_sending().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_is_sending(), local_is_sending()), &room_a, &room_b), Ordering::Equal ); assert_eq!( - cmp( - |_, _| (local_is_sending().is_local(), local_cannot_be_sent().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_is_sending(), local_has_been_sent()), &room_a, &room_b), + Ordering::Less + ); + assert_eq!( + cmp(|_, _| pair(local_is_sending(), local_cannot_be_sent()), &room_a, &room_b), Ordering::Equal ); + assert_eq!( - cmp( - |_, _| (local_cannot_be_sent().is_local(), local_is_sending().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_has_been_sent(), local_is_sending()), &room_a, &room_b), + Ordering::Greater + ); + assert_eq!( + cmp(|_, _| pair(local_has_been_sent(), local_has_been_sent()), &room_a, &room_b), + Ordering::Equal + ); + assert_eq!( + cmp(|_, _| pair(local_has_been_sent(), local_cannot_be_sent()), &room_a, &room_b), + Ordering::Greater + ); + + assert_eq!( + cmp(|_, _| pair(local_cannot_be_sent(), local_is_sending()), &room_a, &room_b), Ordering::Equal ); assert_eq!( - cmp( - |_, _| (local_cannot_be_sent().is_local(), local_cannot_be_sent().is_local()), - &room_a, - &room_b - ), + cmp(|_, _| pair(local_cannot_be_sent(), local_has_been_sent()), &room_a, &room_b), + Ordering::Less + ); + assert_eq!( + cmp(|_, _| pair(local_cannot_be_sent(), local_cannot_be_sent()), &room_a, &room_b), Ordering::Equal ); } diff --git a/crates/matrix-sdk-ui/src/timeline/latest_event.rs b/crates/matrix-sdk-ui/src/timeline/latest_event.rs index 7c157aa4644..51751efddcd 100644 --- a/crates/matrix-sdk-ui/src/timeline/latest_event.rs +++ b/crates/matrix-sdk-ui/src/timeline/latest_event.rs @@ -69,12 +69,19 @@ pub enum LatestEventValue { /// The content of the local event. content: TimelineItemContent, - /// Whether the local event is sending if it is set to `true`, otherwise - /// it cannot be sent. - is_sending: bool, + /// Whether the local event is sending, has been sent or cannot be sent. + state: LatestEventValueLocalState, }, } +#[derive(Debug)] +#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))] +pub enum LatestEventValueLocalState { + IsSending, + HasBeenSent, + CannotBeSent, +} + impl LatestEventValue { pub(crate) async fn from_base_latest_event_value( value: BaseLatestEventValue, @@ -153,14 +160,11 @@ impl LatestEventValue { _ => Self::None, } } - BaseLatestEventValue::LocalIsSending(LocalLatestEventValue { - timestamp, - content: ref serialized_content, - }) - | BaseLatestEventValue::LocalCannotBeSent(LocalLatestEventValue { - timestamp, - content: ref serialized_content, - }) => { + BaseLatestEventValue::LocalIsSending(ref local_value) + | BaseLatestEventValue::LocalHasBeenSent(ref local_value) + | BaseLatestEventValue::LocalCannotBeSent(ref local_value) => { + let LocalLatestEventValue { timestamp, content: serialized_content } = local_value; + let Ok(message_like_event_content) = serialized_content.deserialize() else { return Self::None; }; @@ -172,12 +176,28 @@ impl LatestEventValue { .await .map(TimelineDetails::Ready) .unwrap_or(TimelineDetails::Unavailable); - let is_sending = matches!(value, BaseLatestEventValue::LocalIsSending(_)); match TimelineAction::from_content(message_like_event_content, None, None, None) { - TimelineAction::AddItem { content } => { - Self::Local { timestamp, sender, profile, content, is_sending } - } + TimelineAction::AddItem { content } => Self::Local { + timestamp: *timestamp, + sender, + profile, + content, + state: match value { + BaseLatestEventValue::LocalIsSending(_) => { + LatestEventValueLocalState::IsSending + } + BaseLatestEventValue::LocalHasBeenSent(_) => { + LatestEventValueLocalState::HasBeenSent + } + BaseLatestEventValue::LocalCannotBeSent(_) => { + LatestEventValueLocalState::CannotBeSent + } + BaseLatestEventValue::Remote(_) | BaseLatestEventValue::None => { + unreachable!("Only local latest events are supposed to be handled"); + } + }, + }, TimelineAction::HandleAggregation { kind, .. } => { // Add some debug logging here to help diagnose issues with the latest @@ -210,7 +230,7 @@ mod tests { use super::{ super::{MsgLikeContent, MsgLikeKind, TimelineItemContent}, - BaseLatestEventValue, LatestEventValue, TimelineDetails, + BaseLatestEventValue, LatestEventValue, LatestEventValueLocalState, TimelineDetails, }; #[async_test] @@ -275,7 +295,35 @@ mod tests { let value = LatestEventValue::from_base_latest_event_value(base_value, &room, &client).await; - assert_matches!(value, LatestEventValue::Local { timestamp, sender, profile, content, is_sending } => { + assert_matches!(value, LatestEventValue::Local { timestamp, sender, profile, content, state } => { + assert_eq!(u64::from(timestamp.get()), 42u64); + assert_eq!(sender, "@example:localhost"); + assert_matches!(profile, TimelineDetails::Unavailable); + assert_matches!( + content, + TimelineItemContent::MsgLike(MsgLikeContent { kind: MsgLikeKind::Message(_), .. }) + ); + assert_matches!(state, LatestEventValueLocalState::IsSending); + }) + } + + #[async_test] + async fn test_local_has_been_sent() { + let server = MatrixMockServer::new().await; + let client = server.client_builder().build().await; + let room = server.sync_room(&client, JoinedRoomBuilder::new(room_id!("!r0"))).await; + + let base_value = BaseLatestEventValue::LocalHasBeenSent(LocalLatestEventValue { + timestamp: MilliSecondsSinceUnixEpoch(uint!(42)), + content: SerializableEventContent::new(&AnyMessageLikeEventContent::RoomMessage( + RoomMessageEventContent::text_plain("raclette"), + )) + .unwrap(), + }); + let value = + LatestEventValue::from_base_latest_event_value(base_value, &room, &client).await; + + assert_matches!(value, LatestEventValue::Local { timestamp, sender, profile, content, state } => { assert_eq!(u64::from(timestamp.get()), 42u64); assert_eq!(sender, "@example:localhost"); assert_matches!(profile, TimelineDetails::Unavailable); @@ -283,7 +331,7 @@ mod tests { content, TimelineItemContent::MsgLike(MsgLikeContent { kind: MsgLikeKind::Message(_), .. }) ); - assert!(is_sending); + assert_matches!(state, LatestEventValueLocalState::HasBeenSent); }) } @@ -303,7 +351,7 @@ mod tests { let value = LatestEventValue::from_base_latest_event_value(base_value, &room, &client).await; - assert_matches!(value, LatestEventValue::Local { timestamp, sender, profile, content, is_sending } => { + assert_matches!(value, LatestEventValue::Local { timestamp, sender, profile, content, state } => { assert_eq!(u64::from(timestamp.get()), 42u64); assert_eq!(sender, "@example:localhost"); assert_matches!(profile, TimelineDetails::Unavailable); @@ -311,7 +359,7 @@ mod tests { content, TimelineItemContent::MsgLike(MsgLikeContent { kind: MsgLikeKind::Message(_), .. }) ); - assert!(is_sending.not()); + assert_matches!(state, LatestEventValueLocalState::CannotBeSent); }) } diff --git a/crates/matrix-sdk-ui/src/timeline/mod.rs b/crates/matrix-sdk-ui/src/timeline/mod.rs index e4f8420b8a9..af8bec3fc87 100644 --- a/crates/matrix-sdk-ui/src/timeline/mod.rs +++ b/crates/matrix-sdk-ui/src/timeline/mod.rs @@ -101,7 +101,7 @@ pub use self::{ }, event_type_filter::TimelineEventTypeFilter, item::{TimelineItem, TimelineItemKind, TimelineUniqueId}, - latest_event::LatestEventValue, + latest_event::{LatestEventValue, LatestEventValueLocalState}, traits::RoomExt, virtual_item::VirtualTimelineItem, }; diff --git a/crates/matrix-sdk/src/latest_events/latest_event.rs b/crates/matrix-sdk/src/latest_events/latest_event.rs index 6cd3156f44e..e7db41e63f5 100644 --- a/crates/matrix-sdk/src/latest_events/latest_event.rs +++ b/crates/matrix-sdk/src/latest_events/latest_event.rs @@ -526,7 +526,7 @@ mod tests_latest_event { assert_matches!( latest_event.current_value.get().await, - LatestEventValue::LocalIsSending(_) + LatestEventValue::LocalHasBeenSent(_) ); } @@ -710,13 +710,14 @@ impl LatestEventValueBuilder { // If a local previous `LatestEventValue` exists and has been marked // as “cannot be sent”, it means the new `LatestEventValue` must // also be marked as “cannot be sent”. - let value = if let Some(LatestEventValue::LocalCannotBeSent(_)) = - buffer_of_values_for_local_events.last() - { - LatestEventValue::LocalCannotBeSent(local_value) - } else { - LatestEventValue::LocalIsSending(local_value) - }; + let value = + if let Some((_, LatestEventValue::LocalCannotBeSent(_))) = + buffer_of_values_for_local_events.last() + { + LatestEventValue::LocalCannotBeSent(local_value) + } else { + LatestEventValue::LocalIsSending(local_value) + }; buffer_of_values_for_local_events .push(transaction_id.to_owned(), value.clone()); @@ -768,31 +769,34 @@ impl LatestEventValueBuilder { // values. Finally, return the last `LatestEventValue` or calculate a new // one. RoomSendQueueUpdate::SentEvent { transaction_id, .. } => { - let position = - buffer_of_values_for_local_events.mark_is_sending_after(transaction_id); - - // First, compute the new value. Then we remove the sent local event from the - // buffer. - // - // Why in this order? Because even if the Send Queue inserts the sent event in - // the Event Cache, the Send Queue sends its updates (this update) before the - // insertion in the Event Cache. Thus, the Event Cache isn't yet aware of the - // newly sent event yet at this time of execution. Computing the - // `LatestEventValue` from the local ones ensure no hide-and-seek game with the - // event before it lands in the Event Cache. - let value = Self::new_local_or_remote( + if let Some(position) = + buffer_of_values_for_local_events.mark_is_sending_after(transaction_id) + { + let (_, value) = buffer_of_values_for_local_events.remove(position); + + // The sent event is the last of the buffer. Let's compute a proper “detached” + // one if and only if the sent one was the last local one: not from the buffer, + // not from the Event Cache!. + if buffer_of_values_for_local_events.last().is_none() { + match value { + LatestEventValue::LocalIsSending(local_value) + | LatestEventValue::LocalCannotBeSent(local_value) + // Technically impossible, but it's not harmful to handle this that way. + | LatestEventValue::LocalHasBeenSent(local_value ) => { + return LatestEventValue::LocalHasBeenSent(local_value); + } + LatestEventValue::Remote(_) | LatestEventValue::None => unreachable!("Impossible to get a remote `LatestEventValue`"), + } + } + } + + Self::new_local_or_remote( buffer_of_values_for_local_events, room_event_cache, own_user_id, power_levels, ) - .await; - - if let Some(position) = position { - buffer_of_values_for_local_events.remove(position); - } - - value + .await } // A local event has been replaced by another one. @@ -891,7 +895,7 @@ impl LatestEventValueBuilder { own_user_id: &UserId, power_levels: Option<&RoomPowerLevels>, ) -> LatestEventValue { - if let Some(value) = buffer_of_values_for_local_events.last() { + if let Some((_, value)) = buffer_of_values_for_local_events.last() { value.clone() } else { Self::new_remote(room_event_cache, own_user_id, power_levels).await @@ -955,8 +959,8 @@ impl LatestEventValuesForLocalEvents { } /// Get the last [`LatestEventValue`]. - fn last(&self) -> Option<&LatestEventValue> { - self.buffer.last().map(|(_, value)| value) + fn last(&self) -> Option<&(OwnedTransactionId, LatestEventValue)> { + self.buffer.last() } /// Find the position of the [`LatestEventValue`] matching `transaction_id`. @@ -973,13 +977,7 @@ impl LatestEventValuesForLocalEvents { /// Panics if `value` is not of kind [`LatestEventValue::LocalIsSending`] or /// [`LatestEventValue::LocalCannotBeSent`]. fn push(&mut self, transaction_id: OwnedTransactionId, value: LatestEventValue) { - assert!( - matches!( - value, - LatestEventValue::LocalIsSending(_) | LatestEventValue::LocalCannotBeSent(_) - ), - "`value` must be either `LocalIsSending` or `LocalCannotBeSent`" - ); + assert!(value.is_local(), "`value` must be a local `LatestEventValue`"); self.buffer.push((transaction_id, value)); } @@ -1621,12 +1619,18 @@ mod tests_latest_event_values_for_local_events { assert!(buffer.last().is_none()); + let transaction_id = OwnedTransactionId::from("txnid"); buffer.push( - OwnedTransactionId::from("txnid"), + transaction_id.clone(), LatestEventValue::LocalIsSending(local_room_message("tome")), ); - assert_matches!(buffer.last(), Some(LatestEventValue::LocalIsSending(_))); + assert_matches!( + buffer.last(), + Some((expected_transaction_id, LatestEventValue::LocalIsSending(_))) => { + assert_eq!(expected_transaction_id, &transaction_id); + } + ); } #[test] @@ -1679,16 +1683,51 @@ mod tests_latest_event_values_for_local_events { OwnedTransactionId::from("txnid1"), LatestEventValue::LocalCannotBeSent(local_room_message("raclette")), ); + buffer.push( + OwnedTransactionId::from("txnid1"), + LatestEventValue::LocalHasBeenSent(local_room_message("raclette")), + ); // no panic. } #[test] - fn test_replace_content() { + #[should_panic] + fn test_replace_content_on_remote() { let mut buffer = LatestEventValuesForLocalEvents::new(); buffer.push( - OwnedTransactionId::from("txnid0"), + OwnedTransactionId::from("txnid"), + LatestEventValue::Remote(remote_room_message("gruyère")), + ); + + let LocalLatestEventValue { content: new_content, .. } = local_room_message("comté"); + + buffer.replace_content(0, new_content); + } + + #[test] + #[should_panic] + fn test_replace_content_on_local_has_been_sent() { + let mut buffer = LatestEventValuesForLocalEvents::new(); + + buffer.push( + OwnedTransactionId::from("txnid"), + LatestEventValue::LocalHasBeenSent(local_room_message("gruyère")), + ); + + let LocalLatestEventValue { content: new_content, .. } = local_room_message("comté"); + + buffer.replace_content(0, new_content); + } + + #[test] + fn test_replace_content_on_local_is_sending() { + let mut buffer = LatestEventValuesForLocalEvents::new(); + + let transaction_id = OwnedTransactionId::from("txnid0"); + buffer.push( + transaction_id.clone(), LatestEventValue::LocalIsSending(local_room_message("gruyère")), ); @@ -1698,7 +1737,36 @@ mod tests_latest_event_values_for_local_events { assert_matches!( buffer.last(), - Some(LatestEventValue::LocalIsSending(local_event)) => { + Some((expected_transaction_id, LatestEventValue::LocalIsSending(local_event))) => { + assert_eq!(expected_transaction_id, &transaction_id); + assert_matches!( + local_event.content.deserialize().unwrap(), + AnyMessageLikeEventContent::RoomMessage(content) => { + assert_eq!(content.body(), "comté"); + } + ); + } + ); + } + + #[test] + fn test_replace_content_on_local_cannot_be_sent() { + let mut buffer = LatestEventValuesForLocalEvents::new(); + + let transaction_id = OwnedTransactionId::from("txnid0"); + buffer.push( + transaction_id.clone(), + LatestEventValue::LocalCannotBeSent(local_room_message("gruyère")), + ); + + let LocalLatestEventValue { content: new_content, .. } = local_room_message("comté"); + + buffer.replace_content(0, new_content); + + assert_matches!( + buffer.last(), + Some((expected_transaction_id, LatestEventValue::LocalCannotBeSent(local_event))) => { + assert_eq!(expected_transaction_id, &transaction_id); assert_matches!( local_event.content.deserialize().unwrap(), AnyMessageLikeEventContent::RoomMessage(content) => { @@ -2226,7 +2294,8 @@ mod tests_latest_event_value_builder { } // Receiving a `SentEvent` targeting the first event. The `LatestEventValue` - // hasn't changed, this is still this event. + // hasn't changed, this is still this event, but the status has changed to + // `LocalHasBeenSent`. { let update = RoomSendQueueUpdate::SentEvent { transaction_id: transaction_id_1, @@ -2236,7 +2305,7 @@ mod tests_latest_event_value_builder { // The `LatestEventValue` hasn't changed. assert_local_value_matches_room_message_with_body!( LatestEventValueBuilder::new_local(&update, &mut buffer, &room_event_cache, user_id, None).await, - LatestEventValue::LocalIsSending => with body = "B" + LatestEventValue::LocalHasBeenSent => with body = "B" ); assert!(buffer.buffer.is_empty()); @@ -2632,7 +2701,8 @@ mod tests_latest_event_value_builder { // We get a `Remote` because there is no `Local*` values! assert_remote_value_matches_room_message_with_body!( LatestEventValueBuilder::new_local( - // An update that won't be create a new `LatestEventValue`. + // An update that won't create a new `LatestEventValue`: it maps + // to zero existing local value. &RoomSendQueueUpdate::SentEvent { transaction_id: OwnedTransactionId::from("txnid"), event_id: event_id_1.to_owned(), @@ -2643,7 +2713,7 @@ mod tests_latest_event_value_builder { None, ) .await - => with body = "hello" + => with body = "hello" ); } }