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

Threads: Improve Notifications for Exit Beta #196

Closed
6 tasks done
Tracked by #187
daniellekirkwood opened this issue Mar 5, 2022 · 4 comments
Closed
6 tasks done
Tracked by #187

Threads: Improve Notifications for Exit Beta #196

daniellekirkwood opened this issue Mar 5, 2022 · 4 comments

Comments

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented Mar 5, 2022

We need to close the gap to the notifications system depicted in the figma spec.

The Exit Beta Milestone Notifications requirements are here

We have the following problems:

  1. The clients do not sync the thread read state in response to users reading threads, so marking a thread as unread on one device doesn’t mark it as unread on the other devices.
  2. The room's read counter is based on the main timeline’s read receipt, which doesn’t consider threaded messages separately, so reading the main timeline decrements any count due to threaded messages in that period even though they aren’t visible in the main timeline.
  3. According to the Figma, every updated thread that a user has participated in should be counted as one in the thread icon’s counter badge on the room header. That is, the number on that badge equals the number of updated threads that the user has participated in. (currently, we don't show a number, just a dot)
  4. According to the Figma, the total count in the thread icon badge should be used to calculate the total count in the room and space badges of the left sidebar (needs discussion)

We have a proposal to add threading support to read receipts, which solves 1, 2 and 3

Implementation

After implementing two proof of concepts, element-hq/element-web#22980 & element-hq/element-web#22981 we have confidence in the approach that we want to take

@clokep
Copy link
Contributor

clokep commented Apr 25, 2022

The work here resulted in 3 MSCs which should enable the features we need:

  • MSC3771: Read receipts for threads
  • MSC3772: Push rule for mutually related events
  • MSC3773: Notifications for threads

There are some open questions on them that are still being worked through.

(Presentation with some background info.)

@clokep
Copy link
Contributor

clokep commented Jun 30, 2022

From working on implementing this the original design has changed a little bit; the current plan is to:

  • Implement a proof-of-concept for thread-specific read receipts (this is essentially MSC3771, although that might need some updates).
    • This will likely run into some issues if you're using both a threaded and unthreaded client, but we need to see how bad this is in reality before discarding this solution.
    • An old branch exists which does this, but some recent changes in Synapse might conflict with it -- it needs to be dusted off.
    • There are open questions on the MSC, including how it interacts with private read receipts. I think this can be punted until after the PoC is finished.
  • Notification counts for threads -- implement the (now simplified) MSC3773.
    • This is pretty much done, but the work was bitrotted by some performance improvements the Synapse team landed and after rebasing the tests are failing.
    • The current work also needs some minor improvements: tests, experimental config flag, flag for /sync, but that should all be fairly straightforward.
  • Limiting notifications to only threads which a user has participated in -- this is covered by MSC3772, but might need to be updated a bit.
    • This is seen as a refinement to the above and will be done after the proof-of-concept.

Originally MSC3773 depended on MSC3771 and MSC3772, but after some rewrites this is no longer true. MSC3772 will likely end up depending on MSC3773, however.


In order to solve the issue with threaded and unthreaded clients it was briefly investigated if using "ranged read receipts" could help here (see notes, proto-MSC). For now this is being shelved since the user experience is unclear. It might be revisited based on the result of the proof-of-concept.

@daniellekirkwood
Copy link
Contributor Author

We're removing the need for MSC3772 from Exit Beta as we've changed the default settings for the MVP of Threads notifications. In the future we should build 3772 but not for this milestone.

More info on what we are doing here: https://docs.google.com/document/d/1DYyDsf24OlrME3h-Hg-H2GvCrKkqZJrQwIbNWjqxB2A/edit?usp=sharing

@germain-gg
Copy link

Closing as all items have been ticked off

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants