Skip to content
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

Change Android push intercepting technique #192

Conversation

mabdurrahman
Copy link
Contributor

Issue

I'm using this plugin from early days on a Flutter 1 project along with the old FlutterFire - Cloud Messaging, however, as you might know already that the old FlutterFire - Cloud Messaging wasn't working on background properly (i.e. FlutterFire#4269, FlutterFire#5697, FlutterFire#5387, and more...) and I wasn't able to get the Intercom Plugin to handle the FCM messages on the background as being mentioned on the README.md,

Solution

As a result of the mentioned issue, I had to fork this plugin and use a different technique to intercept FCM messages in a way that doesn't conflict with FlutterFire implementation at all, and it's working perfectly since then (almost over a year in production).

The solution is basically to replace the io.maido.intercom.PushInterceptService with io.maido.intercom.PushInterceptReceiver, because only one push notification service can work at a time (either FlutterFire's one, or this plugin's one) on contrary to the broadcast receivers which can co-exist next to each other without any conflicts.

Copy link
Collaborator

@deepak786 deepak786 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting...

But need to test this first.

@mabdurrahman mabdurrahman force-pushed the change-android-push-intercepting-technique branch from 1dbf6af to 0159b20 Compare December 21, 2021 08:46
@mabdurrahman
Copy link
Contributor Author

@deepak786 feel free to test it and merge whenever you feel comfortable with the change, I myself already using this change for over a year in production now and never experienced an issue

@deepak786
Copy link
Collaborator

@mabdurrahman Thank you for this PR.
But as I said It needs to be tested first. So I will test this and will proceed accordingly.

@deepak786
Copy link
Collaborator

@mabdurrahman I tested this change on Android.

  • App is in the background.
  • Sent the message from the Intercom dashboard to the user.
  • Log printed "Intercom message received" which means the receiver worked.
  • but App freezes and the emulator is disconnected.
V/FA      (10368): Inactivity, disconnecting from the service
D/PushInterceptReceiver(10368): Intercom message received
D/CompatibilityChangeReporter(10368): Compat change id reported: 160794467; UID 10159; state: ENABLED
I/endaboa.app.de(10368): Thread[5,tid=10374,WaitingInMainSignalCatcherLoop,Thread*=0x7d353ec319a0,peer=0x12e401f8,"Signal Catcher"]: reacting to signal 3
I/endaboa.app.de(10368): 
I/endaboa.app.de(10368): Wrote stack traces to tombstoned
Lost connection to device.

could be this issue #151.
Are you facing this issue? Could you please test?

The new push intercept receiver is meant to work on parallel to Firebase Messaging service
@mabdurrahman mabdurrahman force-pushed the change-android-push-intercepting-technique branch from 0159b20 to 4d68d75 Compare December 24, 2021 19:47
@mabdurrahman
Copy link
Contributor Author

@deepak786 Sorry for the delay, I was actually migrating my app to flutter 2 and wasn't able to test earlier, after testing I confirm I can get the exact same behavior as yours, however, I believe you're correct about relating that issue to #150 and I'm confident that it's not really related to the change proposed by this PR.

Moreover, after I migrated my app to flutter 2 and the new Firebase Messaging I discovered that Firebase Messaging itself is doing the exact same thing proposed by this PR (more details here), and it even takes it a step further by declaring the broadcast receiver though the plugin's AndroidManifiest.xml so that it's merged automatically to the main app AndroidManified.xml which makes the plugin setup easier, thus, I updated the PR to do the same and updated the README.md accordingly.

@deepak786
Copy link
Collaborator

@mabdurrahman
I know that if the plugin itself declares the receiver then it will be easy to use this plugin.
But due to the #151 issue, we will get a lot of complaints.

Currently, the users can display a manual notification or can ignore the background notification.
But with handling the background notification itself by the plugin, users will not be able to do that.

I see no issue with your PR.
But first I need to solve issue #151. So I will get in touch with the Intercom support team again about the same.
It would be a great help if you also report the same issue to the Intercom support team. This will prioritize the issue.

intercom_flutter/README.md Show resolved Hide resolved
intercom_flutter/README.md Show resolved Hide resolved
@deepak786 deepak786 added the platform-android Specific to Android platform label Dec 25, 2021
@mabdurrahman
Copy link
Contributor Author

@deepak786 Hey, sorry for the delays again, I've raised the issue this morning with Intercom support, however, I think it might delay anyway because of the holidays season, let's hope for the best.

As for the changes in the README.md, IMHO we can keep the combined example as it's, it's not really listed under Android section, so it might be better to give a non-misleading example rather than confusing the developer with 2 examples split apart into 2 different sections. I also liked the idea of adding the example into the example app, however, it needs integrating Firebase Messaging first and I'm afraid it will complicate the example a little bit.

Anyway, if you still insist on having the change on the README.md revered then I don't have issues and will try to find a suitable time to have the example app modified as per your suggestion.

Copy link
Collaborator

@deepak786 deepak786 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some changes that need to be done in the readME. But I think it can be done separately.

@deepak786 deepak786 merged commit 81ac8a2 into v3rm0n:master Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-android Specific to Android platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants