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

Avoid taking control of UNUserNotificationCenter (Closes #206, closes #256) #268

Merged

Conversation

danielgindi
Copy link
Contributor

@danielgindi danielgindi commented Aug 20, 2018

There's a problem with the iOS >= 10 condition to set the delegate of the UNUserNotificationCenter.
What it does is take control of the UNUserNotificationCenter, and the user has no chance to change it back.
The effect is that every push notification also shows when the app is in foreground, which is undesired in most case.

This should be left to the user to define a delegate if he desires, and send the notifications to the foreground (and the logic in the delegate is defaulted anyway.)

Edit:
This also fixes Firebase notifications, as Firebase also registers for the delegate and take control from there.

@danielgindi danielgindi changed the title Avoid taking control of UNUserNotificationCenter Avoid taking control of UNUserNotificationCenter (Closes #206) Aug 20, 2018
@danielgindi danielgindi changed the title Avoid taking control of UNUserNotificationCenter (Closes #206) Avoid taking control of UNUserNotificationCenter (Closes #206, #256) Aug 20, 2018
@danielgindi danielgindi changed the title Avoid taking control of UNUserNotificationCenter (Closes #206, #256) Avoid taking control of UNUserNotificationCenter (Closes #206, closes #256) Aug 20, 2018
@mauron85
Copy link
Owner

mauron85 commented Aug 20, 2018

I see the motivation and agree those referenced issues should be resolved. But removing notification management it's not best way to do, because it will stop displaying debug notifications when app is in foreground. You can argue, "hey who is needing those", but my reply would be, we need a fix that target both issues.

@danielgindi
Copy link
Contributor Author

Maybe we can enable those in debug mode only

@danielgindi danielgindi force-pushed the bugfix/ios_notification_delegate branch from d9370e9 to 6df9a81 Compare August 20, 2018 10:46
@danielgindi
Copy link
Contributor Author

Okay so I've changed it to be enabled in debug mode only - and also then - give FCM a chance. It keeps the previous delegate pointer.
If another module (like FCM) was loaded after this one, then it's out of our hands anyway and the delegate was overriden.

@mauron85 mauron85 merged commit 7685972 into mauron85:master Aug 20, 2018
@mauron85
Copy link
Owner

Merged. Thank you!

mauron85 added a commit to mauron85/cordova-plugin-background-geolocation that referenced this pull request Aug 20, 2018
@danielgindi danielgindi deleted the bugfix/ios_notification_delegate branch August 21, 2018 06:56
@rudiminty
Copy link

@mauron85, has this been made available in an official version on npmjs for cordova-plugin-mauron85-background-geolocation?

@mauron85
Copy link
Owner

mauron85 commented Sep 16, 2018 via email

@rudiminty
Copy link

The reason why I ask is that I used 3.0.0 alpha 42 but still find that on iOS push notifications in the foreground are not handled correctly. Could this issue be responsible for such behaviour?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants