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

Make clear the thread root is not in the thread #1677

Merged

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Nov 22, 2023

This was originally written up as MSC4037, but discussion there concluded that a spec PR was more appropriate.

Quoting @turt2live in that discussion:

MSC3771 (merged) originally introduced the threaded receipts behaviour into the spec in v1.4, but did not describe what actually constitutes what is "in a thread". Though, the implied wording of MSC3771 does not include the thread root in the thread for purposes of notifications. This is further backed up by Synapse being the primary cited implementation proof for MSC3771, which is backed up by this proposal's text in the above paragraph.

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 andy.balaam@matrix.org

Preview: https://pr1677--matrix-spec-previews.netlify.app

Signed-off-by: Andy Balaam <andy.balaam@matrix.org>
@andybalaam andybalaam requested a review from a team as a code owner November 22, 2023 10:41
@andybalaam
Copy link
Contributor Author

I believe this should have the spec-bug label but I don't have permission to add it.

@andybalaam
Copy link
Contributor Author

I think the changelog entry should be .bugfix but the validator appears to reject items with that name?

@richvdh
Copy link
Member

richvdh commented Nov 22, 2023

I think the changelog entry should be .bugfix but the validator appears to reject items with that name?

We use different suffixes in spec to other projects: https://github.com/matrix-org/matrix-spec/blob/main/CONTRIBUTING.rst#adding-to-the-changelog. clarification is right here, if the argument is just that the spec was "misleading".

As for labels: we don't normally bother with them on PRs for the spec.

@andybalaam
Copy link
Contributor Author

@richvdh I was going by:

Likewise, corrections to the specification, to fix situations where, in practice, servers and clients behave differently to the specification, including anything with the spec-bug label.

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 .bugfix existed I think it would fit better, since we are correcting the spec to conform with implementations, because those implementations do what we meant to say first time.

@andybalaam
Copy link
Contributor Author

@richvdh I was going by:

Likewise, corrections to the specification, to fix situations where, in practice, servers and clients behave differently to the specification, including anything with the spec-bug label.

Ah, but now I see that is referring to labels on issues, not PRs.

andybalaam and others added 3 commits November 27, 2023 16:26
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Copy link
Member

@turt2live turt2live left a 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.

content/client-server-api/modules/receipts.md Outdated Show resolved Hide resolved
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
Copy link
Member

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.

Copy link
Contributor Author

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).

content/client-server-api/modules/receipts.md Outdated Show resolved Hide resolved
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Thanks!

@turt2live turt2live merged commit 6fe2ff4 into matrix-org:main Nov 28, 2023
12 checks passed
@zecakeh zecakeh mentioned this pull request Nov 30, 2023
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants