From 0ba4e421616c93d04c6d2fd29b5f11d5014fdc3c Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Tue, 30 Apr 2024 19:55:56 +0200 Subject: [PATCH 1/2] notification client: get rid of builder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The builder had only one meaningful method, `filter_by_push_rules`, which was always called by the applications — and in fact should always be true. It was designed as an extra method because it was experimental at the time, but it's stabilized sufficiently that we can enable this behavior by default now, considering that a notification that is not wanted by the user shouldn't be kept, to respect their intent. (This is in the UI crate, which is opinionated, so it's fine to assume such intents by design.) --- bindings/matrix-sdk-ffi/src/client.rs | 15 +++- bindings/matrix-sdk-ffi/src/notification.rs | 45 +---------- .../matrix-sdk-ui/src/notification_client.rs | 78 ++++--------------- .../tests/integration/notification_client.rs | 6 +- .../tests/sliding_sync/notification_client.rs | 9 +-- 5 files changed, 38 insertions(+), 115 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/client.rs b/bindings/matrix-sdk-ffi/src/client.rs index 60ed07e99..dd4079656 100644 --- a/bindings/matrix-sdk-ffi/src/client.rs +++ b/bindings/matrix-sdk-ffi/src/client.rs @@ -37,7 +37,10 @@ use matrix_sdk::{ }, AuthApi, AuthSession, Client as MatrixClient, SessionChange, SessionTokens, }; -use matrix_sdk_ui::notification_client::NotificationProcessSetup as MatrixNotificationProcessSetup; +use matrix_sdk_ui::notification_client::{ + NotificationClient as MatrixNotificationClient, + NotificationProcessSetup as MatrixNotificationProcessSetup, +}; use mime::Mime; use ruma::{ api::client::discovery::discover_homeserver::AuthenticationServerInfo, @@ -58,7 +61,7 @@ use super::{room::Room, session_verification::SessionVerificationController, RUN use crate::{ client, encryption::Encryption, - notification::NotificationClientBuilder, + notification::NotificationClient, notification_settings::NotificationSettings, room_directory_search::RoomDirectorySearch, room_preview::RoomPreview, @@ -642,8 +645,12 @@ impl Client { pub async fn notification_client( self: Arc, process_setup: NotificationProcessSetup, - ) -> Result, ClientError> { - NotificationClientBuilder::new(self.clone(), process_setup.into()).await + ) -> Result, ClientError> { + Ok(Arc::new(NotificationClient { + inner: MatrixNotificationClient::new((*self.inner).clone(), process_setup.into()) + .await?, + _client: self.clone(), + })) } pub fn sync_service(&self) -> Arc { diff --git a/bindings/matrix-sdk-ffi/src/notification.rs b/bindings/matrix-sdk-ffi/src/notification.rs index 29012a2b9..bcec8f065 100644 --- a/bindings/matrix-sdk-ffi/src/notification.rs +++ b/bindings/matrix-sdk-ffi/src/notification.rs @@ -1,15 +1,11 @@ use std::sync::Arc; use matrix_sdk_ui::notification_client::{ - NotificationClient as MatrixNotificationClient, - NotificationClientBuilder as MatrixNotificationClientBuilder, - NotificationItem as MatrixNotificationItem, NotificationProcessSetup, + NotificationClient as MatrixNotificationClient, NotificationItem as MatrixNotificationItem, }; use ruma::{EventId, RoomId}; -use crate::{ - client::Client, error::ClientError, event::TimelineEvent, helpers::unwrap_or_clone_arc, -}; +use crate::{client::Client, error::ClientError, event::TimelineEvent}; #[derive(uniffi::Enum)] pub enum NotificationEvent { @@ -80,49 +76,16 @@ impl NotificationItem { } } -#[derive(Clone, uniffi::Object)] -pub struct NotificationClientBuilder { - client: Arc, - builder: MatrixNotificationClientBuilder, -} - -impl NotificationClientBuilder { - pub(crate) async fn new( - client: Arc, - process_setup: NotificationProcessSetup, - ) -> Result, ClientError> { - let builder = - MatrixNotificationClient::builder((*client.inner).clone(), process_setup).await?; - Ok(Arc::new(Self { builder, client })) - } -} - -#[uniffi::export] -impl NotificationClientBuilder { - /// Filter out the notification event according to the push rules present in - /// the event. - pub fn filter_by_push_rules(self: Arc) -> Arc { - let this = unwrap_or_clone_arc(self); - let builder = this.builder.filter_by_push_rules(); - Arc::new(Self { builder, client: this.client }) - } - - pub fn finish(self: Arc) -> Arc { - let this = unwrap_or_clone_arc(self); - Arc::new(NotificationClient { inner: this.builder.build(), _client: this.client }) - } -} - #[derive(uniffi::Object)] pub struct NotificationClient { - inner: MatrixNotificationClient, + pub(crate) inner: MatrixNotificationClient, /// A reference to the FFI client. /// /// Note: we do this to make it so that the FFI `NotificationClient` keeps /// the FFI `Client` and thus the SDK `Client` alive. Otherwise, we /// would need to repeat the hack done in the FFI `Client::drop` method. - _client: Arc, + pub(crate) _client: Arc, } #[uniffi::export(async_runtime = "tokio")] diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 3ece08dee..06a6fe33b 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -87,10 +87,6 @@ pub struct NotificationClient { /// Is the notification client running on its own process or not? process_setup: NotificationProcessSetup, - /// Should we try to filter out the notification event according to the push - /// rules? - filter_by_push_rules: bool, - /// A mutex to serialize requests to the notifications sliding sync. /// /// If several notifications come in at the same time (e.g. network was @@ -111,12 +107,19 @@ impl NotificationClient { const CONNECTION_ID: &'static str = "notifications"; const LOCK_ID: &'static str = "notifications"; - /// Create a new builder for a notification client. - pub async fn builder( - client: Client, + /// Create a new notification client. + pub async fn new( + parent_client: Client, process_setup: NotificationProcessSetup, - ) -> Result { - NotificationClientBuilder::new(client, process_setup).await + ) -> Result { + let client = parent_client.notification_client().await?; + Ok(NotificationClient { + client, + parent_client, + notification_sync_mutex: AsyncMutex::new(()), + encryption_sync_mutex: AsyncMutex::new(()), + process_setup, + }) } /// Fetches the content of a notification. @@ -446,7 +449,7 @@ impl NotificationClient { }; if let Some(push_actions) = &push_actions { - if self.filter_by_push_rules && !push_actions.iter().any(|a| a.should_notify()) { + if !push_actions.iter().any(|a| a.should_notify()) { return Ok(NotificationStatus::EventFilteredOut); } } @@ -491,11 +494,10 @@ impl NotificationClient { timeline_event = decrypted_event; } - if self.filter_by_push_rules - && !timeline_event - .push_actions - .as_ref() - .is_some_and(|actions| actions.iter().any(|a| a.should_notify())) + if !timeline_event + .push_actions + .as_ref() + .is_some_and(|actions| actions.iter().any(|a| a.should_notify())) { return Ok(None); } @@ -529,52 +531,6 @@ pub enum NotificationStatus { EventFilteredOut, } -/// Builder for a `NotificationClient`. -/// -/// Fields have the same meaning as in `NotificationClient`. -#[derive(Clone)] -pub struct NotificationClientBuilder { - /// SDK client that uses an in-memory state store, to be used with the - /// sliding sync method. - client: Client, - /// SDK client that uses the same state store as the caller's context. - parent_client: Client, - filter_by_push_rules: bool, - - /// Is the notification client running on its own process or not? - process_setup: NotificationProcessSetup, -} - -impl NotificationClientBuilder { - async fn new( - parent_client: Client, - process_setup: NotificationProcessSetup, - ) -> Result { - let client = parent_client.notification_client().await?; - - Ok(Self { client, parent_client, filter_by_push_rules: false, process_setup }) - } - - /// Filter out the notification event according to the push rules present in - /// the event. - pub fn filter_by_push_rules(mut self) -> Self { - self.filter_by_push_rules = true; - self - } - - /// Finishes configuring the `NotificationClient`. - pub fn build(self) -> NotificationClient { - NotificationClient { - client: self.client, - parent_client: self.parent_client, - filter_by_push_rules: self.filter_by_push_rules, - notification_sync_mutex: AsyncMutex::new(()), - encryption_sync_mutex: AsyncMutex::new(()), - process_setup: self.process_setup, - } - } -} - /// The Notification event as it was fetched from remote for the /// given `event_id`, represented as Raw but decrypted, thus only /// whether it is an invite or regular Timeline event has been diff --git a/crates/matrix-sdk-ui/tests/integration/notification_client.rs b/crates/matrix-sdk-ui/tests/integration/notification_client.rs index 071a33b8d..0c02be0af 100644 --- a/crates/matrix-sdk-ui/tests/integration/notification_client.rs +++ b/crates/matrix-sdk-ui/tests/integration/notification_client.rs @@ -59,8 +59,7 @@ async fn test_notification_client_with_context() { let dummy_sync_service = Arc::new(SyncService::builder(client.clone()).build().await.unwrap()); let process_setup = NotificationProcessSetup::SingleProcess { sync_service: dummy_sync_service }; - let notification_client = - NotificationClient::builder(client, process_setup).await.unwrap().build(); + let notification_client = NotificationClient::new(client, process_setup).await.unwrap(); { // The notification client retrieves the event via `/rooms/*/context/`. @@ -242,8 +241,7 @@ async fn test_notification_client_sliding_sync() { let dummy_sync_service = Arc::new(SyncService::builder(client.clone()).build().await.unwrap()); let process_setup = NotificationProcessSetup::SingleProcess { sync_service: dummy_sync_service }; - let notification_client = - NotificationClient::builder(client, process_setup).await.unwrap().build(); + let notification_client = NotificationClient::new(client, process_setup).await.unwrap(); let item = notification_client.get_notification_with_sliding_sync(room_id, event_id).await.unwrap(); diff --git a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/notification_client.rs b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/notification_client.rs index 96501b6de..d711cc1c0 100644 --- a/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/notification_client.rs +++ b/testing/matrix-sdk-integration-testing/src/tests/sliding_sync/notification_client.rs @@ -85,7 +85,7 @@ async fn test_notification() -> Result<()> { // Try with sliding sync first. let notification_client = - NotificationClient::builder(bob.clone(), process_setup.clone()).await.unwrap().build(); + NotificationClient::new(bob.clone(), process_setup.clone()).await.unwrap(); assert_let!( NotificationStatus::Event(notification) = notification_client.get_notification_with_sliding_sync(&room_id, &event_id).await? @@ -112,7 +112,7 @@ async fn test_notification() -> Result<()> { // Then with /context. let notification_client = - NotificationClient::builder(bob.clone(), process_setup.clone()).await.unwrap().build(); + NotificationClient::new(bob.clone(), process_setup.clone()).await.unwrap(); let notification = notification_client.get_notification_with_context(&room_id, &event_id).await; // We aren't authorized to inspect events from rooms we were not invited to. @@ -198,15 +198,14 @@ async fn test_notification() -> Result<()> { }; let notification_client = - NotificationClient::builder(bob.clone(), process_setup.clone()).await.unwrap().build(); + NotificationClient::new(bob.clone(), process_setup.clone()).await.unwrap(); assert_let!( NotificationStatus::Event(notification) = notification_client.get_notification_with_sliding_sync(&room_id, &event_id).await? ); check_notification(true, notification); - let notification_client = - NotificationClient::builder(bob.clone(), process_setup).await.unwrap().build(); + let notification_client = NotificationClient::new(bob.clone(), process_setup).await.unwrap(); let notification = notification_client .get_notification_with_context(&room_id, &event_id) .await? From f69db1d16913c16a38fa31880eb0e32efd5ca30f Mon Sep 17 00:00:00 2001 From: Benjamin Bouvier Date: Wed, 1 May 2024 13:09:01 +0200 Subject: [PATCH 2/2] notification client(bugfix): don't filter out the notification if we couldn't compute push actions with /context This is in line with what the other method using sliding sync does. This wasn't tested before, because this required `filter_by_push_rules()` to be enabled in the notification client; now that it's the default, the test revealed the bug, and so it could be fixed. --- crates/matrix-sdk-ui/src/notification_client.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-ui/src/notification_client.rs b/crates/matrix-sdk-ui/src/notification_client.rs index 06a6fe33b..eab81d6f2 100644 --- a/crates/matrix-sdk-ui/src/notification_client.rs +++ b/crates/matrix-sdk-ui/src/notification_client.rs @@ -494,12 +494,10 @@ impl NotificationClient { timeline_event = decrypted_event; } - if !timeline_event - .push_actions - .as_ref() - .is_some_and(|actions| actions.iter().any(|a| a.should_notify())) - { - return Ok(None); + if let Some(actions) = timeline_event.push_actions.as_ref() { + if !actions.iter().any(|a| a.should_notify()) { + return Ok(None); + } } Ok(Some(