Update notifications rules: make a sound for each notification #5796

Claire1817 · 2022-04-20T12:33:57Z

Type of change

Feature
Bugfix
Technical
Other :

Content

#4632
Align notification experience to web / ios client.
Notify with noise for each notification.

Motivation and context

Screenshots / GIFs

Tests

Step 1
Step 2
Step ...

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes has been tested on an Android device or Android emulator with API 21
[] UI change has been tested on both light and dark themes
[] Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

github-actions · 2022-04-20T13:14:02Z

Unit Test Results

122 files +  8 122 suites +8 2m 3s ⏱️ +44s
205 tests +  3 205 ✔️ +  3 0 💤 ±0 0 ❌ ±0
690 runs +12 690 ✔️ +12 0 💤 ±0 0 ❌ ±0

Results for commit f188eb6. ± Comparison against base commit 7c7822a.

This pull request removes 9 and adds 12 tests. Note that renamed tests count towards both.

org.matrix.android.sdk.api.pushrules.PushRuleActionsTest ‑ test_action_parsing
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_displayName_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_cake_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_cakelie_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_path_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_type_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_eventmatch_words_only_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_notice_condition
org.matrix.android.sdk.api.pushrules.PushRulesConditionTest ‑ test_roommember_condition

im.vector.app.features.crypto.UISIDetectorTest ‑ If event decrypted during grace period should not trigger detection
im.vector.app.features.crypto.UISIDetectorTest ‑ trigger detection after grace period
im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email
org.matrix.android.sdk.api.session.pushrules.PushRuleActionsTest ‑ test_action_parsing
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_displayName_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_cake_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_cakelie_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_path_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_type_condition
org.matrix.android.sdk.api.session.pushrules.PushRulesConditionTest ‑ test_eventmatch_words_only_condition
…

♻️ This comment has been updated with latest results.

ouchadam · 2022-04-20T13:31:59Z

some things to look out for when making changes to this area

spam when using a connected device like a smart watch/fitness band Notification keeps resending #1673
double notifications when receiving fastlane updates followed by a sync Doubled notifications #4152

Hopefully the combination of all fixes in this area might have solved these without setAlertOnce() 🤞

ouchadam · 2022-04-27T10:01:30Z

for #1673 - tested locally on a mi band 5 and can confirm there's no regression

however #4152 returns, a way to test this locally is to apply a delay to https://github.com/vector-im/element-android/blob/develop/vector/src/gplay/java/im/vector/app/gplay/push/fcm/VectorFirebaseMessagingService.kt#L174

coroutineScope.launch {
    delay(5000)
    session.requireBackgroundSync()
}

to avoid double notifications we'll need to further refine the notifications to dynamically enable alertOnce if the notification is new or an update

Claire1817 · 2022-05-02T09:52:19Z

@ouchadam I had tested the #4152 by adding the "delay" and i didn't reproduce the bug.

mnaturel

After some tests with an emulator Pixel 3a Android 12, I frequently have 2 notification sounds per message. I don't know how notifications work on the project but it may be worth to check/add logs when notify() is called on the NotificationManager for a single message.

Claire1817 · 2022-05-04T08:01:21Z

@mnaturel I didn't reproduce your bug with an emulator.
I had tested with long and short messages, good and bad network.
Can you test it on device ?

mnaturel · 2022-05-04T08:29:40Z

@mnaturel I didn't reproduce your bug with an emulator. I had tested with long and short messages, good and bad network. Can you test it on device ?

After some tests on real device (Nokia 7.2, Android 11) I could not reproduce the double notification bug. I suspect it may happen sometimes on very high connectivity.

ouchadam · 2022-05-04T09:15:16Z

To clarify the double the notification, the notification UI is correct but two pings will be triggered for each message that goes through the fastlane flow (push from server -> fetch message directly -> notify message -> request sync -> sync finds new messages and notifies again) https://github.com/vector-im/element-android/blob/develop/vector/src/gplay/java/im/vector/app/gplay/push/fcm/VectorFirebaseMessagingService.kt#L174

The double ping will only happen if the sync triggered by the push returns after the getEvent/fastlane , otherwise the event is ignored. The fastlane flow is only entered when on wifi and we're not currently viewing the room the message is part of https://github.com/vector-im/element-android/blob/develop/vector/src/gplay/java/im/vector/app/gplay/push/fcm/VectorFirebaseMessagingService.kt#L187

Claire1817 · 2022-05-04T09:53:04Z

Thanks @ouchadam for your explanation, it's more clear now.
Should we do a verification to activate or not the setOnlyAlertOnce ? to avoid the double ping.
Should we check if we get a notification coming from the sync ? In which case do we have to display notification coming from the sync ?
Should we only display the notifications coming from getEvent/fastlane ?
Is it possible to know if a notification is already shown ?
I don't know what is the best solution :/.

ouchadam · 2022-05-04T12:20:15Z

ultimately my gut feeling is that we'll need to dynamicly apply the setOnlyAlertOnce based on new content or updates/edits, I'm thinking we could check if the latest room message event is an update, and if so use setOnlyAlertOnce(true) to avoid the double ping

there's a few changes needed to enable this

We need to confirm that notifying twice with setOnlyAlertOnce(false) setOnlyAlertOnce(true) only pings once
Add state to know if an existing event has been replaced https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/notifications/NotificationEventQueue.kt#L115
When creating the room notifications https://github.com/vector-im/element-android/blob/develop/vector/src/main/java/im/vector/app/features/notifications/RoomGroupMessageCreator.kt#L69 make use of the new state, could be something like setOnlyAlertOnce(events.last().isUpdate)

ouchadam · 2022-05-05T09:04:05Z

thanks for the update, can confirm it's working for me locally 💯

ouchadam · 2022-05-05T09:05:03Z

changelog.d/46312.misc

@@ -0,0 +1 @@
+Use the same notification rules than web / iOs client


is it worth calling out that the change will mean we will notify for all (new) messages?

yes we can, i update the message

update notifications rules: make a sound for each notification

00f232d

Claire1817 marked this pull request as draft April 20, 2022 12:34

giomfo mentioned this pull request Apr 27, 2022

[Notifications] Play sound on each notification when the notification mode is "All messages" tchapgouv/tchap-android#509

Closed

ouchadam self-requested a review April 27, 2022 09:29

Claire1817 marked this pull request as ready for review May 2, 2022 09:52

Claire1817 requested review from a team and mnaturel and removed request for a team May 2, 2022 09:52

mnaturel reviewed May 2, 2022

View reviewed changes

mnaturel self-requested a review May 4, 2022 08:30

mnaturel approved these changes May 4, 2022

View reviewed changes

add isUpdated in NotifiableEvent and RoomEventGroupInfo

132e44f

ouchadam reviewed May 5, 2022

View reviewed changes

ouchadam approved these changes May 5, 2022

View reviewed changes

Claire1817 added 3 commits May 5, 2022 11:12

use val instead of var

96921b6

update changelog file

051bee6

Fix TUs

f188eb6

Claire1817 merged commit c9bd1f3 into develop May 5, 2022

Claire1817 deleted the cgizard/ISSUE-4632 branch May 5, 2022 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update notifications rules: make a sound for each notification #5796

Update notifications rules: make a sound for each notification #5796

Claire1817 commented Apr 20, 2022

github-actions bot commented Apr 20, 2022 •

edited

Loading

ouchadam commented Apr 20, 2022 •

edited

Loading

ouchadam commented Apr 27, 2022

Claire1817 commented May 2, 2022

mnaturel left a comment

Claire1817 commented May 4, 2022

mnaturel commented May 4, 2022

ouchadam commented May 4, 2022 •

edited

Loading

Claire1817 commented May 4, 2022 •

edited

Loading

ouchadam commented May 4, 2022 •

edited

Loading

ouchadam commented May 5, 2022

ouchadam May 5, 2022

Claire1817 May 5, 2022

		@@ -0,0 +1 @@
		Use the same notification rules than web / iOs client

Update notifications rules: make a sound for each notification #5796

Update notifications rules: make a sound for each notification #5796

Conversation

Claire1817 commented Apr 20, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Apr 20, 2022 • edited Loading

Unit Test Results

ouchadam commented Apr 20, 2022 • edited Loading

ouchadam commented Apr 27, 2022

Claire1817 commented May 2, 2022

mnaturel left a comment

Choose a reason for hiding this comment

Claire1817 commented May 4, 2022

mnaturel commented May 4, 2022

ouchadam commented May 4, 2022 • edited Loading

Claire1817 commented May 4, 2022 • edited Loading

ouchadam commented May 4, 2022 • edited Loading

ouchadam commented May 5, 2022

ouchadam May 5, 2022

Choose a reason for hiding this comment

Claire1817 May 5, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 20, 2022 •

edited

Loading

ouchadam commented Apr 20, 2022 •

edited

Loading

ouchadam commented May 4, 2022 •

edited

Loading

Claire1817 commented May 4, 2022 •

edited

Loading

ouchadam commented May 4, 2022 •

edited

Loading