Skip to content
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

Homeserver Warning Messages #1452

Closed
dbkr opened this issue Jul 25, 2018 · 30 comments
Closed

Homeserver Warning Messages #1452

dbkr opened this issue Jul 25, 2018 · 30 comments
Assignees
Labels
disposition-merge kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Comments

@dbkr
Copy link
Member

dbkr commented Jul 25, 2018

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

@KitsuneRal
Copy link
Member

KitsuneRal commented Jul 31, 2018

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
Copy link
Member Author

dbkr commented Jul 31, 2018

Thanks @KitsuneRal : have moved comments onto the doc.

@dbkr
Copy link
Member Author

dbkr commented Aug 1, 2018

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

@turt2live
Copy link
Member

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
Copy link
Member Author

dbkr commented Aug 2, 2018

Yeah, probably.

@KitsuneRal
Copy link
Member

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

@dbkr
Copy link
Member Author

dbkr commented Aug 13, 2018

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

@erikjohnston
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator

mscbot commented Nov 12, 2018

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

Concerns:

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.

@mscbot mscbot added the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Nov 12, 2018
@KitsuneRal
Copy link
Member

@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
Copy link
Member

richvdh commented Apr 10, 2019

@mscbot resolve what uhoreg said

@KitsuneRal
Copy link
Member

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

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 10, 2019
@mscbot
Copy link
Collaborator

mscbot commented Apr 10, 2019

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

@mscbot mscbot added finished-final-comment-period and removed final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Apr 15, 2019
@mscbot
Copy link
Collaborator

mscbot commented Apr 15, 2019

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

@turt2live
Copy link
Member

I believe the implementations for this are over at:

@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels May 24, 2019
@turt2live turt2live self-assigned this May 26, 2019
turt2live added a commit that referenced this issue May 27, 2019
As per [MSC1452](#1452) 

Fixes #1254

Although MSC1452 focuses on just the warnings part of the server notices, the base for notices has not been established in the spec. This commit adds the needed support to be able to handle notices.

No intentional divergences from the proposal are included in this changeset. There are a few additions which are used in practice although not defined in the proposal, such as who is responsible for aesthetics, sending notices, and other misc rules.
turt2live added a commit that referenced this issue May 27, 2019
As per [MSC1452](#1452) 

Fixes #1254

Although MSC1452 focuses on just the warnings part of the server notices, the base for notices has not been established in the spec. This commit adds the needed support to be able to handle notices.

No intentional divergences from the proposal are included in this changeset. There are a few additions which are used in practice although not defined in the proposal, such as who is responsible for aesthetics, sending notices, and other misc rules.
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels May 27, 2019
@turt2live
Copy link
Member

Spec PR: #2026

@turt2live
Copy link
Member

merged 🎉

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels May 28, 2019
@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

No branches or pull requests

9 participants