From fb763e3ab3b8769fc14d03fd2a36a73ce913a8de Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 19 Dec 2023 12:03:40 +0100 Subject: [PATCH] read receipts: move the unread messages and mentions counts to separate fields of `RoomInfo` --- bindings/matrix-sdk-ffi/src/room_info.rs | 4 ++ crates/matrix-sdk-base/src/read_receipts.rs | 51 +++++++++--------- crates/matrix-sdk-base/src/rooms/normal.rs | 53 ++++++++++++++++--- .../src/store/migration_helpers.rs | 2 +- .../src/tests/sliding_sync/room.rs | 46 +++++++--------- 5 files changed, 94 insertions(+), 62 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room_info.rs b/bindings/matrix-sdk-ffi/src/room_info.rs index cbcb5a6a812..2d953165827 100644 --- a/bindings/matrix-sdk-ffi/src/room_info.rs +++ b/bindings/matrix-sdk-ffi/src/room_info.rs @@ -31,6 +31,8 @@ pub struct RoomInfo { user_defined_notification_mode: Option, has_room_call: bool, active_room_call_participants: Vec, + 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(), }) } } diff --git a/crates/matrix-sdk-base/src/read_receipts.rs b/crates/matrix-sdk-base/src/read_receipts.rs index 0bebe8e0945..4e160f6e9f9 100644 --- a/crates/matrix-sdk-base/src/read_receipts.rs +++ b/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,7 +29,7 @@ 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, @@ -37,16 +37,8 @@ pub(crate) async fn compute_notifications( 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::("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; } } diff --git a/crates/matrix-sdk-base/src/rooms/normal.rs b/crates/matrix-sdk-base/src/rooms/normal.rs index ac0b8464be6..b4e5c4b728f 100644 --- a/crates/matrix-sdk-base/src/rooms/normal.rs +++ b/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, +} + /// 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>, - /// 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, + 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_mentions": 0, + "num_unread_messages": 0, + "latest_read_receipt_event_id": null, } }); diff --git a/crates/matrix-sdk-base/src/store/migration_helpers.rs b/crates/matrix-sdk-base/src/store/migration_helpers.rs index a54e7fc0213..6db9e2ebbe7 100644 --- a/crates/matrix-sdk-base/src/store/migration_helpers.rs +++ b/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), } } diff --git a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs index b5f194f4d89..1ecc95a82e2 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs +++ b/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);