Matrix-doc: Homeserver Warning Messages

[dbkr]

Documentation: https://docs.google.com/document/d/1R_a1zi5qRe06D6D3fNZaKTqQSyaKcQT2Ejt4kImt6yo/edit#

[KitsuneRal]

I believe that both options should be applied, each in its own case.
1 - Specifically for the case like reaching the limit there's a clear state semantics (I guess there's no sense in having two usage limit notices active at the same time).
2- A warning like "an administrator has been notified about your activity, watch out" can be sent as a pinned message rather than as a state event since this one has message semantics (and, as grim as it sounds, two such notices can coexist).
As for server notices room identification options - as a client author, I'd (naturally) prefer getting all server notices information along with a sync, so that I don't have to issue a separate request just to figure which room has a special status. On the other hand, I don't want to give even a backdoor for altering the configuration of that room. Therefore:
1 - /client/config is the worst: it's yet another call not only to locate the room but also to get updates.
2 - account_data - as mentioned, a server will have to explicitly disable tinkering with this account data key, or live with them being able to tinker.
3 - for me as a client author, it's not really different from 4. From the server perspective, 4 kinda wins if we forget about having to special-case this event for all APIs.
4 - as mentioned in the proposal, this doesn't work, _unless_ this event cannot be normally set (as in - the server rejects/ignores this event coming either through federation or from clients).
5 - this one is almost 4 except that it's not the room admin but the client tinkering so you'll have to enforce clients to behave (which is unlikely to work).

[dbkr]

Thanks @KitsuneRal : have moved comments onto the doc.

[dbkr]

I've nominated a set of options on the doc: moving this into 'ready to review' to gather any remaining feedback.

[turt2live]

Seems like a sensible conclusion to me. I wonder if we should just put it "in review" though given it's on the hot path.

[dbkr]

Yeah, probably.

[KitsuneRal]

Is the tag going to be m.server_notice or m.server_notices? The proposal is not consistent on the name, please rectify.

[dbkr]

@KitsuneRal I've accepted half-shot's correction which should fix this

[erikjohnston]

AFAIK we haven't yet told clients to render any event with a body key in the content, which I think we'd want so that the messages is actually displayed in clients that don't implement special handling. So I would vote to have the message as a m.room.message type with a msgtype of m.server_notice.* (or whatever), to more closely match the current event format semantics.

This raises another problem, that using a msgtype (or event type) as m.server_notice.* removes the ability to namespace custom server notices types. Ideally one would have another key to specify the type of server notice, so something like:

{
  "type": "m.room.message",
  "content": {
    "body": "This is a server notice",
    "msgtype": "m.server_notice",
    "server_notice_type": "m.resource_limit",
    "resource": "users",
    "limit": 50,
    "current": 49
  }
}

Or something.

[erikjohnston]

Personally, I think this should be a state event, as this feels exactly the sort of thing that state should be used for. However, we don't currently spec if or how clients should render state changes that they don't understand, so wouldn't work for clients that didn't implement the exact event types. In which case its probably the right call not to use it for now.

[turt2live]

Using msgtypes makes me eternally sad, as I really think we should just get extensible events (#1225) out the door before another event falls victim to not being displayed correctly.

If we're looking to get this through for r0 though, I'll still be sad about msgtype, but won't fight it (as it makes sense).

[erikjohnston]

Using msgtypes makes me eternally sad, as I really think we should just get extensible events (#1225) out the door before another event falls victim to not being displayed correctly.

Yeah, but I think it would be even worse if we started "randomly" making up rules as we went along.

[KitsuneRal]

In the meantime, while we still don't have extensible events, there's no difference for me as a client author between an entirely new type of events (no matter state or not); it will be rendered as "Unknown event" by Quaternion, and I need to actually add support of that new type before it does anything sensible. Meanwhile m.room.message with a new msgtype will cause a warning in the logs of current Quaternion/uMatriks/Matrique releases but the body will be displayed (hereby confirming Erik's statement).
And once we have extensible events, we can even turn server notices to state events.

[turt2live]

I spent a bit of time to check that the implementation and proposal vaguely match and I think they do. it also looks like the implementation went the route of using an m.room.message with an appropriate msgtype so the proposal just needs a bit of updating.

With the assumption that the proposal can be updated:

@mscbot fcp merge

[mscbot]

Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people:

• [x] @KitsuneRal
• [x] @anoadragon453
• [ ] @ara4n
• [x] @dbkr
• [ ] @erikjohnston
• [x] @richvdh
• [x] @turt2live
• [x] @uhoreg

Concerns:

• so what does "past month" mean, in the end? resolved by https://github.com/matrix-org/matrix-doc/issues/1452#issuecomment-481640173
• what uhoreg said resolved by https://github.com/matrix-org/matrix-doc/issues/1452#issuecomment-481594780

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[uhoreg]

The conclusion seems reasonable, but the proposal itself is a bit hard to read due to the presence of not only multiple options, but also multiple sets options. It would be nice if someone could edit the proposal so that it starts off by presenting what is actually being done, and then possibly listing the alternatives later on.

[erikjohnston]

This has been tested against matrix.org and its users, @ara4n you mentioned the other day that some people found it a bit confusing? Do we want to consider any changes to the approach before going forwards?

[richvdh]

@mscbot concern what uhoreg said

If we're doing a retrospective MSC after the fact (argh) please could the doc make clear what's been implemented, and whether there remain any changes?

[neilisfragile]

I've made some updates to the doc to reflect the actual implementation - feedback welcome on how to improve

[dbkr]

It was a while back but yes, I think this reflects the reality.

[richvdh]

looks plausible to me.

@mscbot resolve what uhoreg said
@mscbot reviewed

[turt2live]

apparently the bot doesn't like spelling errors or edits:

@mscbot resolve what uhoreg said

Edit: or me >:(

[anoadragon453]

The original author of the concern has to be the one to resolve it.

[KitsuneRal]

@mscbot concern so what does "past month" mean, in the end?

Given that the MSC's documenting the current behaviour, I'd really appreciate if the question about what "past month" means be resolved before accepting it. And yeah, the practice of hastily drafting MSCs in parallel with implementation and the actual spec PR dragging behind in half a year is less and less acceptable.

[richvdh]

@mscbot resolve what uhoreg said

[KitsuneRal]

@mscbot resolve so what does "past month" mean, in the end?

[mscbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[mscbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[turt2live]

I believe the implementations for this are over at:

• https://github.com/matrix-org/synapse/pull/3680
• https://github.com/vector-im/riot-web/issues/7091 (see associated PRs)

[turt2live]

Spec PR: https://github.com/matrix-org/matrix-doc/pull/2026

[turt2live]

merged 🎉