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

MSC3517: "Mention" Pushrule #3517

Closed

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Nov 21, 2021

Rendered

Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>

Related: matrix-org/matrix-spec#353


Preview: https://pr3517--matrix-org-previews.netlify.app

@ShadowJonathan ShadowJonathan changed the title "Mention" Pushrule MSC3517: "Mention" Pushrule Nov 21, 2021
@uhoreg uhoreg added kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal labels Nov 21, 2021
@uhoreg
Copy link
Member

uhoreg commented Nov 21, 2021

I've marked this as kind:maintenance because it feels to me like just an update to push rules, rather than a new feature. But someone who's more familiar with push rules can feel free to disagree with me and change it to kind:feature.

@ShadowJonathan
Copy link
Contributor Author

I've marked this as kind:maintenance

It was completely intended like that, thanks enough though.

@richvdh
Copy link
Member

richvdh commented Nov 22, 2021

related: matrix-org/matrix-spec#353, #2463

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe. Updating the default push rules is a lot harder than just adding a new rule - you've only thought about the easy bits ;)

proposals/3517-mention-pushrule.md Outdated Show resolved Hide resolved

## Proposal

This proposal aims to change that, adding the following push rule;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can guess, but where in the list of predefined push rules should this be inserted?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and do we keep or get rid of the existing mention rules?

Copy link
Contributor Author

@ShadowJonathan ShadowJonathan Nov 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo, keep the existing mention rules (displayname, username), as i know they're quite useful for some other users, this is just about adding additional options. We can bikeshed about un-defaulting them or removing them later (in another MSC).


## Proposal

This proposal aims to change that, adding the following push rule;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to specify a whole lot of detail about migrations:

  • what should servers do for existing users? should they add the new rule in, or not? if so, should they consider whether the user has changed the default settings for other rules?
  • How should a client behave when it encounters a user without the new rule? It may depend on whether the server supports the new rule or not.
  • How should the server behave when a client tries to modify the existing displayname/localpart mention rules? should it magically update the new rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the conservative approach would be to disable this, and let clients detect and auto-enable this.

However, that would just shove the problem to clients, and it doesn't tell anything about what if client A doesnt "know" this push rule, and B does, and enables it.

The alternative (just enabling this) would exacerbate the above problem.


I think, this being default-on (atm), that the server should just enable this once it encounters a user without this push-rule installed, be it that it lazily detects this upon push-rule changes, or as a background job when booting into the new version.

I do think that, if this was enabled while unstable, the server should first try to migrate over the unstable prefix, instead of defaulting and auto-enabling (as it could be quite annoying for some users)

Or, while the rule is unstable, the rule should be non-default, this could give a grace period in which clients and servers learn of this rule, before it switches to stable and becomes default-on.

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@ShadowJonathan
Copy link
Contributor Author

I want to deprecate this MSC in favour of MSC3952.

This MSC wasn't ever meant to fully solve the problem, but to wedge a solution in that would alleviate a large annoyance today, but I see MSC3952 solving this problem completely. Secondly, MSC3952 also templates exactly what I saw in a "follow up" or "more serious" MSC after this one, so having this superseded in favour of it is logical.

@turt2live turt2live added the obsolete A proposal which has been overtaken by other proposals label Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants