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

Refactors in _generate_sync_entry_for_rooms #11515

Merged
merged 13 commits into from
Dec 7, 2021
Merged

Refactors in _generate_sync_entry_for_rooms #11515

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
36357f3
Move sync_token up to the top
Dec 6, 2021
f9dff2d
Pull out _get_ignored_users
Dec 6, 2021
0af0643
Try to signpost the body of `_generate_sync_entry_for_rooms`
Dec 6, 2021
963de4e
Pull out _calculate_user_changes
Dec 6, 2021
bca977c
Add docstring for _calculate_user_changes
Dec 6, 2021
0e2c6f6
Introduce membership_changes_summary
Dec 6, 2021
463d2fd
Changelog
Dec 6, 2021
d8aa9d0
Update synapse/handlers/sync.py
Dec 6, 2021
dba7c94
docstring for _get_ignored_users
Dec 6, 2021
ecccfb4
Note that we could use a DB table
Dec 6, 2021
c384789
Revert "Introduce membership_changes_summary"
Dec 6, 2021
dd39d82
Remove unnecessary quote marks.
Dec 6, 2021
d9cf23d
Make `_calculate_user_changes` a method
Dec 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 32 additions & 24 deletions synapse/handlers/sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1593,30 +1593,10 @@ async def handle_room_entries(room_entry: "RoomSyncResultBuilder") -> None:
# 5. Work out which users have joined or left rooms we're in. We use this
# to build the device_list part of the sync response in
# `_generate_sync_entry_for_device_list`.
newly_joined_or_invited_or_knocked_users = set()
newly_left_users = set()
if since_token:
for joined_sync in sync_result_builder.joined:
it = itertools.chain(
joined_sync.timeline.events, joined_sync.state.values()
)
for event in it:
if event.type == EventTypes.Member:
if (
event.membership == Membership.JOIN
or event.membership == Membership.INVITE
or event.membership == Membership.KNOCK
):
newly_joined_or_invited_or_knocked_users.add(
event.state_key
)
else:
prev_content = event.unsigned.get("prev_content", {})
prev_membership = prev_content.get("membership", None)
if prev_membership == Membership.JOIN:
newly_left_users.add(event.state_key)

newly_left_users -= newly_joined_or_invited_or_knocked_users
(
newly_joined_or_invited_or_knocked_users,
newly_left_users,
) = _calculate_user_changes(sync_result_builder)

return (
set(newly_joined_rooms),
Expand Down Expand Up @@ -2353,6 +2333,34 @@ class SyncResultBuilder:
to_device: List[JsonDict] = attr.Factory(list)


def _calculate_user_changes(
sync_result_builder: "SyncResultBuilder",
) -> Tuple[Set[str], Set[str]]:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be start of SyncResultBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean here, sorry. Perhaps that this ought to be placed above the definition of SyncResultBuilder?

Oh, actually I think you mean the quotes are unnecessary.

(My personal preference would be to move SyncResultBuilder and all other POD structs/classes all the way up to the top of the file, so the quotes aren't needed throughout.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trimmed quotes in dd39d82.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant to say was:

I wonder if this should be part of SyncResultBuilder?

As in a method on that class! Clearly I failed to say anything reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I see. I considered this, but opted to make _calculate_user_changes a free function, to try and suggest that it's just reading data and not mutating the SyncResultBuilder instance.

I think this is just an old C++ habit dying hard though. Happy to go with whatever you think is clearest!

Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine either way, SyncResultBuilder seems to just be a way to pass around mutable state so probably best to keep logic out of it.

newly_joined_or_invited_or_knocked_users = set()
newly_left_users = set()
if sync_result_builder.since_token:
for joined_sync in sync_result_builder.joined:
it = itertools.chain(
joined_sync.timeline.events, joined_sync.state.values()
)
for event in it:
if event.type == EventTypes.Member:
if (
event.membership == Membership.JOIN
or event.membership == Membership.INVITE
or event.membership == Membership.KNOCK
):
newly_joined_or_invited_or_knocked_users.add(event.state_key)
else:
prev_content = event.unsigned.get("prev_content", {})
prev_membership = prev_content.get("membership", None)
if prev_membership == Membership.JOIN:
newly_left_users.add(event.state_key)

newly_left_users -= newly_joined_or_invited_or_knocked_users
return newly_joined_or_invited_or_knocked_users, newly_left_users


@attr.s(slots=True, auto_attribs=True)
class RoomSyncResultBuilder:
"""Stores information needed to create either a `JoinedSyncResult` or
Expand Down