Use firebaseReceiver with FCM #7068

p1gp1g · 2022-09-07T16:02:51Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Use Firebase-Messaging instead of embedded_fcm_distributor hen using FCM.

Motivation and context

There are issues with notifications with FCM

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()

ATM, it uses the default fallback if cancelled

The forced unregistration always happens in register function

Signed-off-by: sim <git@sgougeon.fr>

p1gp1g · 2022-09-07T17:24:43Z

Should we disable Instantiatable Lint check for vector-app module to fix the workflows ?

diff --git a/vector-app/build.gradle b/vector-app/build.gradle
index a8262fde40..c89fba3440 100644
--- a/vector-app/build.gradle
+++ b/vector-app/build.gradle
@@ -317,7 +317,7 @@ android {

     lintOptions {
         lintConfig file("../tools/lint/lint.xml")
-
+        disable "Instantiatable"
         checkDependencies true
         abortOnError true
     }

DoM1niC · 2022-09-07T20:30:47Z

THX for your work :) I will test your changes like your Camera PR, what works amazing here !

Use VectorMessagingHelper to directly call onMessage

bmarty

I do not really see why avoiding using android-embedded_fcm_distributor will help to fix the issue with Push arriving to the app but not handled. Can you explain please?
Thanks

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt

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

p1gp1g · 2022-09-08T09:03:15Z

Now FCM totally bypass UnifiedPush. There were some error with the gateway too. Also, it now always ensure FCM is retrieved (done with a merge of #6936 to add simplicity)

p1gp1g · 2022-09-08T09:52:27Z

I do not really see why avoiding using android-embedded_fcm_distributor will help to fix the issue with Push arriving to the app but not handled. Can you explain please? Thanks

Now, as soon as FirebaseReceiver.onMessageReceived is called, vectorMessagingHelper.onMessage will handle that message like it did before unifiedpush was merged

bmarty · 2022-09-08T13:18:06Z

I am testing the code and it's working fine so far. I will wait to see if I got some issue after a while.

I have found a bug, this is maybe not new, but if I select NTFY several times, I can see several token on the NTFY app. I think some unregistration is missing.

p1gp1g · 2022-09-08T13:28:14Z

I am testing the code and it's working fine so far. I will wait to see if I got some issue after a while.

Good news, I hope no issue will pop :)

I have found a bug, this is maybe not new, but if I select NTFY several times, I can see several token on the NTFY app. I think some unregistration is missing.

I'll check that, I think it is better to handle this in another PR later

bmarty

Some remarks. The code readability could be improved by doing some renaming.
Still testing it on my side. Thanks!

bmarty · 2022-09-08T13:19:29Z

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

+import javax.inject.Inject
+
+@AndroidEntryPoint
+class FirebaseReceiver : FirebaseMessagingService() {


Maybe rename to VectorFirebaseMessagingService.

bmarty · 2022-09-08T13:21:45Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingHelper.kt

+private val loggerTag = LoggerTag("Push", LoggerTag.SYNC)
+
+/**
+ * Hilt injection happen at super.onReceive().


This comment has to be updated.

bmarty · 2022-09-08T13:22:38Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingHelper.kt

+ * Hilt injection happen at super.onReceive().
+ */
+
+class VectorMessagingHelper @Inject constructor(


This class will take care of Push received by one of our Push receivers. Maybe rename to VectorPushHandler?

bmarty · 2022-09-08T13:23:44Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingHelper.kt

+     *
+     * @param message the message
+     */
+    fun onMessage(message: String) {


This could be defined in an interface with this single method.

I am not sure how to handle the injections with the interface.

bmarty · 2022-09-08T13:27:40Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingHelper.kt

+        }
+
+        val pushData = pushParser.parseData(message, unifiedPushHelper.isEmbeddedDistributor())
+                ?: return Unit.also { Timber.tag(loggerTag.value).w("Invalid received data Json format") }


I would move the parsing of the data to the Push receiver, since the model is different depending on the Push solution. So the the parsing can take place in FirebaseReceiver and VectorMessagingReceiver. So onMessage argument can be of type PushData, and the fun onMessage could be rename to handle

bmarty · 2022-09-08T13:30:10Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt

@@ -57,27 +39,16 @@ private val loggerTag = LoggerTag("Push", LoggerTag.SYNC)
 */
 @AndroidEntryPoint
 class VectorMessagingReceiver : MessagingReceiver() {


Maybe for code clarity rename to VectorUnifiedPushMessagingReceiver

bmarty

Thanks for the update.
My test is still running, I have in parallel the nightly build and the build from this PR, using the same big account, and for now I get Push working on both app.
For the DI of VectorPushHandler I can handle it later.

vector/src/main/java/im/vector/app/core/pushers/VectorUnifiedPushMessagingReceiver.kt

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

bmarty

Thanks for the update with Logs.

After a night, I can see that the nightly build are not receiving push (or they have no effect), and the version from this PR is still receiving it, notification are showing up.

I am going to merge the PR for the coming release.

bmarty · 2022-09-09T07:44:51Z

(I will merge on a branch I own to do some cleanup)

bahur142 · 2022-09-10T18:07:57Z

I have found a bug, this is maybe not new, but if I select NTFY several times, I can see several token on the NTFY app. I think some unregistration is missing.

I'll check that, I think it is better to handle this in another PR later

Just made a symbolic donation on liberapay for your work on unified push :)

We do not use `android-embedded_fcm_distributor` anymore (since #7068). The code was compiling this `android-embedded_fcm_distributor` has a dependency on `firebase-messaging`.

We do not use `android-embedded_fcm_distributor` anymore (since #7068). The code was compiling because `android-embedded_fcm_distributor` has a dependency on `firebase-messaging`.

ouchadam and others added 9 commits August 25, 2022 09:05

trigger CI

cda50dc

Set distributor dialog always cancelable

02d5487

ATM, it uses the default fallback if cancelled

Always use register to open the distributor dialog

a4dd08d

The forced unregistration always happens in register function

Add comments

42c580c

Remove never-matched if

e2646c3

Add changelog

f7ae377

Signed-off-by: sim <git@sgougeon.fr>

Use firebaseReceiver with FCM

9cf6dd7

Add changelog detail

5b46c80

Signed-off-by: sim <git@sgougeon.fr>

Lint

00a04b8

Do not intent to VectorMessagingReceiver from FirebaseReceiver

0b9b825

Use VectorMessagingHelper to directly call onMessage

bmarty reviewed Sep 8, 2022

View reviewed changes

p1gp1g added 3 commits September 8, 2022 10:49

Fix gateway

6ba963b

Merge branch 'misc/refactor-uphelper' into fix/fcm-push2

7bdcacf

Ensure FCM token is always retrieved

94e2a0a

p1gp1g added 3 commits September 8, 2022 11:15

Lint

3267cbb

Ensure FCM always have the good gateway

4849802

Update pusher with FCM new token only if using FCM

f65074b

Lint

65654e7

bmarty reviewed Sep 8, 2022

View reviewed changes

p1gp1g added 4 commits September 8, 2022 15:37

Rename FirebaseReceiver to VectorFirebaseMessagingService

2527dbb

Rename VectorMessagingHelper to VectorPushHandler

dbea172

Rename VectorMessagingReceiver to VectorUnifiedPushMessagingReceiver

bc5309b

Parse pushData in the push receiver

3e12907

bmarty reviewed Sep 8, 2022

View reviewed changes

vector/src/main/java/im/vector/app/core/pushers/VectorUnifiedPushMessagingReceiver.kt Show resolved Hide resolved

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

Add logs for invalid push data

a17a4fe

bmarty approved these changes Sep 9, 2022

View reviewed changes

bmarty changed the base branch from develop to feature/bma/fix_push September 9, 2022 07:45

bmarty merged commit f88039b into element-hq:feature/bma/fix_push Sep 9, 2022

bmarty mentioned this pull request Sep 9, 2022

Feature/bma/fix push #7085

Merged

bmarty mentioned this pull request Sep 12, 2022

Push notification not working (Google Play variant) #6896

Closed

p1gp1g mentioned this pull request Sep 12, 2022

Small refactor UnifiedPushHelper #6936

Closed

15 tasks

bmarty mentioned this pull request Sep 19, 2022

Fix regression on our dependency, due to merge of #6788. #7169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use firebaseReceiver with FCM #7068

Use firebaseReceiver with FCM #7068

p1gp1g commented Sep 7, 2022 •

edited

Loading

p1gp1g commented Sep 7, 2022

DoM1niC commented Sep 7, 2022

bmarty left a comment

p1gp1g commented Sep 8, 2022

p1gp1g commented Sep 8, 2022

bmarty commented Sep 8, 2022

p1gp1g commented Sep 8, 2022

bmarty left a comment •

edited

Loading

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

p1gp1g Sep 8, 2022

bmarty Sep 8, 2022

bmarty Sep 8, 2022

bmarty left a comment

bmarty left a comment

bmarty commented Sep 9, 2022

bahur142 commented Sep 10, 2022

Use firebaseReceiver with FCM #7068

Use firebaseReceiver with FCM #7068

Conversation

p1gp1g commented Sep 7, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

p1gp1g commented Sep 7, 2022

DoM1niC commented Sep 7, 2022

bmarty left a comment

Choose a reason for hiding this comment

p1gp1g commented Sep 8, 2022

p1gp1g commented Sep 8, 2022

bmarty commented Sep 8, 2022

p1gp1g commented Sep 8, 2022

bmarty left a comment • edited Loading

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

p1gp1g Sep 8, 2022

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

bmarty Sep 8, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

bmarty commented Sep 9, 2022

bahur142 commented Sep 10, 2022

p1gp1g commented Sep 7, 2022 •

edited

Loading

bmarty left a comment •

edited

Loading