Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MSC3664: Pushrules for relations #3664
base: main
Are you sure you want to change the base?
MSC3664: Pushrules for relations #3664
Changes from all commits
d5f43bb
53a4607
fa8fc97
b90cdb6
a6eebe7
0161daa
242a65c
070babc
06b5393
f2a5c57
aebe3e9
1ead1da
6e1d1bb
3a288b6
df9aab5
00b0ef9
519b95d
c9d55e2
22e67c4
7248826
69fe03e
b1897fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Aren't these identical to:
(And a corresponding one for
m.thread
.)I'm not sure this example really shows the need for this MSC?
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.
Well, in the case of replies you need to also match, if it isn't a fallback. Also, it interacts better with #3051 or any other reorganization of the relation format. Matching dotted keys is also #3873. Yes, you can solve this differently, but that solution also has downsides. This is just a convenient extension of this MSC, the actual meat is matching the content of the related event.
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.
for reference:
.m.rule.invite_for_me
currently implements such a pattern.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.
It might be helpful to number each separate issue.
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 doesn't really define the homeserver behavior here -- are they to attempt to backfill if they can't find the event? Just skip it?
Similarly for clients, should they attempt to ask the homeservers for the missing event if they don't have it locally?
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.
That's a quality of implementation issue and I don't think that needs to be defined. For example in the case of matching replies you usually already have the events sent from your server and don't need to backfill them. In threads this is similar, usually you have the participating thread already. If you don't have an event, it is a tradeoff. Do you want accurate notifications or low latency? Alternatively you can always update the notification counter asynchronously. I don't see a need to require any specific behaviour 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.
the threading MSC has landed, so changing it would mean a new MSC and changing a bunch of implementations. I suggest opening a separate issue for this explaining your concerns.
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.
Well, it is not really an issue, servers can work around that to carry the threading semantics forward or not. It does make the code quite ugly, but eh, that ship has sailed.
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
include_fallbacks
is on the event in question, not the related event, correct? I find it a bit weird thatrelated_event_match
matches some fields on the current event and other fields on the related event.(Can we also update the links to point the spec?)