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
MSC4103: Make threaded read receipts opt-in in /sync #4103
base: main
Are you sure you want to change the base?
Conversation
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:
- Client using the opt-in flag to enable the behaviour.
- Client not using the opt-in flag which clearly fixes the bug described.
them `m.read` events as received by the server. If this filter is missing or false, then the server must only send the | ||
client `m.read` events whose `thread_id` is missing or `main`. |
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 think this is an API breaking change, because one would expect to receive threaded receipts from /sync by default. So, to preserve backwards compatibility with clients, how about making it opt-out? That is, thread receipts would be sent if the filter is true
or missing, and not sent if it's set to false
.
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.
..if we're having to add a filter to filter them out for thread unaware clients then why not just ask clients to check for thread_id: main? I think Matthew's point is that it was a breaking change to include threaded RRs by default, and this is trying to fix it. This puts the onus on threaded clients to add this filter flag to ask for threaded receipts. I agree it is a breaking change from the current status quo though.
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 think this MSC also implies a server-side change: now a server can't merge a threaded receipt with an unthreaded receipt on the same event id anymore, because two clients might request the two different flavors of the filter. Am I understanding it right?
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 don't see this anywhere in the MSC. The proposal is just:
- add
threaded_read_receipts
as a sync filter param. - default to ??? if missing (default=true doesn't break threaded clients today, default=false breaks threaded clients but unbreaks thread unaware clients which were broken when MSC3771 landed.
- if set, send ALL THE RECEIPTS. If unset, only send unthreaded/thread_id:main receipts.
Co-authored-by: Benjamin Bouvier <public@benj.me>
@@ -0,0 +1,77 @@ | |||
# MSC4103: Make threaded read receipts opt-in in /sync |
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.
We could solve this by making threaded RRs a different event type (e.g. `m.read_thread`), so non-thread-aware clients | ||
would automatically ignore them. However, this would require threaded clients to send double the read receipts whenever | ||
the user reads the main thread - both an `m.read` and an `m.read_thread`, which feels inefficient. (It would however | ||
mean that [MSC4102](https://github.com/matrix-org/matrix-spec-proposals/pull/4102) would not be necessary, as the RR types | ||
would be clearly distinct). |
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 was rejected by MSC3771 FWIW (see https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/3771-read-receipts-for-threads.md#receipt-type). Not saying it shouldn't be revisited, just noting this.
This means that non-thread-aware clients are not spammed or confused with irrelevant threaded read receipts, and can | ||
calculate read state purely based on main timeline activity. | ||
|
||
## Alternatives |
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.
Unsure it is even worth considering, but MSC3773 did add the unread_thread_notifications
flag to filters which essentially means "I understand threads". It could be re-used, but that feels a bit awful. Is there a situation where a client might realistically set one and not the other?
Rendered
Disclaimer: Written with my SCT hat on. (Other hats include SCT lead, Matrix project lead, Matrix guardian, CEO/CTO at Element)