-
Notifications
You must be signed in to change notification settings - Fork 139
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
Add warning message to 'mentions and keywords only' notification option #2028
Add warning message to 'mentions and keywords only' notification option #2028
Conversation
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2028 +/- ##
===========================================
- Coverage 67.70% 67.67% -0.03%
===========================================
Files 1329 1329
Lines 33418 33413 -5
Branches 7192 7202 +10
===========================================
- Hits 22626 22613 -13
- Misses 7384 7385 +1
- Partials 3408 3415 +7 ☔ View full report in Codecov by Sentry. |
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 for the PR. 2 remarks, else LGTM!
.../android/features/roomdetails/impl/notificationsettings/RoomNotificationSettingsPresenter.kt
Outdated
Show resolved
Hide resolved
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.
This is the UI displayed for a room, so maybe the disclaimer message can be more accurate like:
Your homeserver does not support this option for this room, you may not get notified.
instead of reusing the wording for the global setting.
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 the same string we use on iOS. I guess we can iterate on this later, since there is already an UI polishing planned?
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
It should be displayed when it's not supported by the homeserver
… is encrypted Co-authored-by: Benoit Marty <[email protected]>
88711bc
to
94be166
Compare
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
Type of change
Content
NotificationSettingsService. canHomeServerPushEncryptedEventsToDevice()
method to check if the home server can filter notifications for encrypted events containing mentions and keywords (the naming in Rust is a bit weird).displayMentionsOnlyDisclaimer
parameter to all notification screens and components.ListItem
.Motivation and context
Closes #1794 .
Screenshots / GIFs
Inside the PR.
Tests
Tested devices
Checklist