read receipts: move the unread messages and mentions counts to separa…

…te fields of `RoomInfo`

bindings/matrix-sdk-ffi/src/room_info.rs

@@ -31,6 +31,8 @@ pub struct RoomInfo {
     user_defined_notification_mode: Option<RoomNotificationMode>,
     has_room_call: bool,
     active_room_call_participants: Vec<String>,
+    num_unread_messages: u64,
+    num_unread_mentions: u64,
 }
 
 impl RoomInfo {
@@ -75,6 +77,8 @@ impl RoomInfo {
                 .iter()
                 .map(|u| u.to_string())
                 .collect(),
+            num_unread_messages: room.num_unread_messages(),
+            num_unread_mentions: room.num_unread_mentions(),
         })
     }
 }

crates/matrix-sdk-base/src/read_receipts.rs

@@ -10,7 +10,7 @@ use ruma::{
     serde::Raw,
     EventId, OwnedEventId, RoomId, UserId,
 };
-use tracing::{field::display, instrument, trace};
+use tracing::{instrument, trace};
 
 use super::BaseClient;
 use crate::{error::Result, store::StateChanges, RoomInfo};
@@ -29,24 +29,16 @@ impl PreviousEventsProvider for () {
     }
 }
 
-#[instrument(skip_all, fields(room_id))]
+#[instrument(skip_all, fields(room_id = %room_info.room_id))]
 pub(crate) async fn compute_notifications(
     client: &BaseClient,
     changes: &StateChanges,
     previous_events_provider: &dyn PreviousEventsProvider,
     new_events: &[SyncTimelineEvent],
     room_info: &mut RoomInfo,
 ) -> Result<()> {
-    // Only apply the algorithm to encrypted rooms, since unencrypted rooms' unread
-    // notification counts ought to be properly computed by the server.
-    if !room_info.is_encrypted() {
-        return Ok(());
-    }
-
-    tracing::Span::current().record("room_id", display(&room_info.room_id));
-
     let user_id = &client.session_meta().unwrap().user_id;
-    let prev_latest_receipt_event_id = room_info.latest_read_receipt_event_id.clone();
+    let prev_latest_receipt_event_id = room_info.read_receipts.latest_read_receipt_event_id.clone();
 
     if let Some(receipt_event) = changes.receipts.get(room_info.room_id()) {
         trace!("Got a new receipt event!");
@@ -76,7 +68,7 @@ pub(crate) async fn compute_notifications(
             // about.
 
             // First, save the event id as the latest one that has a read receipt.
-            room_info.latest_read_receipt_event_id = Some(receipt_event_id.clone());
+            room_info.read_receipts.latest_read_receipt_event_id = Some(receipt_event_id.clone());
 
             // Try to find if the read receipts refers to an event from the current sync, to
             // avoid searching the cached timeline events.
@@ -124,17 +116,28 @@ pub(crate) async fn compute_notifications(
     // for the next receipt.
     trace!("All other ways failed, including all new events for the receipts count.");
     for event in new_events {
-        if event.push_actions.iter().any(ruma::push::Action::is_highlight) {
-            room_info.notification_counts.highlight_count += 1;
-        }
-        if marks_as_unread(&event.event, user_id) {
-            room_info.notification_counts.notification_count += 1;
-        }
+        count_unread_and_mentions(event, user_id, room_info);
     }
 
     Ok(())
 }
 
+#[inline(always)]
+fn count_unread_and_mentions(
+    event: &SyncTimelineEvent,
+    user_id: &UserId,
+    room_info: &mut RoomInfo,
+) {
+    for action in &event.push_actions {
+        if action.should_notify() && marks_as_unread(&event.event, user_id) {
+            room_info.read_receipts.num_unread += 1;
+        }
+        if action.is_highlight() {
+            room_info.read_receipts.num_mentions += 1;
+        }
+    }
+}
+
 /// Try to find the event to which the receipt attaches to, and if found, will
 /// update the notification count in the room.
 ///
@@ -148,20 +151,14 @@ fn find_and_count_events<'a>(
     let mut counting_receipts = false;
     for event in events {
         if counting_receipts {
-            for action in &event.push_actions {
-                if action.is_highlight() {
-                    room_info.notification_counts.highlight_count += 1;
-                }
-                if action.should_notify() && marks_as_unread(&event.event, user_id) {
-                    room_info.notification_counts.notification_count += 1;
-                }
-            }
+            count_unread_and_mentions(event, user_id, room_info);
         } else if let Ok(Some(event_id)) = event.event.get_field::<OwnedEventId>("event_id") {
             if event_id == receipt_event_id {
                 // Bingo! Switch over to the counting state, after resetting the
                 // previous counts.
                 trace!("Found the event the receipt was referring to! Starting to count.");
-                room_info.notification_counts = Default::default();
+                room_info.read_receipts.num_unread = 0;
+                room_info.read_receipts.num_mentions = 0;
                 counting_receipts = true;
             }
         }

crates/matrix-sdk-base/src/rooms/normal.rs

@@ -187,6 +187,23 @@ impl Room {
         self.inner.read().notification_counts
     }
 
+    /// Get the number of unread messages (computed client-side).
+    ///
+    /// This might be more precise than [`Self::unread_notification_counts`] for
+    /// encrypted rooms.
+    pub fn num_unread_messages(&self) -> u64 {
+        self.inner.read().read_receipts.num_unread
+    }
+
+    /// Get the number of unread mentions (computed client-side), that is,
+    /// messages causing a highlight in a room.
+    ///
+    /// This might be more precise than [`Self::unread_notification_counts`] for
+    /// encrypted rooms.
+    pub fn num_unread_mentions(&self) -> u64 {
+        self.inner.read().read_receipts.num_mentions
+    }
+
     /// Check if the room has its members fully synced.
     ///
     /// Members might be missing if lazy member loading was enabled for the
@@ -710,6 +727,22 @@ impl Room {
     }
 }
 
+/// Information about read receipts collected during processing of that room.
+#[derive(Clone, Debug, Serialize, Deserialize, Default)]
+pub struct RoomReadReceipts {
+    /// Does the room have unread messages?
+    pub(crate) num_unread: u64,
+
+    /// Does the room have messages causing highlights for the users? (aka
+    /// mentions)
+    pub(crate) num_mentions: u64,
+
+    /// The id of the event the last unthreaded (or main-threaded, for better
+    /// compatibility with clients that have thread support) read receipt is
+    /// attached to.
+    pub(crate) latest_read_receipt_event_id: Option<OwnedEventId>,
+}
+
 /// The underlying pure data structure for joined and left rooms.
 ///
 /// Holds all the info needed to persist a room into the state store.
@@ -721,7 +754,10 @@ pub struct RoomInfo {
     /// The state of the room.
     pub(crate) room_state: RoomState,
 
-    /// The unread notifications counts.
+    /// The unread notifications counts, as returned by the server.
+    ///
+    /// These might be incorrect for encrypted rooms, since the server doesn't
+    /// have access to the content of the encrypted events.
     pub(crate) notification_counts: UnreadNotificationsCount,
 
     /// The summary of this room.
@@ -743,11 +779,9 @@ pub struct RoomInfo {
     #[cfg(feature = "experimental-sliding-sync")]
     pub(crate) latest_event: Option<Box<LatestEvent>>,
 
-    /// The id of the event the last unthreaded (or main-threaded, for better
-    /// compatibility with clients that have thread support) read receipt is
-    /// attached to.
+    /// Information about read receipts for this room.
     #[serde(default)]
-    pub(crate) latest_read_receipt_event_id: Option<OwnedEventId>,
+    pub(crate) read_receipts: RoomReadReceipts,
 
     /// Base room info which holds some basic event contents important for the
     /// room state.
@@ -785,7 +819,7 @@ impl RoomInfo {
             encryption_state_synced: false,
             #[cfg(feature = "experimental-sliding-sync")]
             latest_event: None,
-            latest_read_receipt_event_id: None,
+            read_receipts: Default::default(),
             base_info: Box::new(BaseRoomInfo::new()),
         }
     }
@@ -1267,7 +1301,7 @@ mod tests {
                 Raw::from_json_string(json!({"sender": "@u:i.uk"}).to_string()).unwrap().into(),
             ))),
             base_info: Box::new(BaseRoomInfo::new()),
-            latest_read_receipt_event_id: None,
+            read_receipts: Default::default(),
         };
 
         let info_json = json!({
@@ -1307,6 +1341,11 @@ mod tests {
                 "name": null,
                 "tombstone": null,
                 "topic": null,
+            },
+            "read_receipts": {
+                "num_unread": 0,
+                "num_mentions": 0,
+                "latest_read_receipt_event_id": null,
             }
         });
 

crates/matrix-sdk-base/src/store/migration_helpers.rs

@@ -118,7 +118,7 @@ impl RoomInfoV1 {
             encryption_state_synced,
             #[cfg(feature = "experimental-sliding-sync")]
             latest_event: latest_event.map(|ev| Box::new(LatestEvent::new(ev))),
-            latest_read_receipt_event_id: None,
+            read_receipts: Default::default(),
             base_info: base_info.migrate(create),
         }
     }

testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs

@@ -304,9 +304,8 @@ async fn test_room_notification_count() -> Result<()> {
     let mut info_updates = alice_room.subscribe_info();
 
     // At first, nothing has happened, so we shouldn't have any notifications.
-    let count = alice_room.unread_notification_counts();
-    assert_eq!(count.highlight_count, 0);
-    assert_eq!(count.notification_count, 0);
+    assert_eq!(alice_room.num_unread_messages(), 0);
+    assert_eq!(alice_room.num_unread_mentions(), 0);
 
     assert_pending!(info_updates);
 
@@ -315,9 +314,8 @@ async fn test_room_notification_count() -> Result<()> {
 
     assert!(info_updates.next().await.is_some());
 
-    let count = alice_room.unread_notification_counts();
-    assert_eq!(count.highlight_count, 0);
-    assert_eq!(count.notification_count, 0);
+    assert_eq!(alice_room.num_unread_messages(), 0);
+    assert_eq!(alice_room.num_unread_mentions(), 0);
     assert!(alice_room.latest_event().is_none());
 
     assert_pending!(info_updates);
@@ -329,10 +327,8 @@ async fn test_room_notification_count() -> Result<()> {
 
     assert!(info_updates.next().await.is_some());
 
-    let count = alice_room.unread_notification_counts();
-    assert_eq!(count.highlight_count, 0);
-    assert_eq!(count.notification_count, 1);
-    let mut prev_count = count;
+    assert_eq!(alice_room.num_unread_messages(), 1);
+    assert_eq!(alice_room.num_unread_mentions(), 0);
 
     assert_pending!(info_updates);
 
@@ -348,17 +344,16 @@ async fn test_room_notification_count() -> Result<()> {
     loop {
         assert!(info_updates.next().await.is_some());
 
-        let count = alice_room.unread_notification_counts();
-        if count == prev_count {
-            // Sometimes we get notified for changes to unrelated, other fields of
-            // `info_updates`.
+        // FIXME we receive multiple spurious room info updates.
+        if alice_room.num_unread_messages() == 1 && alice_room.num_unread_mentions() == 0 {
             tracing::warn!("ignoring");
             continue;
         }
 
-        assert_eq!(count.highlight_count, 1); // one new highlight
-        assert_eq!(count.notification_count, 2); // the highlight counts as a new notification
-        prev_count = count;
+        // The highlight also counts as a notification.
+        assert_eq!(alice_room.num_unread_messages(), 2);
+        // One new highlight.
+        assert_eq!(alice_room.num_unread_mentions(), 1);
         break;
     }
 
@@ -374,16 +369,15 @@ async fn test_room_notification_count() -> Result<()> {
     loop {
         assert!(info_updates.next().await.is_some());
 
-        let count = alice_room.unread_notification_counts();
-        if count == prev_count {
+        if alice_room.num_unread_messages() == 2 && alice_room.num_unread_mentions() == 1 {
             // Sometimes we get notified for changes to unrelated, other fields of
             // `info_updates`.
             tracing::warn!("ignoring");
             continue;
         }
 
-        assert_eq!(count.highlight_count, 0, "{count:?}");
-        assert_eq!(count.notification_count, 0, "{count:?}");
+        assert_eq!(alice_room.num_unread_messages(), 0);
+        assert_eq!(alice_room.num_unread_mentions(), 0);
         break;
     }
 
@@ -395,16 +389,14 @@ async fn test_room_notification_count() -> Result<()> {
     // Local echo for our own message.
     assert!(info_updates.next().await.is_some());
 
-    let count = alice_room.unread_notification_counts();
-    assert_eq!(count.highlight_count, 0, "{count:?}");
-    assert_eq!(count.notification_count, 0, "{count:?}");
+    assert_eq!(alice_room.num_unread_messages(), 0);
+    assert_eq!(alice_room.num_unread_mentions(), 0);
 
     // Remote echo for our own message.
     assert!(info_updates.next().await.is_some());
 
-    let count = alice_room.unread_notification_counts();
-    assert_eq!(count.highlight_count, 0, "{count:?}");
-    assert_eq!(count.notification_count, 0, "{count:?}");
+    assert_eq!(alice_room.num_unread_messages(), 0);
+    assert_eq!(alice_room.num_unread_mentions(), 0);
 
     assert_pending!(info_updates);