MSC2785: Event notification attributes and actions #2785
Conversation
richvdh
force-pushed
the
rav/proposals/notification_attributes
branch
from
49ed950
to
affed03
Compare
| follows. | ||
|
|
||
| Any event with no `body` property, or an empty `body` property, or a `body` property | ||
| which is not a string, should *not* match. |
There was a problem hiding this comment.
Should state events be skipped as well?
There was a problem hiding this comment.
I don't think so. It's entirely valid for a state event to contain a keyword.
| Implementations should determine whether to set this attribute as follows. | ||
|
|
||
| Any event with no `body` property, or an empty `body` property, or a `body` property | ||
| which is not a string, should *not* match. |
There was a problem hiding this comment.
Ditto on state events.
|
|
||
| #### `m.msg` | ||
|
|
||
| Any event with a `body` attribute which is a non-empty string. |
There was a problem hiding this comment.
Ditto on state events.
| #### `m.voip_call` | ||
|
|
||
| An `m.call.invite` non-state event. |
There was a problem hiding this comment.
I think we want this to catch VoIP widgets being added too, i.e BBB & Jitsi
There was a problem hiding this comment.
Probably. One of the points of this exercise is to make it easier to change the rules about "what counts as an event of type X", so it's not absolutely critical we get this right from the get-go.
I'll leave the text as-is for now: it's the sort of thing we're likely to end up wanting to fine-tune during development.
| Firstly, clients must decrypt events before assigning notification | ||
| attributes. They can then proceed as normal with the rest of the algorithm. |
There was a problem hiding this comment.
Could/should clients rely on server-assigned attributes for things like m.dm, m.invite and m.room_upgrade which the server can evaluate even for encrypted rooms.
m.keywordm.mentionm.voip_callm.msg
There was a problem hiding this comment.
well yes, but we don't (currently) specify a mechanism by which the server tells the client which attributes it assigned. So even if the server does figure out, the client needs to do it again.
I've added some text to say that clients might be able to skip decryption.
| This action also causes the event to be returned via future calls to the | ||
| [`/notifications`](https://matrix.org/docs/spec/client_server/r0.6.1#get-matrix-client-r0-notifications) | ||
| API endpoint. |
There was a problem hiding this comment.
How does this work with changing profiles.
If I change to profile none does /notifications empty out or does it show the messages which at the time of their sending caused the active profile to trigger m.notify
(do profile changes apply retroactively or not)
There was a problem hiding this comment.
That's a very good question.
I've tried to descope /notifications here as far as I can, because it is a whole can of worms in its own right. If we extend from the current pushrules-based behaviour, then changes do not apply retroactively. My inclination is to stick with this behaviour for now, and save "improvements to the notifications panel" for a future iteration.
|
@auscompgeek @t3chguy tyvm for the helpful reviews. I've added some clarifications in the doc and comments above. |
| Any event with no `body` property, or an empty `body` property, or a `body` property | ||
| which is not a string, should *not* match. | ||
|
|
||
| Otherwise, the `body` of the event is inspected for whole-word matches. The |
There was a problem hiding this comment.
I would like to only be mentioned by https://matrix.org/docs/spec/client_server/r0.6.1#user-room-and-group-mentions style mentions
It doesn’t look like this MSC has an option for that.
There was a problem hiding this comment.
yeah, I have deliberately descoped "what actually counts as a mention" here, since it's a hornet's nest. The intention is that in future it'll be much easier to change the rules by changing what counts as an m.mention than it will at present by trying to fit it into push rules.
https://github.com/matrix-org/matrix-doc/issues/1549 concerns fixing mentions.
Co-authored-by: Aaron Raimist <aaron@raim.ist>
| `m.notifications_profile` event does not exist, then no actions are triggered. | ||
|
|
||
| An `m.notifications_profile.<profile>` event can also be set at the room level, | ||
| using the same structure. This gives overrides specific to that room. If an |
There was a problem hiding this comment.
If spaces are a thing by the time this work gets done, we should also remember that it would be useful to control notifications per-space.
There was a problem hiding this comment.
I think the concept looks good but I think we need to clarify what bits are server-only, client-only and server and client. I think splitting these chunks apart will help answer a lot of questions with the spec. Particularly for server-only the server should reject rules that it doesn't understand so that clients know they won't get notified.
|
|
||
| Events sent by a user are not expected to cause any notification actions when | ||
| they are received by that same user (whether on the same or a different | ||
| device), so are excluded from this process. |
There was a problem hiding this comment.
There was discussion in other MSCs about allowing a user to call their other devices or transfer files between their devices. How does this rule affect that?
| Homeservers should check the content of any uploaded | ||
| `m.notification_attribute_data` events, and reject any malformed events with a | ||
| 400 error and `M_BAD_JSON`. However, implementations should be resilient to | ||
| malformed data. |
There was a problem hiding this comment.
What is malformed? Is malformed any value the server doesn't understand such as extra attributes? Or is it only known keys with invalid values? (ex: "mxid": 17)
| strings in the `keywords` list matches, the `m.keyword` attribute should be | ||
| set. | ||
|
|
||
| If any entry in the `keywords` list is not a simple string, that entry is |
There was a problem hiding this comment.
What is a "simple string"?
Why not reject non-string elements as specified earlier?
|
|
||
| * `room_notif`: a case-*insensitive* match for the string `@room`, provided | ||
| the sender of the event has power level greater or equal to that required | ||
| for `notifications`. |
There was a problem hiding this comment.
Is this the state of the art? I would expect that sending a message such as You can notify everyone in the room by sending `@room`. to not notify people. I see that this is currently not the case but do we want to 1. solve this here or at least 2. explicitly defer and suggest how this may be fixed? (Maybe by adding a room_notif_precise key in the future to replace this one or that this algorithm is expected to be change to be precise in the future?
|
|
||
| The `profile` property refers to an `account_data` event of type | ||
| `m.notifications_profile.<profile>`, and is composed of the characters `[a-z]`, | ||
| `[0-9]`, `-`, and `_`. |
There was a problem hiding this comment.
Why are we storing data (the profile key) in the event type? Is there precedent for this?
| any malformed events with a 400 error and `M_BAD_JSON`. However, | ||
| implementations should be resilient to malformed data. Homeservers should allow | ||
| unrecognised actions or attributes to be referenced in the mapping, to allow | ||
| future extension. |
There was a problem hiding this comment.
Is this desired? How will the client know if an attribute or action is supported by the server? Users will be confused if they enable m.file_transfer in their client but don't get notified when a file transfer starts.
| The `none` profile id is reserved, and servers should not allow clients to | ||
| upload `m.notifications_profile.none` events. This can be used to disable all | ||
| notifications without discarding other configuration that the user may have | ||
| set. |
There was a problem hiding this comment.
Should we namespace special profiles? Or are we confident that we won't need any past none?
|
|
||
| We don't specify a "default" notification profile, leaving it up to the server | ||
| implementation to provide sensible defaults. The example shown above is | ||
| anticipated to be a good starting point. |
There was a problem hiding this comment.
While we don't specify a default profile should we name the default profile so that clients can agree?
m.defaultis the default profile for new devices. Otherwise a normal profile.m.desktopis the default profile for desktop devices, falls back tom.default.m.mobileis the default profile for mobile devices, falls back tom.default.
Of course clients are welcome to use their own defaults but it might be nice to standardize on the "default default".
| * `keywords`: an array of strings which form "notification keywords". See | ||
| `m.keyword` below. If `keywords` is absent, the list is implicitly empty. |
There was a problem hiding this comment.
Is order significant? Should the client or server sort them to prevent anyone from depending on it?
turt2live
added
the
needs-implementation
|
It seems there's nothing stopping this being merged. When can we see this happening? |
well, the lack of an implementation is a farirly major blocker. I don't know of any immediate plans to implement it. |
| This event can be set at the global level. (Room-level overrides will be a | ||
| future extension.) |
There was a problem hiding this comment.
I think that room-level overrides are a critical part of this, leaving it out seems half-baked. If this were to replace push rules' core functionality, I think that room-level overrides (muting some, making others verbose), something people enjoy on many clients today, has to be a core part of this.
There was a problem hiding this comment.
this is broken thinking: you are saying "there is no point doing anything unless you fix all the world's problems". If you follow that route, nothing will ever be done, because fixing all the world's problems is impossible.
The only way we can ever hope to make any progress is by splitting down the problem and taking an iterative approach.
There was a problem hiding this comment.
My intention isn't to fix all problems at once, I'm just saying that this is a critical feature for a MVP that makes a good comparison to push rules
There was a problem hiding this comment.
Causing regressions in features when changing underlying components of software isn't good for users.
Rendered
(this was based on a previous draft at https://docs.google.com/document/d/1s6oGuScNSe4O5uTCC_9lZ7RFF3Ps98u0SI4kAhe4Xoc)