Skip to content
This repository has been archived by the owner on Mar 26, 2024. It is now read-only.

No read receipts in large rooms #67

Open
wants to merge 8 commits into
base: beeper
Choose a base branch
from

Conversation

incognitobeeper
Copy link

@incognitobeeper incognitobeeper commented Mar 3, 2023

Overview

Prevents read receipts being sent when rooms are over a defined size, whilst preserving the behaviour of read/unread notifications. This has been achieved by converting receipts types to be private when the room size is large, as this is the simplest method I've found to decouple ephemeral receipt notification and read/unread notifications.

@incognitobeeper incognitobeeper force-pushed the no-read-receipts-in-large-rooms branch from f0f1137 to fcd34c3 Compare March 3, 2023 21:23
@bradtgmurray
Copy link
Member

"com.beeper.inbox.done" is ofc not in the matrix protocol, so I'm not certain how it should be handled. Can "com.beeper.inbox.done" be handled the same as the other receipt types?

Whoops, we should actually remove this code, we're no longer using that type.

@incognitobeeper incognitobeeper force-pushed the no-read-receipts-in-large-rooms branch 3 times, most recently from 20569b5 to 28780f8 Compare March 6, 2023 22:18
@incognitobeeper incognitobeeper force-pushed the no-read-receipts-in-large-rooms branch from 28780f8 to f74f4bf Compare March 7, 2023 14:31
@incognitobeeper incognitobeeper marked this pull request as ready for review March 7, 2023 18:20
# so we convert messages to private, that are over RECEIPT_MAX_ROOM_SIZE.
for i, r in enumerate(receipts):
if r.receipt_type != ReceiptTypes.READ_PRIVATE:
num_users = await self.store.get_number_joined_users_in_room(r.room_id)
Copy link
Member

Choose a reason for hiding this comment

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

Should we gather all of these ahead of time? Also a little clunky awaited on these one at a time, can we bulk grab these with twisted async somehow? @Fizzadar plz

Copy link
Member

Choose a reason for hiding this comment

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

Note this handles federation as well so there could be up to 100 read receipts here

Copy link
Author

Choose a reason for hiding this comment

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

updated to ensure we only check the size of each room once beeper/synapse@fbcd582

But would be great to hear if anyone knows how to do the equivalent of asyncio.gather in twisted, I haven't found it myself yet....

Copy link
Member

Choose a reason for hiding this comment

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

There’s a gatherResults that roughly does the same!

Copy link
Author

@incognitobeeper incognitobeeper Mar 8, 2023

Choose a reason for hiding this comment

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

Nice, I'll check that out when I next get a chance to work on this. thanks @Fizzadar

@incognitobeeper
Copy link
Author

@Fizzadar This is ready to go so feel free to merge. But if you would like a warranty with it (i.e. me to monitor it) then you'll need to wait till I join (start of May). Up to you....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants