feat(ui): don't mark each thread reply as an actual reply to the previous message, in threaded timelines

This correctly handles the reply fallback behavior:

- the behavior isn't changed for a live timeline,
- when a timeline is thread-focused, we will extract the `replied_to`
field if and only if the thread relation is *not* marked as behaving in
a fallback manner.

This makes it possible to distinguish actual in-thread replies.

Author: bnjbvr

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

@@ -38,6 +38,7 @@ use super::{
 };
 use crate::{
     timeline::{
+        controller::TimelineFocusKind,
         event_item::{
             extract_bundled_edit_event_json, extract_poll_edit_content,
             extract_room_msg_edit_content,
@@ -316,6 +317,7 @@ impl TimelineMetadata {
         raw_event: &Raw<AnySyncTimelineEvent>,
         bundled_edit_encryption_info: Option<Arc<EncryptionInfo>>,
         timeline_items: &Vector<Arc<TimelineItem>>,
+        timeline_focus: &TimelineFocusKind,
     ) -> (Option<InReplyToDetails>, Option<OwnedEventId>) {
         if let AnySyncTimelineEvent::MessageLike(ev) = event {
             if let Some(content) = ev.original_content() {
@@ -325,7 +327,12 @@ impl TimelineMetadata {
                     relations: ev.relations(),
                     bundled_edit_encryption_info,
                 });
-                return self.process_content_relations(&content, remote_ctx, timeline_items);
+                return self.process_content_relations(
+                    &content,
+                    remote_ctx,
+                    timeline_items,
+                    timeline_focus,
+                );
             }
         }
         (None, None)
@@ -341,12 +348,14 @@ impl TimelineMetadata {
         content: &AnyMessageLikeEventContent,
         remote_ctx: Option<RemoteEventContext<'_>>,
         timeline_items: &Vector<Arc<TimelineItem>>,
+        timeline_focus: &TimelineFocusKind,
     ) -> (Option<InReplyToDetails>, Option<OwnedEventId>) {
         match content {
             AnyMessageLikeEventContent::Sticker(content) => {
                 let (in_reply_to, thread_root) = Self::extract_reply_and_thread_root(
                     content.relates_to.clone().and_then(|rel| rel.try_into().ok()),
                     timeline_items,
+                    timeline_focus,
                 );
 
                 if let Some(event_id) = remote_ctx.map(|ctx| ctx.event_id) {
@@ -359,8 +368,11 @@ impl TimelineMetadata {
             AnyMessageLikeEventContent::UnstablePollStart(UnstablePollStartEventContent::New(
                 c,
             )) => {
-                let (in_reply_to, thread_root) =
-                    Self::extract_reply_and_thread_root(c.relates_to.clone(), timeline_items);
+                let (in_reply_to, thread_root) = Self::extract_reply_and_thread_root(
+                    c.relates_to.clone(),
+                    timeline_items,
+                    timeline_focus,
+                );
 
                 // Record the bundled edit in the aggregations set, if any.
                 if let Some(ctx) = remote_ctx {
@@ -398,6 +410,7 @@ impl TimelineMetadata {
                 let (in_reply_to, thread_root) = Self::extract_reply_and_thread_root(
                     msg.relates_to.clone().and_then(|rel| rel.try_into().ok()),
                     timeline_items,
+                    timeline_focus,
                 );
 
                 // Record the bundled edit in the aggregations set, if any.
@@ -441,6 +454,7 @@ impl TimelineMetadata {
     fn extract_reply_and_thread_root(
         relates_to: Option<RelationWithoutReplacement>,
         timeline_items: &Vector<Arc<TimelineItem>>,
+        timeline_focus: &TimelineFocusKind,
     ) -> (Option<InReplyToDetails>, Option<OwnedEventId>) {
         let mut thread_root = None;
 
@@ -450,9 +464,25 @@ impl TimelineMetadata {
             }
             RelationWithoutReplacement::Thread(thread) => {
                 thread_root = Some(thread.event_id);
-                thread
-                    .in_reply_to
-                    .map(|in_reply_to| InReplyToDetails::new(in_reply_to.event_id, timeline_items))
+
+                if matches!(timeline_focus, TimelineFocusKind::Thread { .. })
+                    && thread.is_falling_back
+                {
+                    // In general, a threaded event is marked as a response to the previous message
+                    // in the thread, to maintain backwards compatibility with clients not
+                    // supporting threads.
+                    //
+                    // But we can have actual replies to other in-thread events. The
+                    // `is_falling_back` bool helps distinguishing both use cases.
+                    //
+                    // If this timeline is thread-focused, we only mark non-falling-back replies as
+                    // actual in-thread replies.
+                    None
+                } else {
+                    thread.in_reply_to.map(|in_reply_to| {
+                        InReplyToDetails::new(in_reply_to.event_id, timeline_items)
+                    })
+                }
             }
             _ => None,
         });

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

@@ -165,7 +165,7 @@ impl TimelineState {
         let mut date_divider_adjuster = DateDividerAdjuster::new(date_divider_mode);
 
         let (in_reply_to, thread_root) =
-            txn.meta.process_content_relations(&content, None, &txn.items);
+            txn.meta.process_content_relations(&content, None, &txn.items, &txn.timeline_focus);
 
         // TODO merge with other should_add, one way or another?
         let should_add_new_items = match &txn.timeline_focus {

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

@@ -626,6 +626,7 @@ impl<'a> TimelineStateTransaction<'a> {
                     &raw,
                     bundled_edit_encryption_info,
                     &self.items,
+                    &self.timeline_focus,
                 );
 
                 let should_add = self.should_add_event_item(

crates/matrix-sdk-ui/tests/integration/timeline/thread.rs

@@ -102,7 +102,7 @@ async fn test_thread_backpagination() {
         factory
             .text_msg("Threaded event 4")
             .event_id(event_id!("$4"))
-            .in_thread(&thread_root_event_id, event_id!("$3"))
+            .in_thread_reply(&thread_root_event_id, event_id!("$2"))
             .into_raw_sync()
             .cast(),
         factory
@@ -159,14 +159,17 @@ async fn test_thread_backpagination() {
     assert_eq!(items.len(), 2 + 1); //  A date divider + the 2 events
     assert!(items[0].is_date_divider());
 
-    assert_eq!(
-        items[1].as_event().unwrap().content().as_message().unwrap().body(),
-        "Threaded event 3"
-    );
-    assert_eq!(
-        items[2].as_event().unwrap().content().as_message().unwrap().body(),
-        "Threaded event 4"
-    );
+    let event_item = items[1].as_event().unwrap();
+    assert_eq!(event_item.content().as_message().unwrap().body(), "Threaded event 3");
+    // In a threaded timeline, threads aren't using the reply fallback, unless
+    // they're an actual reply to another thread event.
+    assert_matches!(event_item.content().in_reply_to(), None);
+
+    let event_item = items[2].as_event().unwrap();
+    assert_eq!(event_item.content().as_message().unwrap().body(), "Threaded event 4");
+    // But this one is an actual reply to another thread event, so it has the
+    // replied-to event correctly set.
+    assert_eq!(event_item.content().in_reply_to().unwrap().event_id, event_id!("$2"));
 
     let hit_start = timeline.paginate_backwards(100).await.unwrap();
     assert!(hit_start);
@@ -181,12 +184,12 @@ async fn test_thread_backpagination() {
     assert_let!(VectorDiff::PushFront { value } = &timeline_updates[0]);
     let event_item = value.as_event().unwrap();
     assert_eq!(event_item.event_id().unwrap(), event_id!("$2"));
-    assert_eq!(event_item.content().in_reply_to().unwrap().event_id, event_id!("$1"));
+    assert_matches!(event_item.content().in_reply_to(), None);
 
     assert_let!(VectorDiff::PushFront { value } = &timeline_updates[1]);
     let event_item = value.as_event().unwrap();
     assert_eq!(event_item.event_id().unwrap(), event_id!("$1"));
-    assert_eq!(event_item.content().in_reply_to().unwrap().event_id, event_id!("$root"));
+    assert_matches!(event_item.content().in_reply_to(), None);
 
     assert_let!(VectorDiff::PushFront { value } = &timeline_updates[2]);
     assert_eq!(value.as_event().unwrap().event_id().unwrap(), event_id!("$root"));

testing/matrix-sdk-test/src/event_factory.rs

@@ -345,14 +345,22 @@ impl EventBuilder<RoomMessageEventContent> {
         self
     }
 
-    /// Adds a thread relation to the root event, setting the latest thread
-    /// event id too.
+    /// Adds a thread relation to the root event, setting the reply fallback to
+    /// the latest in-thread event.
     pub fn in_thread(mut self, root: &EventId, latest_thread_event: &EventId) -> Self {
         self.content.relates_to =
             Some(Relation::Thread(Thread::plain(root.to_owned(), latest_thread_event.to_owned())));
         self
     }
 
+    /// Adds a thread relation to the root event, that's a non-fallback reply to
+    /// another thread event.
+    pub fn in_thread_reply(mut self, root: &EventId, replied_to: &EventId) -> Self {
+        self.content.relates_to =
+            Some(Relation::Thread(Thread::reply(root.to_owned(), replied_to.to_owned())));
+        self
+    }
+
     /// Adds a replacement relation to the current event, with the new content
     /// passed.
     pub fn edit(