-
Notifications
You must be signed in to change notification settings - Fork 380
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
MSC4222: Adding state_after
to sync v2
#4222
base: main
Are you sure you want to change the base?
Conversation
36f8310
to
87fb070
Compare
87fb070
to
55e34c9
Compare
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.
Implementation requirements:
Clients will not be able to tell when a state change happened within the timeline. This was used by some clients to | ||
render e.g. display names of users at the time they sent the message (rather than their current display name), though | ||
e.g. Element clients have moved away from this UX. This behavior can be replicated in the same way that clients dealt | ||
with messages received via pagination (i.e. calling `/messages`), by walking the timeline backwards and inspecting the | ||
`unsigned.prev_state` field. While this can lead to incorrect results, this is no worse than the previous situation. |
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 kind of usage of the timeline section should be not impacted at all.
We still send all state events (even if they might be wrong) in the timeline section so clients using those state event to render things in the timeline should be not impacted at all.
Clients will not be able to tell when a state change happened within the timeline. This was used by some clients to | |
render e.g. display names of users at the time they sent the message (rather than their current display name), though | |
e.g. Element clients have moved away from this UX. This behavior can be replicated in the same way that clients dealt | |
with messages received via pagination (i.e. calling `/messages`), by walking the timeline backwards and inspecting the | |
`unsigned.prev_state` field. While this can lead to incorrect results, this is no worse than the previous situation. | |
As before clients will not be able to tell when a state change happened within the timeline. This was used by some clients to | |
render e.g. display names of users at the time they sent the message (rather than their current display name), though | |
e.g. Element clients have moved away from this UX. | |
This MSC allows to compute the current state correctly. It does not fix the behavior for the state history in the timeline. So clients using such an approach still will face the same issues they had before this MSC. | |
A proper solution for this usecase would be to paginate back state using the `replaces_state` id. With this one can find all actual state event ids. This list of id's can be used in combination with the timeline history to filter for invalid and valid state changes. |
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.
I guess what I wanted to make clear was that clients will need to change how they calculate the "state at each event", and that that behaviour is really considered best-effort at best.
> Both responses are the same, but the client **MUST NOT** update its state with the event. | ||
|
||
|
||
## Potential issues |
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 format of returned state in
required_state
is a list of events. This does now allow the server to indicate if a "state reset" has happened which removed an entry from the state entirely (rather than it being replaced with another event).
Does this issue from MSC4186: Simplified Sliding Sync apply to this MSC too?
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.
Mmm, that is true. I guess there is an open question as to whether we want to try and fix that here or not 🤔
Co-authored-by: Andrew Morgan <[email protected]>
Co-authored-by: Andrew Morgan <[email protected]>
This is to allow clients to opt-in to a change of the sync v2 API that allows them to correctly track the state of the room.
Rendered