-
Notifications
You must be signed in to change notification settings - Fork 323
feat(push): add experimental support for MSC3768 (in-app-only notifications) #5441
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
be72c82
to
474c5d8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5441 +/- ##
=======================================
Coverage 88.92% 88.92%
=======================================
Files 333 333
Lines 92267 92311 +44
Branches 92267 92311 +44
=======================================
+ Hits 82051 82091 +40
- Misses 6374 6378 +4
Partials 3842 3842 ☔ View full report in Codecov by Sentry. |
3021ba4
to
86d2107
Compare
86d2107
to
6dc5b4e
Compare
CodSpeed Performance ReportMerging #5441 will not alter performanceComparing Summary
|
33af4fc
to
4478d63
Compare
…ations) Signed-off-by: Johannes Marbach <[email protected]>
4478d63
to
cac0103
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting MSC. How can it be used, right now, by an embedder? Is everything already wired up, so that timeline items would be marked as notifying the user if this rule is set?
I would also like to see some tests for the new expected behaviors, please, and at both the base, SDK, and UI layers, to at least smoke-test the behavior is correctly implemented.
cfg_if! { | ||
if #[cfg(feature = "unstable-msc3768")] { | ||
if !rule.triggers_remote_notification() { | ||
// This rule contains a `NotifyInApp` action. | ||
return Some(RoomNotificationMode::MentionsAndKeywordsOnlyTheRestInApp); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe we don't need a cfg_if
here? suggestion modulo rustfmt. I think the outer block is needed because one can't put cfg guards on statements, so we need a fake expression statement to make it work. But this might be worth checking without the outer block, in case it works too.
cfg_if! { | |
if #[cfg(feature = "unstable-msc3768")] { | |
if !rule.triggers_remote_notification() { | |
// This rule contains a `NotifyInApp` action. | |
return Some(RoomNotificationMode::MentionsAndKeywordsOnlyTheRestInApp); | |
} | |
} | |
} | |
#[cfg(feature = "unstable-msc3768")] | |
{ | |
if !rule.triggers_remote_notification() { | |
// This rule contains a `NotifyInApp` action. | |
return Some(RoomNotificationMode::MentionsAndKeywordsOnlyTheRestInApp); | |
} | |
} |
Also, maybe can we use a custom helper? Here we're doing .triggers_notifications()
which iterates over all the rules, then .triggers_remote_notification()
which iterates again. We could iterate once, and figure out on the go if it would trigger a remote or non-remote notification, instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you're right about cfg_if
. I have eliminated it with cd21783.
As for the second comment, it's actually not iterating over the rules but only over the actions on each rule. That array should normally have at most two items. So I think doing that twice is ok-ish?
Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Flip assertions Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Eliminate cfg_if Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Reformat Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Make Notify Copy Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Combine the two enum variants Signed-off-by: Johannes Marbach <[email protected]>
…notifications) Add read receipt test Signed-off-by: Johannes Marbach <[email protected]>
For the tests, I'd like to integrate something into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, noticed a thing that makes it not ideal to mix the bool within the field. I should've noticed the first time 😔 So let's get back to the new variant instead (with a suggested new name). And yeah let's add some notification client tests for the new notify-in-app mode. Likely we'll need to sync/fetch account data to tweak the default push rules for the user.
Co-authored-by: Benjamin Bouvier <[email protected]> Signed-off-by: Johannes Marbach <[email protected]>
…pp-only notifications)" This reverts commit deef961.
…notifications) Rename MentionsAndKeywordsOnlyTheRestInApp to PushMentionsAndKeywordsOnly Signed-off-by: Johannes Marbach <[email protected]>
Tests are still missing. I will need to figure out how to extend the existing ones. |
Moving back to draft to re-evaluate alternatives to MSC3768. |
This adds experimental support for MSC3768 by introducing the push rule action
NotifyInApp
and a room notification mode that is similar to mentions & keywords but notifies in-app rather than muting other notifications.Signed-off-by: