-
Notifications
You must be signed in to change notification settings - Fork 297
ffi: RoomInfo notification mode must reflect the user-defined mode #2545
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
Conversation
@@ -63,7 +63,7 @@ impl RoomInfo { | |||
joined_members_count: room.joined_members_count(), | |||
highlight_count: unread_notification_counts.highlight_count, | |||
notification_count: unread_notification_counts.notification_count, | |||
notification_mode: room.notification_mode().await.map(Into::into), | |||
notification_mode: room.notification_mode(true).await.map(Into::into), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really clear what (true)
means as an argument list for .notification_mode()
. Maybe we should have separate notification_mode
and user_defined_notification_mode
or notification_mode
and effective_notification_mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I've added a separate user_defined_notification_mode()
function.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2545 +/- ##
==========================================
- Coverage 77.48% 77.46% -0.03%
==========================================
Files 180 180
Lines 19364 19369 +5
==========================================
- Hits 15005 15004 -1
- Misses 4359 4365 +6
☔ View full report in Codecov by Sentry. |
This is required to fix element-hq/element-x-ios#1648 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question, looks good otherwise. We can merge if you disagree.
@@ -63,7 +63,7 @@ impl RoomInfo { | |||
joined_members_count: room.joined_members_count(), | |||
highlight_count: unread_notification_counts.highlight_count, | |||
notification_count: unread_notification_counts.notification_count, | |||
notification_mode: room.notification_mode().await.map(Into::into), | |||
notification_mode: room.user_defined_notification_mode().await.map(Into::into), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need notification_mode
anymore? Would it make sense to name the notification_mode
field the same way as the method we're calling, i.e. user_defined_notification_mode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes. Ok, I've renamed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good.
Oh, except |
With this PR, the notification mode in
RoomInfo
now reflects the user-defined mode.