timeline: don't insert a read marker when all subsequent events have been inserted by ourselves

Author: bnjbvr

crates/matrix-sdk-ui/src/timeline/controller/mod.rs

@@ -272,6 +272,7 @@ impl<P: RoomDataProvider> TimelineController<P> {
 
         let state = TimelineState::new(
             focus_kind,
+            room_data_provider.own_user_id().to_owned(),
             room_data_provider.room_version(),
             internal_id_prefix,
             unable_to_decrypt_hook,

crates/matrix-sdk-ui/src/timeline/controller/state.rs

@@ -70,6 +70,7 @@ pub(in crate::timeline) struct TimelineState {
 impl TimelineState {
     pub(super) fn new(
         timeline_focus: TimelineFocusKind,
+        own_user_id: OwnedUserId,
         room_version: RoomVersionId,
         internal_id_prefix: Option<String>,
         unable_to_decrypt_hook: Option<Arc<UtdHookManager>>,
@@ -81,6 +82,7 @@ impl TimelineState {
             // small enough.
             items: ObservableVector::with_capacity(32),
             meta: TimelineMetadata::new(
+                own_user_id,
                 room_version,
                 internal_id_prefix,
                 unable_to_decrypt_hook,
@@ -691,6 +693,9 @@ pub(in crate::timeline) struct TimelineMetadata {
     /// This value is constant over the lifetime of the metadata.
     pub room_version: RoomVersionId,
 
+    /// The own [`OwnedUserId`] of the client who opened the timeline.
+    own_user_id: OwnedUserId,
+
     // **** DYNAMIC FIELDS ****
     /// The next internal identifier for timeline items, used for both local and
     /// remote echoes.
@@ -717,12 +722,14 @@ pub(in crate::timeline) struct TimelineMetadata {
 
 impl TimelineMetadata {
     pub(crate) fn new(
+        own_user_id: OwnedUserId,
         room_version: RoomVersionId,
         internal_id_prefix: Option<String>,
         unable_to_decrypt_hook: Option<Arc<UtdHookManager>>,
         is_room_encrypted: bool,
     ) -> Self {
         Self {
+            own_user_id,
             all_events: Default::default(),
             next_internal_id: Default::default(),
             reactions: Default::default(),
@@ -805,7 +812,38 @@ impl TimelineMetadata {
         trace!(?fully_read_event, "Updating read marker");
 
         let read_marker_idx = items.iter().rposition(|item| item.is_read_marker());
-        let fully_read_event_idx = rfind_event_by_id(items, fully_read_event).map(|(idx, _)| idx);
+
+        let mut fully_read_event_idx =
+            rfind_event_by_id(items, fully_read_event).map(|(idx, _)| idx);
+
+        if let Some(i) = &mut fully_read_event_idx {
+            // The item at position `i` is the first item that's fully read, we're about to
+            // insert a read marker just after it.
+            //
+            // Do another forward pass to skip all the events we've sent too.
+
+            // Find the position of the first element…
+            let next = items
+                .iter()
+                .enumerate()
+                // …strictly *after* the fully read event…
+                .skip(*i + 1)
+                // …that's not virtual and not sent by us…
+                .find(|(_, item)| {
+                    item.as_event().map_or(false, |event| event.sender() != self.own_user_id)
+                })
+                .map(|(i, _)| i);
+
+            if let Some(next) = next {
+                // `next` point to the first item that's not sent by us, so the *previous* of
+                // next is the right place where to insert the fully read marker.
+                *i = next.wrapping_sub(1);
+            } else {
+                // There's no event after the read marker that's not sent by us, i.e. the full
+                // timeline has been read: the fully read marker goes to the end.
+                *i = items.len().wrapping_sub(1);
+            }
+        }
 
         match (read_marker_idx, fully_read_event_idx) {
             (None, None) => {

crates/matrix-sdk-ui/src/timeline/day_dividers.rs

@@ -642,12 +642,16 @@ mod tests {
         )
     }
 
+    fn test_metadata() -> TimelineMetadata {
+        TimelineMetadata::new(owned_user_id!("@a:b.c"), ruma::RoomVersionId::V11, None, None, false)
+    }
+
     #[test]
     fn test_no_trailing_day_divider() {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
         let timestamp_next_day =
@@ -681,7 +685,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
         let timestamp_next_day =
@@ -713,7 +717,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
         let timestamp_next_day =
@@ -747,7 +751,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
         let timestamp_next_day =
@@ -777,7 +781,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
 
@@ -803,7 +807,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
+        let mut meta = test_metadata();
 
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
 
@@ -827,8 +831,7 @@ mod tests {
         let mut items = ObservableVector::new();
         let mut txn = items.transaction();
 
-        let mut meta = TimelineMetadata::new(ruma::RoomVersionId::V11, None, None, false);
-
+        let mut meta = test_metadata();
         let timestamp = MilliSecondsSinceUnixEpoch(uint!(42));
 
         txn.push_back(meta.new_timeline_item(VirtualTimelineItem::ReadMarker));

crates/matrix-sdk-ui/src/timeline/tests/echo.rs

@@ -267,7 +267,7 @@ async fn test_day_divider_removed_after_local_echo_disappeared() {
 }
 
 #[async_test]
-async fn test_read_marker_removed_after_local_echo_disappeared() {
+async fn test_no_read_marker_with_local_echo() {
     let event_id = event_id!("$1");
 
     let timeline = TestTimeline::with_room_data_provider(
@@ -306,11 +306,10 @@ async fn test_read_marker_removed_after_local_echo_disappeared() {
 
     let items = timeline.controller.items().await;
 
-    assert_eq!(items.len(), 4);
+    assert_eq!(items.len(), 3);
     assert!(items[0].is_day_divider());
     assert!(items[1].is_remote_event());
-    assert!(items[2].is_read_marker());
-    assert!(items[3].is_local_echo());
+    assert!(items[2].is_local_echo());
 
     // Cancel the local echo.
     timeline

crates/matrix-sdk-ui/src/timeline/tests/virt.rs

@@ -16,6 +16,7 @@ use assert_matches::assert_matches;
 use assert_matches2::assert_let;
 use chrono::{Datelike, Local, TimeZone};
 use eyeball_im::VectorDiff;
+use futures_util::{FutureExt, StreamExt as _};
 use matrix_sdk_test::{async_test, ALICE, BOB};
 use ruma::{
     event_id,
@@ -24,7 +25,7 @@ use ruma::{
 use stream_assert::assert_next_matches;
 
 use super::TestTimeline;
-use crate::timeline::{TimelineItemKind, VirtualTimelineItem};
+use crate::timeline::{traits::RoomDataProvider as _, TimelineItemKind, VirtualTimelineItem};
 
 #[async_test]
 async fn test_day_divider() {
@@ -91,60 +92,99 @@ async fn test_update_read_marker() {
     let timeline = TestTimeline::new();
     let mut stream = timeline.subscribe().await;
 
-    let f = &timeline.factory;
+    let own_user = timeline.controller.room_data_provider.own_user_id().to_owned();
 
-    timeline.handle_live_event(f.text_msg("A").sender(&ALICE)).await;
+    let f = &timeline.factory;
+    timeline.handle_live_event(f.text_msg("A").sender(&own_user)).await;
 
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
-    let first_event_id = item.as_event().unwrap().event_id().unwrap().to_owned();
+    let event_id1 = item.as_event().unwrap().event_id().unwrap().to_owned();
 
     let day_divider = assert_next_matches!(stream, VectorDiff::PushFront { value } => value);
     assert!(day_divider.is_day_divider());
 
-    timeline.controller.handle_fully_read_marker(first_event_id.clone()).await;
+    timeline.controller.handle_fully_read_marker(event_id1.to_owned()).await;
 
     // Nothing should happen, the marker cannot be added at the end.
 
     timeline.handle_live_event(f.text_msg("B").sender(&BOB)).await;
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
-    let second_event_id = item.as_event().unwrap().event_id().unwrap().to_owned();
+    let event_id2 = item.as_event().unwrap().event_id().unwrap().to_owned();
 
     // Now the read marker appears after the first event.
     let item = assert_next_matches!(stream, VectorDiff::Insert { index: 2, value } => value);
     assert_matches!(item.as_virtual(), Some(VirtualTimelineItem::ReadMarker));
 
-    timeline.controller.handle_fully_read_marker(second_event_id.clone()).await;
+    timeline.controller.handle_fully_read_marker(event_id2.clone()).await;
 
     // The read marker is removed but not reinserted, because it cannot be added at
     // the end.
     assert_next_matches!(stream, VectorDiff::Remove { index: 2 });
 
-    timeline.handle_live_event(f.text_msg("C").sender(&ALICE)).await;
+    timeline.handle_live_event(f.text_msg("C").sender(&BOB)).await;
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
-    let third_event_id = item.as_event().unwrap().event_id().unwrap().to_owned();
+    let event_id3 = item.as_event().unwrap().event_id().unwrap().to_owned();
 
     // Now the read marker is reinserted after the second event.
     let marker = assert_next_matches!(stream, VectorDiff::Insert { index: 3, value } => value);
     assert_matches!(marker.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker));
 
-    // Nothing should happen if the fully read event is set back to an older event.
-    timeline.controller.handle_fully_read_marker(first_event_id).await;
+    // Nothing should happen if the fully read event is set back to an older event
+    // sent by another user.
+    timeline.controller.handle_fully_read_marker(event_id1.to_owned()).await;
+    assert!(stream.next().now_or_never().is_none());
 
     // Nothing should happen if the fully read event isn't found.
     timeline.controller.handle_fully_read_marker(event_id!("$fake_event_id").to_owned()).await;
+    assert!(stream.next().now_or_never().is_none());
 
     // Nothing should happen if the fully read event is referring to an event
     // that has already been marked as fully read.
-    timeline.controller.handle_fully_read_marker(second_event_id).await;
+    timeline.controller.handle_fully_read_marker(event_id2).await;
+    assert!(stream.next().now_or_never().is_none());
 
-    timeline.handle_live_event(f.text_msg("D").sender(&ALICE)).await;
+    timeline.handle_live_event(f.text_msg("D").sender(&BOB)).await;
     let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
-    item.as_event().unwrap();
+    let event_id4 = item.as_event().unwrap().event_id().unwrap().to_owned();
 
-    timeline.controller.handle_fully_read_marker(third_event_id).await;
+    timeline.controller.handle_fully_read_marker(event_id3).await;
 
-    // The read marker is moved after the third event.
+    // The read marker is moved after the third event (sent by another user).
     assert_next_matches!(stream, VectorDiff::Remove { index: 3 });
     let marker = assert_next_matches!(stream, VectorDiff::Insert { index: 4, value } => value);
     assert_matches!(marker.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker));
+
+    // If the current user sends an event afterwards, the read marker doesn't move.
+    timeline.handle_live_event(f.text_msg("E").sender(&own_user)).await;
+    let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
+    item.as_event().unwrap();
+
+    assert!(stream.next().now_or_never().is_none());
+
+    // If the marker moved forward to another user's event, and there's no other
+    // event sent from another user, then it will be removed.
+    timeline.controller.handle_fully_read_marker(event_id4).await;
+    assert_next_matches!(stream, VectorDiff::Remove { index: 4 });
+
+    assert!(stream.next().now_or_never().is_none());
+
+    // When a last event is inserted by ourselves, still no read marker.
+    timeline.handle_live_event(f.text_msg("F").sender(&own_user)).await;
+    let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
+    item.as_event().unwrap();
+
+    timeline.handle_live_event(f.text_msg("G").sender(&own_user)).await;
+    let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
+    item.as_event().unwrap();
+
+    assert!(stream.next().now_or_never().is_none());
+
+    // But when it's another user who sent the event, then we get a read marker for
+    // their message.
+    timeline.handle_live_event(f.text_msg("H").sender(&BOB)).await;
+    let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
+    item.as_event().unwrap();
+
+    let marker = assert_next_matches!(stream, VectorDiff::Insert { index: 8, value } => value);
+    assert_matches!(marker.kind, TimelineItemKind::Virtual(VirtualTimelineItem::ReadMarker));
 }