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.

Sign up for GitHub

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

Jump to bottom

SS: Hook up notification counts #17546

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

SS: Hook up notification counts #17546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
98b3f56
SS: Hook up notification counts
erikjohnston Aug 9, 2024
ba7969b
Newsfile
erikjohnston Aug 9, 2024
f7ad92e
Add tests for notification counts
erikjohnston Aug 16, 2024
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
1 change: 1 addition & 0 deletions changelog.d/17546.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Return correct per-room notification and highlight counts in experimental sliding sync implementation.
13 changes: 8 additions & 5 deletions synapse/handlers/sliding_sync.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2364,6 +2364,11 @@ async def get_room_sync_data(

set_tag(SynapseTags.RESULT_PREFIX + "initial", initial)

notif_counts = await self.store.get_unread_event_push_actions_by_room_for_user(
room_id,
sync_config.user.to_string(),
)
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved

return SlidingSyncResult.RoomResult(
name=room_name,
avatar=room_avatar,
Expand All @@ -2384,11 +2389,9 @@ async def get_room_sync_data(
invited_count=room_membership_summary.get(
Membership.INVITE, empty_membership_summary
).count,
# TODO: These are just dummy values. We could potentially just remove these
# since notifications can only really be done correctly on the client anyway
# (encrypted rooms).
notification_count=0,
highlight_count=0,
# TODO: What about notifications in threads?
notification_count=notif_counts.main_timeline.notify_count,
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this field optional for encrypted rooms please? It's very likely to be inaccurate for such rooms. It's better to return nothing than an incorrect value I think.

highlight_count=notif_counts.main_timeline.highlight_count,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this PR given the latest from ElementX/Rust team is

unread counts sent from the server are indeed thrown away in the Rust SDK, since they can't be precisely computed b/o encryption

Although

I think they are used as a fallback

And we're not sure if the app even shows notification counts.

And we seem to agree that notification counts from the servers perspective seem pointless given encrypted rooms. Perhaps we should just be removing these fields?

Copy link
Contributor

Choose a reason for hiding this comment

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

@erikjohnston asked for further clarification in an internal room on Friday but nothing has materialized yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

The topic came up again in an internal room. Here is a summary:

There was a strong feeling to get rid of them since there is no point in sending known bad data to clients. The highlight_count/notification_count won't be accurate or make sense in end-to-end encrypted (E2EE) rooms (the client needs to decrypt the messages to see what they actually are or if they mention the user). @stefanceriu and @ara4n also mentioned that even in unencrypted rooms, they are an approximation because the server doesn't know what the client can render. Not sure that I follow the point about unencrypted rooms as it seems like the push rules could align with whatever your client considers.

Product-wise, the Rust SDK ignores highlight_count/notification_count and the Element X apps don't have unread counts, only unread badges (product decision) even though WhatsApp/Telegram do have them.

For clients, that do want to show the unread count, the highlight_count/notification_count is useful for big public chat rooms. Otherwise, the client needs to spider the entire room (probably up to your read-receipt) to see if you're mentioned which feels inefficient. @ara4n was originally in favor of dropping these fields but now considers them useful to keep and clients should ignore them for E2EE encrypted rooms and just calculate it themselves by spidering on the client.

)

@trace
Expand Down
Loading