-
Notifications
You must be signed in to change notification settings - Fork 410
MSC3768: Push rule action for in-app notifications #3768
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
Co-authored-by: Patrick Cloke <[email protected]>
keywords: do not disturb |
proposals/3768-notify-in-app.md
Outdated
A new push rule action `notify_in_app` is introduced. | ||
|
||
- `notify_in_app` -- This causes each matching event to generate a | ||
notification **without sending a push**. In particular, this means, | ||
like `notify`, the server should consider the event when computing | ||
`notification_count` and `highlight_count` in the `/sync` response. | ||
Unlike `notify`, the server should not forward the notification to | ||
any of its pushers. Clients should display in-app notifications just | ||
like for `notify`. |
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.
This whole thing sounds like the responsibility of the client or the OS controls on notifications. We could notify
and push all of the time and the client can decide what to do based on whatever preferences. Or people can adjust their OS notification preferences as they see fit.
What benefit is there from not pushing?
(Heads-up: I'm naive on all things push-rules, etc)
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.
Hm, this is a good point actually. Due to E2EE, clients need to filter and selectively hide remote pushes anyway. So we could indeed still push the notifications from the server. The only benefit of not pushing would reduced resource usage.
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.
My suggestion would be to make skipping the push on the server side a MAY. Functionally it's not required but practically it could save some processing resources. It's also relatively easy to implement.
A new push rule action `notify_in_app` is introduced. | |
- `notify_in_app` -- This causes each matching event to generate a | |
notification **without sending a push**. In particular, this means, | |
like `notify`, the server should consider the event when computing | |
`notification_count` and `highlight_count` in the `/sync` response. | |
Unlike `notify`, the server should not forward the notification to | |
any of its pushers. Clients should display in-app notifications just | |
like for `notify`. | |
A new push rule action `notify_in_app` is introduced. | |
- `notify_in_app` -- This causes each matching event to generate a | |
notification **without sending a push**. In particular, this means, | |
like `notify`, the server MUST consider the event when computing | |
`notification_count` and `highlight_count` in the `/sync` response. | |
Unlike `notify`, the server MAY skip forwarding the notification to | |
any of its pushers. Suppressing the push is OPTIONAL because clients | |
need to locally reapply push rules upon receiving push notifications | |
anyway due to E2EE. Clients MUST suppress push notifications that | |
resulted from `notify_in_app` but SHOULD display in-app notifications | |
just like for `notify`. |
(I cannot update the proposal, sadly, because this branch was created when I still worked for Element.)
This makes for a minimally invasive solution to the problem of | ||
in-app-only notifications. | ||
|
||
## Potential issues |
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.
(I'm not sure if it's documented but I believe we're encouraged to only comment on the diff / file to facilitate threading.)
@bnjbvr said:
What about backwards compat: is it specified somewhere (in the existing spec, as I couldn't find it here) how a client should behave in the presence of an unknown action type?
Assuming one's using the new notify-in-app action: the only way to have the badge count correctly updated would be for an app to run the sync in a background service, right? In that case, it sounds like an alternative could be that the app adds some kind of live event handler, and decides, based on pattern matching the event's content, to generate a local notification/increment the badge count. This wouldn't require a new action, while putting the onus of implementation on clients.
The implementation in the Matrix Rust SDK brings a new notification mode that's a bit hard to translate in layperson's words, for the end users: "notify me on mentions and keywords from pushes, but for every message notify me locally". This might be a UX problem very specific to the Matrix Rust SDK (as in, the shape of API it provides), but I'm afraid this overall complexifies the user experience, with a single benefit of having the server generating fewer pushes.
To wit, though: generating fewer pushes is a noble goal that may bring some super nice benefits, especially in the context of MSC4028.
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.
What about backwards compat: is it specified somewhere (in the existing spec, as I couldn't find it here) how a client should behave in the presence of an unknown action type?
The only related reference I am aware of is the one about ignoring dont_notify
and coalesce
.
I think ignoring works somewhat reasonably well here, too. A client that doesn't support this proposal would treat ["notify_in_app"]
like []
and not notify at all. This achieves the goal of not causing remote notifications albeit at the cost of not having any notifications in that client.
Alternatively, we could turn notify_in_app
into a new tweak that can be specified together with notify
. Ignoring would then make old clients notify normally with the cost being that the setting to only notify in-app would have no effect whatsoever in these clients.
Assuming one's using the new notify-in-app action: the only way to have the badge count correctly updated would be for an app to run the sync in a background service, right?
If the server uses notify_in_app
to actually suppress pushes, then yes. We did conclude in https://github.com/matrix-org/matrix-spec-proposals/pull/3768/files#r2229026617, however, that the server could just push regardless because the client has to locally filter remote notifications anyway. In that case the client could still update app-icon badge counts without letting the notifications pop up.
In that case, it sounds like an alternative could be that the app adds some kind of live event handler, and decides, based on pattern matching the event's content, to generate a local notification/increment the badge count. This wouldn't require a new action, while putting the onus of implementation on clients.
I'm not sure I fully understand what you mean. Isn't this essentially the background /sync
option?
The implementation in the Matrix Rust SDK brings a new notification mode that's a bit hard to translate in layperson's words, for the end users: "notify me on mentions and keywords from pushes, but for every message notify me locally". This might be a UX problem very specific to the Matrix Rust SDK (as in, the shape of API it provides), but I'm afraid this overall complexifies the user experience, with a single benefit of having the server generating fewer pushes.
This is true. I had chosen this as a first step because it's very easy to implement (same as the existing mentions & keywords mode but with a different action). UX-wise this might not be overly useful, however. I think the end goal should be to set a room to notify-in-app entirely. This is more complicated to manage though because it also involves tweaking the various override rules.
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.
(Oops, sorry for not starting a thread on the diff, and thanks for reproducing it here 👍)
I'm not sure I fully understand what you mean. Isn't this essentially the background /sync option?
I don't think that /sync
needs to be involved, in this alternative; if the push notifications are still emitted by the server, it can be the client's responsibility to "lower" them into app-only notifications. If the server doesn't send the push notifications, indeed a background /sync
is sufficient to achieve the same effect.
In both cases: if the suppression of a notification is the responsibility of a device, then we have another way of achieving the same outcome without this MSC. I wonder if this was also the question behind #3768 (comment).
So here's me thinking out loud about cost vs benefits, here:
- the cost is mostly the implementation burden (clients + servers), maybe some extra weight in terms of explaining this new behavior in layperson's words so that it's easy to explain what this does.
- one benefit would be that a server now doesn't have to send a push for each notification. For this to be valuable, it would likely have to be a default behavior (e.g. become a default push rule), for the economies of scale to kick in. In other words: it might not be as interesting if only a small fraction of the end users are using this new rule.
- one other benefit is getting the synchronization of the suppression of push notifications across devices. However, I wonder if there are other, more lightweight ways to implement this preference synchronization (account data entry?).
(Of course this list doesn't pretend to be exhaustive, and maybe I'm missing the point of this MSC. Please, feel free to add other benefits/drawbacks here!)
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.
I don't think that
/sync
needs to be involved, in this alternative; if the push notifications are still emitted by the server, it can be the client's responsibility to "lower" them into app-only notifications. If the server doesn't send the push notifications, indeed a background/sync
is sufficient to achieve the same effect.
Ok, I think we're saying the same then actually.
In both cases: if the suppression of a notification is the responsibility of a device, then we have another way of achieving the same outcome without this MSC.
It sounds like what you're suggesting is for a client to store some setting in an account data event and then use that setting when processing remote notifications to selectively suppress them. Such an event could look like this:
{
"type": "some.client.in_app_notifications",
"content": {
"rooms": ["!asdf:matrix.org", ...]
}
}
This seems viable but also somewhat equivalent to what this proposal already does. Push rules are stored in account data and clients use notify
and notify_in_app
to decide whether to suppress a remote notification or not.
The one benefit I can see is not having to fiddle with the notorious push rule system?
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.
The one benefit I can see is not having to fiddle with the notorious push rule system?
This is a huge benefit IMO 😁
|
||
None. | ||
|
||
## Alternatives |
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.
I realise these 2 are newer, but it would be interesting to add references to #3881 and #3890 here. I seem to remember we added support for these to Element Classic (element-hq/element-ios#6815 / element-hq/element-ios#6798 – I'm failing to actually find them in the UI though) and I'm not sure I see the advantage of this approach over those.
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.
I think the difference is that the others are global while this one can work per room (though the push rule legwork to do so is not necessarily trivial).
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.
## Alternatives | |
## Alternatives | |
[MSC3881] and [MSC3890] solve a similar problem but can only disable | |
notifications globally and not per-room. |
(I cannot apply this suggestion because I lost ownership of the branch.)
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.
Ahhhh, thanks for the clarification, I didn't see any mention of wanting to set per-room DnD in the intro so that didn't even cross my mind 🙃
Co-authored-by: Johannes Marbach <[email protected]>
Rendered
Synapse: element-hq/synapse#18720
Ruma: ruma/ruma#2166
Rust SDK: matrix-org/matrix-rust-sdk#5441
In line with matrix-org/matrix-spec#1700, the following disclosure applies:
I am a Systems Architect at gematik, Software Engineer at Unomed, Matrix community member and former Element employee. This proposal was written and published with my Element hat on.