Single `PushTriggerListener` dispatch #4401

ouchadam · 2021-11-03T15:22:11Z

Some preparation/refactors around the Push triggers in order to support suspending NotifiableEventResolver.resolveEvent which is required for including images in the notifications

*points to a feature branch

PushRuleListener functions are replaced with a single onEvents(PushEvents) in order to allow any logic within the implementation to be asynchronous
All notification event changes are handled via the notificationDrawerManager.updateEvents, this ensures a single point of synchronisation and notification render pass (reduces the amount of render passes and synchronisation locks)
Notification queue mutations are extracted to a dedicated class along with unit tests
Lifts the NotifiableEvent fixtures to a dedicated fixtures test package

…llback - simplifies the handling of notifications, will allow us to reduce redundant synchronisations and suspend the entire notification update (will be needed for supporting images)

…a single update point of entry for mutating the events - this avoids multiple synchronisation locks by batching updates and ensures a single notification render pass

… the chain

…tifiableEventReceived not synchronised for use within the synchronized batching - makes the refresh function private as all interactions now come through via update

…viour

ouchadam · 2021-11-03T15:25:26Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/pushrules/PushRuleService.kt

-        fun onRoomLeft(roomId: String)
-        fun onEventRedacted(redactedEventId: String)
-        fun batchFinish()
+        fun onEvents(pushEvents: PushEvents)


using a single function here also allows up to simplfy the notification updates as we can batch all the events into one update pass, reducing the amount of synchronisation locks and redundant render passes

we might not need the private var firstThrottler = FirstThrottler(200) anymore because of how few individual updates we make!

OK, You should add a file for the changelog with .removal extension to notify SDK users about this API break.

added 88fbb8f

ouchadam · 2021-11-03T15:26:07Z

...rc/main/java/org/matrix/android/sdk/internal/session/notification/ProcessEventForPushTask.kt

@@ -39,14 +40,6 @@ internal class DefaultProcessEventForPushTask @Inject constructor(
 ) : ProcessEventForPushTask {

    override suspend fun execute(params: ProcessEventForPushTask.Params) {
-        // Handle left rooms
-        params.syncResponse.leave.keys.forEach {


these have moved to

roomsJoined = params.syncResponse.join.keys, roomsLeft = params.syncResponse.leave.keys,

https://github.com/vector-im/element-android/pull/4401/files#diff-930446ed1e91a2c4f8a100edef89d21db2bec6b6d4a80765ff58f73e4e042182R88

ouchadam · 2021-11-03T15:27:03Z

vector/src/gplay/java/im/vector/app/gplay/push/fcm/VectorFirebaseMessagingService.kt

@@ -201,8 +201,7 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() {
            resolvedEvent
                    ?.also { Timber.tag(loggerTag.value).d("Fast lane: notify drawer") }
                    ?.let {
-                        notificationDrawerManager.onNotifiableEventReceived(it)
-                        notificationDrawerManager.refreshNotificationDrawer()
+                        notificationDrawerManager.updateEvents { it.onNotifiableEventReceived(resolvedEvent) }


updateEvents synchronises all queue mutations within the block and refreshes the drawer at the end

ouchadam · 2021-11-03T15:29:02Z

vector/src/main/java/im/vector/app/features/notifications/PushRuleTriggerListener.kt

+    override fun onEvents(pushEvents: PushEvents) {
+        session?.let { session ->
+            val notifiableEvents = createNotifiableEvents(pushEvents, session)
+            notificationDrawerManager.updateEvents { queuedEvents ->


all notification updates from the sync response are now batched together into a single update, this enables the logic within onEvents to be moved to a coroutine without race conditions

ouchadam · 2021-11-03T15:30:27Z

vector/src/test/java/im/vector/app/features/notifications/NotifiableEventProcessorTest.kt

@@ -145,48 +148,3 @@ class NotifiableEventProcessorTest {
        ProcessedEvent(it.first, it.second)
    }
 }
-
-fun aSimpleNotifiableEvent(eventId: String, type: String? = null) = SimpleNotifiableEvent(


extracted out to their own file

github-actions · 2021-11-03T15:54:10Z

Unit Test Results

  66 files +  4   66 suites +4 50s ⏱️ +2s
135 tests +17 135 ✔️ +17 0 💤 ±0 0 ❌ ±0
418 runs +68 418 ✔️ +68 0 💤 ±0 0 ❌ ±0

Results for commit 88fbb8f. ± Comparison against base commit 3760401.

♻️ This comment has been updated with latest results.

bmarty

LGTM, just one remark about the changelog

bmarty · 2021-11-03T17:31:43Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/pushrules/PushRuleService.kt

-        fun onRoomLeft(roomId: String)
-        fun onEventRedacted(redactedEventId: String)
-        fun batchFinish()
+        fun onEvents(pushEvents: PushEvents)


OK, You should add a file for the changelog with .removal extension to notify SDK users about this API break.

bmarty · 2021-11-03T17:32:39Z

...src/main/java/org/matrix/android/sdk/internal/session/notification/DefaultPushRuleService.kt

+                }
+            }
+        }
+    }


This code looks way better for this class at least :)

bmarty · 2021-11-05T15:01:42Z

@ouchadam please feel free to merge your approved PRs, if not targeting develop :)

ouchadam · 2021-11-05T15:03:30Z

@bmarty thanks, will do!

ouchadam added 10 commits November 3, 2021 11:12

replacing separated push listener callbacks with a single onEvents ca…

5096318

…llback - simplifies the handling of notifications, will allow us to reduce redundant synchronisations and suspend the entire notification update (will be needed for supporting images)

extracting the notification event logic to its own class and provide …

57f7d83

…a single update point of entry for mutating the events - this avoids multiple synchronisation locks by batching updates and ensures a single notification render pass

flattening some of the onNotifiableEventReceived branches to simplify…

6e0bd03

… the chain

moving the notifable queue adding to the queue itself and making onNo…

4014638

…tifiableEventReceived not synchronised for use within the synchronized batching - makes the refresh function private as all interactions now come through via update

extracting notifiable event creation to its own function

b76f881

cleaning up method ordering and visibility

7b38a06

extracting notifiable event fixtures to their own file

cd9175b

adding test cases around the mutating of the notification event queue

576d945

adding catch around the push event dispatching to match previous beha…

5f13ba0

…viour

updating copyright header

805f09f

ouchadam commented Nov 3, 2021

View reviewed changes

ouchadam mentioned this pull request Nov 3, 2021

Supporting images in notifications #4402

Merged

bmarty approved these changes Nov 3, 2021

View reviewed changes

add changelog entry for breaking API change

88fbb8f

ouchadam merged commit 183ca18 into feature/adm/feature-notification-images Nov 5, 2021

ouchadam deleted the feature/adm/single-transaction-push branch November 5, 2021 15:03

ouchadam mentioned this pull request Nov 8, 2021

Notification images #4430

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single `PushTriggerListener` dispatch #4401

Single `PushTriggerListener` dispatch #4401

ouchadam commented Nov 3, 2021 •

edited

Loading

ouchadam Nov 3, 2021

bmarty Nov 3, 2021

ouchadam Nov 3, 2021

ouchadam Nov 3, 2021

ouchadam Nov 3, 2021

ouchadam Nov 3, 2021 •

edited

Loading

ouchadam Nov 3, 2021

github-actions bot commented Nov 3, 2021 •

edited

Loading

bmarty left a comment

bmarty Nov 3, 2021

bmarty Nov 3, 2021

bmarty commented Nov 5, 2021

ouchadam commented Nov 5, 2021

Single PushTriggerListener dispatch #4401

Single PushTriggerListener dispatch #4401

Conversation

ouchadam commented Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2021 • edited Loading

Unit Test Results

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Nov 5, 2021

ouchadam commented Nov 5, 2021

Single `PushTriggerListener` dispatch #4401

Single `PushTriggerListener` dispatch #4401

ouchadam commented Nov 3, 2021 •

edited

Loading

ouchadam Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading