-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
Make clear the thread root is not in the thread #1677
Make clear the thread root is not in the thread #1677
Conversation
Signed-off-by: Andy Balaam <[email protected]>
I believe this should have the spec-bug label but I don't have permission to add it. |
I think the changelog entry should be |
We use different suffixes in spec to other projects: https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst#adding-to-the-changelog. As for labels: we don't normally bother with them on PRs for the spec. |
@richvdh I was going by:
From https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst#other-changes I guess this does fit under "clarification" from https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst?rgh-link-date=2023-11-22T11%3A20%3A43Z#adding-to-the-changelog but if a |
Co-authored-by: Patrick Cloke <[email protected]>
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.
Generally this looks good to me with a few comments.
For future PRs, please avoid re-flowing entire paragraphs as it makes it difficult to see what the changes actually are.
one with an `m.relates_to` property). It is therefore also not possible to nest | ||
threads. All events in a thread reference the thread root instead of the | ||
most recent message, unlike rich reply chains. | ||
*thread root*. It is not possible to create a thread from an event which itself |
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've now lost the definition of a "thread root" 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.
Attempted a definition in 058e97e . I removed the original definition because it was not accurate (because it said the thread root was in the thread, which it is not).
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.
Thanks!
This was originally written up as MSC4037, but discussion there concluded that a spec PR was more appropriate.
Quoting @turt2live in that discussion:
This PR modifies the spec to make clear that thread roots and their non-thread children are not "in the thread" for the purposes of receipt handling and unreadness.
Synapse has the behaviour specified in this PR.
matrix-js-sdk was recently fixed to match the behaviour required by this PR, as a bug fix for stuck notifications bugs resulting from inconsistency between Synapse and Element Web.
Signed-off-by: Andy Balaam [email protected]
Preview: https://pr1677--matrix-spec-previews.netlify.app