From f989c17e06007e546249c9a181b263ab6a43b981 Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Thu, 21 Dec 2023 09:13:22 +0100 Subject: [PATCH] Address review comments - copyright notice - doc comments and better doc in general - use static dispatch instead of &dyn T - other misc comments --- crates/matrix-sdk-base/src/read_receipts.rs | 73 ++++++++++++++++---- crates/matrix-sdk-base/src/sliding_sync.rs | 4 +- crates/matrix-sdk/src/sliding_sync/client.rs | 6 +- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-base/src/read_receipts.rs b/crates/matrix-sdk-base/src/read_receipts.rs index e94f27b4577..6faeabd9e2c 100644 --- a/crates/matrix-sdk-base/src/read_receipts.rs +++ b/crates/matrix-sdk-base/src/read_receipts.rs @@ -1,3 +1,44 @@ +// Copyright 2023 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! # Client-side read receipts computation +//! +//! While Matrix servers have the ability to provide basic information about the +//! unread status of rooms, via [`matrix_sdk::ruma::UnreadNotificationCounts`], +//! it's not reliable for encrypted rooms. Indeed, the server doesn't have +//! access to the content of encrypted events, so it can only makes guesses when +//! estimating unread and highlight counts. +//! +//! Instead, this module provides facilities to compute the number of unread +//! messages, unread notifications and unread highlights in a room. +//! +//! Counting unread messages is performed by looking at the latest receipt of +//! the current user, and inferring which events are following it, according to +//! the sync ordering. +//! +//! For notifications and highlights to be precisely accounted for, we also need +//! to pay attention to the user's notification settings. Fortunately, this is +//! also something we need to for notifications, so we can reuse this code. +//! +//! Of course, not all events are created equal, and some are less interesting +//! than others, and shouldn't cause a room to be marked unread. This module's +//! `marks_as_unread` function shows the opiniated set of rules that will filter +//! out uninterested events. +//! +//! The only public method in that module is [`compute_notifications`], which +//! updates the `RoomInfo` in place according to the new counts. + use eyeball_im::Vector; use matrix_sdk_common::deserialized_responses::SyncTimelineEvent; use ruma::{ @@ -29,11 +70,19 @@ impl PreviousEventsProvider for () { } } +/// Given a set of events coming from sync, for a room, update the +/// [`RoomInfo`]'s counts of unread messages, notifications and highlights' in +/// place. +/// +/// A provider of previous events may be required to reconcile a read receipt +/// that has been just received for an event that came in a previous sync. +/// +/// See this module's documentation for more information. #[instrument(skip_all, fields(room_id = %room_info.room_id))] -pub(crate) async fn compute_notifications( +pub(crate) async fn compute_notifications( client: &BaseClient, changes: &StateChanges, - previous_events_provider: &dyn PreviousEventsProvider, + previous_events_provider: &PEP, new_events: &[SyncTimelineEvent], room_info: &mut RoomInfo, ) -> Result<()> { @@ -45,15 +94,9 @@ pub(crate) async fn compute_notifications( // Find a private or public read receipt for the current user. let mut receipt_event_id = None; - if let Some((event_id, receipt)) = - receipt_event.user_receipt(user_id, ReceiptType::ReadPrivate) - { - if receipt.thread == ReceiptThread::Unthreaded || receipt.thread == ReceiptThread::Main - { - receipt_event_id = Some(event_id.to_owned()); - } - } else if let Some((event_id, receipt)) = - receipt_event.user_receipt(user_id, ReceiptType::Read) + if let Some((event_id, receipt)) = receipt_event + .user_receipt(user_id, ReceiptType::Read) + .or_else(|| receipt_event.user_receipt(user_id, ReceiptType::ReadPrivate)) { if receipt.thread == ReceiptThread::Unthreaded || receipt.thread == ReceiptThread::Main { @@ -70,10 +113,10 @@ pub(crate) async fn compute_notifications( // First, save the event id as the latest one that has a read receipt. 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 + // Try to find if the read receipt refers to an event from the current sync, to // avoid searching the cached timeline events. trace!("We got a new event with a read receipt: {receipt_event_id}. Search in new events..."); - if find_and_count_events(&receipt_event_id, user_id, new_events.iter(), room_info) { + if find_and_count_events(&receipt_event_id, user_id, new_events, room_info) { // It did, so our work here is done. return Ok(()); } @@ -101,7 +144,7 @@ pub(crate) async fn compute_notifications( // properly processed, and we only need to process the new events based // on the previous receipt. trace!("No new receipts, or couldn't find attached event; looking if the past latest known receipt refers to a new event..."); - if find_and_count_events(&receipt_event_id, user_id, new_events.iter(), room_info) { + if find_and_count_events(&receipt_event_id, user_id, new_events, room_info) { // We found the event to which the previous receipt attached to, so our work is // done here. return Ok(()); @@ -148,7 +191,7 @@ fn count_unread_and_mentions( fn find_and_count_events<'a>( receipt_event_id: &EventId, user_id: &UserId, - events: impl Iterator, + events: impl IntoIterator, room_info: &mut RoomInfo, ) -> bool { let mut counting_receipts = false; diff --git a/crates/matrix-sdk-base/src/sliding_sync.rs b/crates/matrix-sdk-base/src/sliding_sync.rs index 37cd5421adb..985659eb7d6 100644 --- a/crates/matrix-sdk-base/src/sliding_sync.rs +++ b/crates/matrix-sdk-base/src/sliding_sync.rs @@ -113,10 +113,10 @@ impl BaseClient { /// * `response` - The response that we received after a successful sliding /// sync. #[instrument(skip_all, level = "trace")] - pub async fn process_sliding_sync( + pub async fn process_sliding_sync( &self, response: &v4::Response, - previous_events_provider: &dyn PreviousEventsProvider, + previous_events_provider: &PEP, ) -> Result { let v4::Response { // FIXME not yet supported by sliding sync. see diff --git a/crates/matrix-sdk/src/sliding_sync/client.rs b/crates/matrix-sdk/src/sliding_sync/client.rs index a1a1f71774e..123676f1209 100644 --- a/crates/matrix-sdk/src/sliding_sync/client.rs +++ b/crates/matrix-sdk/src/sliding_sync/client.rs @@ -3,7 +3,6 @@ use std::collections::BTreeMap; use imbl::Vector; use matrix_sdk_base::{sync::SyncResponse, PreviousEventsProvider}; use ruma::{api::client::sync::sync_events::v4, events::AnyToDeviceEvent, serde::Raw, OwnedRoomId}; -use tracing::{debug, instrument}; use super::{SlidingSync, SlidingSyncBuilder}; use crate::{Client, Result, SlidingSyncRoom}; @@ -22,14 +21,15 @@ impl Client { /// /// If you need to handle encryption too, use the internal /// `SlidingSyncResponseProcessor` instead. - #[instrument(skip(self, response))] + #[cfg(any(test, feature = "testing"))] + #[tracing::instrument(skip(self, response))] pub async fn process_sliding_sync_test_helper( &self, response: &v4::Response, ) -> Result { let response = self.base_client().process_sliding_sync(response, &()).await?; - debug!("done processing on base_client"); + tracing::debug!("done processing on base_client"); self.handle_sync_response(&response).await?; Ok(response)