feat(ffi): Allow to set the notification mode for a room #1967

nimau · 2023-05-24T12:24:57Z

This PR implements the first part of #1959

It exposes a new NotificationSettings object in order to update the notification settings for a given room.
The logic behind has been placed in matrix-sdk, with some unit tests.

The following MSC were considered:
MSC3987: Push actions clean-up
MSC3952: Intentional Mentions

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

stefanceriu

Very nice!
The only other comment I would have is that it would be really nice to have unit tests for this logic, but we don't have unit tests anywhere in the FFI layer at the moment so that's a discussion to be had

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

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

codecov · 2023-05-26T08:08:54Z

Codecov Report

Patch coverage: 81.69% and project coverage change: +0.05 🎉

Comparison is base (d1a1a0f) 75.24% compared to head (355c378) 75.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1967      +/-   ##
==========================================
+ Coverage   75.24%   75.30%   +0.05%     
==========================================
  Files         147      148       +1     
  Lines       16052    16194     +142     
==========================================
+ Hits        12079    12195     +116     
- Misses       3973     3999      +26

Impacted Files	Coverage Δ
crates/matrix-sdk/src/error.rs	`58.92% <0.00%> (-7.08%)`	⬇️
crates/matrix-sdk/src/lib.rs	`100.00% <ø> (ø)`
crates/matrix-sdk/src/notification_settings.rs	`85.07% <85.07%> (ø)`
crates/matrix-sdk/src/client/mod.rs	`81.89% <100.00%> (+0.10%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

bnjbvr

Just skimming and passing by; this looks like a great thing to have in the non-FFI crate, as it implements more than just mapping to concepts existing in the non-FFI, including some lightweight logic. Would it make sense to move it to the non FFI crate? Then you could also more easily write tests, which would be sweet to have to make sure that this behaves as you'd expect.

nimau · 2023-06-01T12:10:40Z

Just skimming and passing by; this looks like a great thing to have in the non-FFI crate, as it implements more than just mapping to concepts existing in the non-FFI, including some lightweight logic. Would it make sense to move it to the non FFI crate? Then you could also more easily write tests, which would be sweet to have to make sure that this behaves as you'd expect.

Hi @bnjbvr, I've moved the logic inside matrix-sdk (in fact this had also been requested by @jplatte).

crates/matrix-sdk/src/error.rs

crates/matrix-sdk/src/notification_settings.rs

giomfo

@nimau I added 3 comments

giomfo

commented

crates/matrix-sdk/src/notification_settings.rs

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

crates/matrix-sdk/src/notification_settings.rs

feat(ffi): Support for MSC3987 and MSC3952

…al mentions and keywords rules are disabled.

nimau · 2023-06-06T09:28:42Z

@giomfo: I've updated the code to reflect your comments.
I've updated the rev for ruma in Cargo.toml to get the fix for set_pushrule.
In addition, I now use the ruma-client API to insert or delete push rules instead of overwriting account data.

…t mode on the next sync response.

jplatte

Not a full review, just some remarks about the tests.

crates/matrix-sdk/src/notification_settings.rs

Hywan

Thanks for the PR. I didn't check the FFI bindings yet, I was focus on matrix-sdk only.

I left few comments.

Please use Markdown in your documentation, and please try to expand your documentation with more than “replacing underscores by spaces from the method name” trick :-).

crates/matrix-sdk/src/notification_settings.rs

crates/matrix-sdk/src/error.rs

nimau · 2023-06-08T06:00:28Z

Hi @Hywan, thanks so much for your time and feedback.
I've updated this branch to reflect your comments, I think it will be better now.

bnjbvr

Thanks! I've got a semi-large request to split the code (and PRs?), since NotificationSettings does a lot; it would be pretty nice in terms of separating concerns, and making the PR review faster (and I'm willing to do the review of such a PR). See below, and let me know your thoughts.

bnjbvr · 2023-06-13T14:32:16Z

bindings/matrix-sdk-ffi/src/api.udl

@@ -167,3 +167,7 @@ callback interface SessionVerificationControllerDelegate {
    void did_cancel();
    void did_finish();
 };
+
+callback interface NotificationSettingsDelegate {


We've started calling those "listeners" recently:

Suggested change

callback interface NotificationSettingsDelegate {

callback interface NotificationSettingsListener {

bnjbvr · 2023-06-13T14:56:24Z

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

+    /// Sets a delegate.
+    pub async fn set_delegate(&self, delegate: Option<Box<dyn NotificationSettingsDelegate>>) {
+        *self.delegate.write().await = delegate;
+    }


Can the delegate be set when constructing the NotificationsSettings immediately? Would avoid the locking here, which would be nice.

bnjbvr · 2023-06-13T15:50:00Z

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

+            }
+            RoomNotificationMode::Mute => SdkRoomNotificationMode::Mute,
+        };
+        let parsed_room_idom_id = RoomId::parse(&room_id)


typo: idom?

bnjbvr · 2023-06-13T17:02:38Z

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

+    ) -> Result<(), NotificationSettingsError> {
+        let mut ruleset = self.push_rules.read().await.clone();
+        let notification_settings = self.sdk_client.notification_settings();
+        let parsed_room_idom_id = RoomId::parse(&room_id)


(idom here too)

bnjbvr · 2023-06-13T17:04:25Z

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

+        // Listen for PushRulesEvent
+        let push_rules_clone = push_rules.to_owned();
+        let delegate_clone = delegate.to_owned();
+        sdk_client.add_event_handler(move |ev: PushRulesEvent| {


Is this sufficient to get all the push rules since the creation of the room? are we sure at least one PushRulesEvent is handled?

bnjbvr · 2023-06-13T17:13:55Z

crates/matrix-sdk/src/notification_settings.rs

+
+/// A high-level API to manage the client owner's push notification settings.
+#[derive(Debug, Clone)]
+pub struct NotificationSettings {


(This is the large change I'm mentioning in the main review comment.)

This object mixes a few responsibilities:

computing the actual rules from observing the raw Ruma Ruleset

interacting with the server to add/remove things

The former tends to not even make use of self, in methods like get_custom_rules_for_room, get_user_defined_room_notification_mode, and a few others. This suggests a different implementation:

create a wrapper struct for Ruleset, which only job is to interact with the inner Ruleset.

then have this NotificationSettings contain this wrapper (I don't think it should be the FFI's responsbility to own the Ruleset), and expose extra methods to interact with the server and update the ruleset.

That would blackbox the different problems, and if the Ruleset's wrapper needs to interact with the server, it can use a command pattern and return a vec of commands to be applied by the client thereafter. Would probably help with testing, and also this would give us the possibility to split this PR into smaller ones that are easier to implement, test, review and merge 😁 : one PR for the Ruleset wrapper and its tests, one for the NotificationSettings. How does that sound?

bnjbvr · 2023-06-13T17:16:23Z

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

+        // Get the current user defined mode for this room
+        if let Some(mode) =
+            notification_settings.get_user_defined_room_notification_mode(&parsed_room_id, ruleset)
+        {
+            return Ok(RoomNotificationSettings::new(mode.into(), false));
+        }
+
+        // If the user didn't defined a notification mode, return the default one for
+        // this room
+        let room = self
+            .sdk_client
+            .get_room(&parsed_room_id)
+            .context("Room not found")
+            .map_err(|_| NotificationSettingsError::RoomNotFound)?;
+
+        let is_encrypted = room.is_encrypted().await.unwrap_or(false);
+        let members_count = room.joined_members_count();
+
+        let mode = notification_settings.get_default_room_notification_mode(
+            is_encrypted,
+            members_count,
+            ruleset,
+        );
+        Ok(RoomNotificationSettings::new(mode.into(), true))


IMO this should be a method in the NotificationSettings itself, there's still a bit too much logic for the FFI layer here.

nimau · 2023-06-19T16:02:44Z

I'm closing this PR, as it will be divided into several smaller PR.

Velin92 reviewed May 24, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/notification_settings.rs Outdated Show resolved Hide resolved

stefanceriu reviewed May 24, 2023

View reviewed changes

bindings/matrix-sdk-ffi/src/error.rs Outdated Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/client.rs Outdated Show resolved Hide resolved

nimau marked this pull request as ready for review May 25, 2023 16:45

nimau requested a review from a team as a code owner May 25, 2023 16:45

nimau requested review from poljar and removed request for a team May 25, 2023 16:45

nimau marked this pull request as draft May 26, 2023 13:47

nimau removed the request for review from poljar May 26, 2023 13:47

bnjbvr reviewed May 29, 2023

View reviewed changes

nimau force-pushed the nicolas/push-rules-ffi branch from d06320a to 2c0e7cb Compare June 1, 2023 09:13

nimau marked this pull request as ready for review June 1, 2023 12:19

nimau requested review from a team, poljar and bnjbvr and removed request for a team and poljar June 1, 2023 12:20

giomfo reviewed Jun 2, 2023

View reviewed changes

crates/matrix-sdk/src/error.rs Outdated Show resolved Hide resolved

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

giomfo reviewed Jun 2, 2023

View reviewed changes

nimau requested review from a team and jplatte and removed request for bnjbvr and a team June 5, 2023 06:50

giomfo reviewed Jun 5, 2023

View reviewed changes

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

bindings/matrix-sdk-ffi/src/notification_settings.rs Outdated Show resolved Hide resolved

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

giomfo reviewed Jun 5, 2023

View reviewed changes

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

nimau force-pushed the nicolas/push-rules-ffi branch from edb136c to 129607b Compare June 5, 2023 18:10

nimau added 3 commits June 6, 2023 10:20

feat(ffi): Expose room notification settings

795a69a

feat(ffi): Support for MSC3987 and MSC3952

feat(sdk): rename contains_keyword_rules in NotificationSettings.

c35e3f5

Fix: rust syntax

0cd7e11

nimau added 5 commits June 6, 2023 10:20

feat(sdk): Allow to set the MentionsAndKeywords mode even if the glob…

78e98d7

…al mentions and keywords rules are disabled.

Stop automatic saving of push rules in matrix-sdk notification_settings.

9f86590

Clean up code

3468351

Use ruma-client-api instead of updating the account_data

5c536ec

Update ruma rev to fix the set_pushrule

1046a63

nimau force-pushed the nicolas/push-rules-ffi branch from 129607b to 1046a63 Compare June 6, 2023 09:08

Fix: API push rules are now called in the right order to get the righ…

6252b6c

…t mode on the next sync response.

jplatte reviewed Jun 7, 2023

View reviewed changes

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

crates/matrix-sdk/src/notification_settings.rs Outdated Show resolved Hide resolved

Fix: Rust syntax in tests

03c0a66

Hywan requested changes Jun 7, 2023

View reviewed changes

Fix: Rust syntax, documentation and comments

cfbf356

nimau requested a review from Hywan June 8, 2023 15:30

Fix: get_default_room_notification_mode should not return a Result<>

355c378

nimau requested a review from bnjbvr June 13, 2023 07:53

bnjbvr requested changes Jun 13, 2023

View reviewed changes

nimau closed this Jun 19, 2023

nimau deleted the nicolas/push-rules-ffi branch July 7, 2023 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ffi): Allow to set the notification mode for a room #1967

feat(ffi): Allow to set the notification mode for a room #1967

nimau commented May 24, 2023 •

edited

Loading

stefanceriu left a comment

codecov bot commented May 26, 2023 •

edited

Loading

bnjbvr left a comment

nimau commented Jun 1, 2023

giomfo left a comment

giomfo left a comment

nimau commented Jun 6, 2023

jplatte left a comment

Hywan left a comment •

edited

Loading

nimau commented Jun 8, 2023

bnjbvr left a comment

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

bnjbvr Jun 13, 2023

nimau commented Jun 19, 2023

	callback interface NotificationSettingsDelegate {
	callback interface NotificationSettingsListener {

feat(ffi): Allow to set the notification mode for a room #1967

feat(ffi): Allow to set the notification mode for a room #1967

Conversation

nimau commented May 24, 2023 • edited Loading

stefanceriu left a comment

Choose a reason for hiding this comment

codecov bot commented May 26, 2023 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

nimau commented Jun 1, 2023

giomfo left a comment

Choose a reason for hiding this comment

giomfo left a comment

Choose a reason for hiding this comment

nimau commented Jun 6, 2023

jplatte left a comment

Choose a reason for hiding this comment

Hywan left a comment • edited Loading

Choose a reason for hiding this comment

nimau commented Jun 8, 2023

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

bnjbvr Jun 13, 2023

Choose a reason for hiding this comment

nimau commented Jun 19, 2023

nimau commented May 24, 2023 •

edited

Loading

codecov bot commented May 26, 2023 •

edited

Loading

Hywan left a comment •

edited

Loading