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

[firebase_messaging] Enable compatibility with other notification plugins #1829

Merged
merged 13 commits into from
Mar 18, 2020
Merged

Conversation

MaikuB
Copy link
Contributor

@MaikuB MaikuB commented Jan 17, 2020

Description

Compatibility issues have been reported with firebase_messaging and other notification plugins (e.g. flutter_local_notification). When both are used in an application, their callbacks may be clobbered.

Users have also recently reported that push notifications were not working since recent changes had mentioned in the readme that the AppDelegate must be modified to register the UNUserNotificationCenterDelegate. However, this causes problems as FCM uses method swizzling. The readme has been updated as part of this change to indicate that registering the UNUserNotificationCenterDelegate should only be done when method swizzling is disabled, which should fix this issue as well.

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@MaikuB
Copy link
Contributor Author

MaikuB commented Jan 18, 2020

Just to add a note that I didn't update the example app use the flutter_local_notifications plugin like @bparrishMines did in https://github.com/FirebaseExtended/flutterfire/pull/1785/files. I imagine that was only done for testing purposes.

…unch is only triggered for a notification coming from FCM
@pro100svitlo
Copy link

Any ETA when this one will be reviewed and merged?

@MaikuB
Copy link
Contributor Author

MaikuB commented Jan 25, 2020

If it's urgent, you could try referencing my fork in your app. See https://dart.dev/tools/pub/dependencies#git-packages in case you haven't referenced packages via git before. Would help knowing if it helps resolve issues for others as well beyond the testing I've done

@pro100svitlo
Copy link

pro100svitlo commented Jan 29, 2020

@MaikuB thanks!

But I still not able to receive message and show local push notification when ios app is off, even with changes from your repo.

I update my pubspec with repo ref:

 firebase_messaging: 
    git:
      url: git@github.com:MaikuB/flutterfire.git
      path: packages/firebase_messaging/

also I add line to infoPlist:

	<key>FirebaseAppDelegateProxyEnabled</key>
	<false/>

To send msg I am using next cmd:

curl --location --request POST 'https://fcm.googleapis.com/fcm/send' \
--header 'Authorization: key=myApiKey' \
--header 'Content-Type: application/json' \
--data-raw '{
  "to": "/topics/myTopic",
  "data": {
  	"id":63
  }
}
'

So,

  • android works as expected 👌
  • when ios app is running - I am able to receive msg and show local notification.
  • when ios app is off - nothing happen. But as soon as app started - it behaves like app just received a push notification.

@MaikuB
Copy link
Contributor Author

MaikuB commented Jan 29, 2020

@pro100svitlo can you post issues on my fork so we can make the PR thread for others easier to follow?

@pro100svitlo
Copy link

@pro100svitlo can you post issues on my fork so we can make the PR thread for others easier to follow?

done 👌

@m-oehme
Copy link

m-oehme commented Feb 11, 2020

Thank you for the fix.
I got it to work.

On both iOS and Android it works for me now. I had to make a nother change though.
When receiving Notification while App is in foreground and sliding down the Notification Center the Notification in the Center gets removed.
Appearently this is intentional behaviour by firebase plugin. In the applicationDidBecomeActive the icon badges are set to 0 which triggers the removal.

- (void)applicationDidBecomeActive:(UIApplication *)application {
  _resumingFromBackground = NO;
...
//  application.applicationIconBadgeNumber = 1;
//  application.applicationIconBadgeNumber = 0;
}

I have tested the various app states on both iOS and Android and together with your fork the change doesn't seem to cause any issue.
So it solved it for me.

Maybe you can include it into this PR.

@Levi-Lesches
Copy link
Contributor

I thought Androids were supposed to dismiss notifications of apps on-screen. As logn as the callback is run, the user shouldn't have to have another notification in their tray.

@MaikuB
Copy link
Contributor Author

MaikuB commented Feb 11, 2020

@m-oehme there's an issue raised on this already #114. See thread for more info but not likely to be in this PR unless there's a response from the firebase team since they consider it to be working as intended for push notifications.

@m-oehme
Copy link

m-oehme commented Feb 12, 2020

@Levi-Lesches not sure about Android. This was about iOS and there you normally have to remove them manually as a developer.

@MaikuB thank you, didn't see that one.

@JoseGeorges8
Copy link

I'm not a top developer and surely not part of a big team such as Flutter... but is there a reason why @collinjackson or @kroikie haven't looked at this PR yet? it's been 27 days now and there's many of us waiting to include this plugin in our projects. imo it doesn't take more than a couple hours max to just take a look at the PR.

Not looking to sound rude here, I'm just curious of how busy can they be!

Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for the contribution! I made some small changes to the CHANGELOG. I removed the breaking changes notes since the is essentially a bug fix. I'll merge once analyze tests pass once again!

@bparrishMines bparrishMines merged commit 3fc3ca8 into firebase:master Mar 18, 2020
@MaikuB
Copy link
Contributor Author

MaikuB commented Mar 18, 2020

Sure thing. I was told on discord that infrastructure changes were going to be done before this could be looked though I don't know if that was with regards to do with priorities/timr or if those changes would help testing PRs like this one

@bparrishMines
Copy link
Contributor

Yea, @collinjackson and I talked offline and there is new testing infrastructure that would allow us to test this on CI, but this issue is blocking many people so I wanted to go ahead and submit it. We can currently test this locally, but we still need to set things up to test it on CI.

@firebase firebase locked and limited conversation to collaborators Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants