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

MSC4005: Explicit read receipts for sent events #4005

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bradtgmurray
Copy link

@bradtgmurray bradtgmurray commented May 1, 2023

@bradtgmurray bradtgmurray changed the title MSCXXXX: Explicit read receipts for sent events MSC4005: Explicit read receipts for sent events May 1, 2023
@uhoreg uhoreg assigned uhoreg and unassigned uhoreg May 1, 2023
@uhoreg uhoreg added proposal A matrix spec change proposal client-server Client-Server API needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. kind:maintenance MSC which clarifies/updates existing spec labels May 1, 2023
bradtgmurray and others added 5 commits May 29, 2023 07:37
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
@bradtgmurray bradtgmurray force-pushed the explicit-read-receipts-on-event-send branch from 12d5f8c to 3f5f2e0 Compare May 29, 2023 11:37
@bradtgmurray
Copy link
Author

FYI, I'm going to take a shot at implementing this in Synapse over the next few weeks when I find some time.

"where the user has read up to" is simplified to just a read receipt, where previously it was the
greater value of the user's read receipt and their last sent event. If the event is "in a thread"
(using the definition from [threaded read receipts](https://spec.matrix.org/v1.7/client-server-api/#threaded-read-receipts))
the generated read receipt should be a threaded read receipt.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I read this correctly, I think it is saying that if an event is in the main timeline, the server should generate an unthreaded read receipt.

I think this would produce unexpected behaviour in a thread-aware client: if I send a message in the main timeline in my thread-aware client, I would not expect all unread threads to become read.

It's possible we need the client to tell the server whether the event it is sending is within the main timeline in a threaded context, or in an unthreaded context.

Copy link
Author

@bradtgmurray bradtgmurray Aug 4, 2023

Choose a reason for hiding this comment

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

While I agree this isn't ideal, I think it's no worse than the status quo today. I believe that for the purpose of calculating notification counts for example, the homeserver will clear notification counts from threads when a message is sent to the the main timeline.

I agree with your thinking on how to solve it (clients need to declare their thread-awareness when sending a message somehow) but I also think this could me merged without tackling this problem.

See this thread here: #4005 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I believe that for the purpose of calculating notification counts for example, the homeserver will clear notification counts from threads when a message is sent to the the main timeline.

Given matrix-org/synapse#14837 I find this hard to believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants