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

Always mark legacy threaded event as read #3020

Closed
wants to merge 7 commits into from

Conversation

germain-gg
Copy link
Contributor

@germain-gg germain-gg commented Jan 4, 2023

Needed for matrix-org/matrix-react-sdk#9763
Will help element-hq/element-web#23907

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Always mark legacy threaded event as read (#3020).

Comment on lines 517 to 521

// We consider all threaded events read if they are part of a thread
// that has no activity since the first ever threaded event recorded in that room
// This prevents rooms to generated unwanted notifications for threads
// created before MSC3771
Copy link
Member

Choose a reason for hiding this comment

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

I'm finding this comment very difficult to read, maybe the following is a bit clearer:

Suggested change
// We consider all threaded events read if they are part of a thread
// that has no activity since the first ever threaded event recorded in that room
// This prevents rooms to generated unwanted notifications for threads
// created before MSC3771
// We consider all events in a thread as read if the thread has not had
// activity since the first ever threaded receipt recorded in that room.
// This prevents rooms generating unwanted notifications for threads
// created before MSC3771.

Comment on lines +2728 to +2731

// Some threads were created before MSC3771 landed. Those threads
// do not have read receipts, and this will be problematic in encrypted
// rooms where clients rely on receipts to compute highlight notifications
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually necessary for more than the "bold" case of thread activity?

@germain-gg
Copy link
Contributor Author

Replaced by #3031

@germain-gg germain-gg closed this Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants