-
Notifications
You must be signed in to change notification settings - Fork 263
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
Fix badge incrementing when cancelling display in foreground #998
Conversation
bf9b1c7
to
c01359a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @emawby, @nan-li, and @rgomezp)
iOS_SDK/OneSignalSDK/Source/OSNotification.m, line 312 at r1 (raw file):
*/ if (!notification) { NSInteger previousBadgeCount = [OneSignalUserDefaults.initShared getSavedIntegerForKey:ONESIGNAL_PREVIOUS_BADGE_KEY defaultValue:0];
Instead of using us having to track a previous value we could instead read the iOS value applicationIconBadgeNumber
again here. This means one less state we have to keep track of and means we also don't need a migration to correctly set the previous badge value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jkasten2, @nan-li, and @rgomezp)
iOS_SDK/OneSignalSDK/Source/OSNotification.m, line 312 at r1 (raw file):
Previously, jkasten2 (Josh Kasten) wrote…
Instead of using us having to track a previous value we could instead read the iOS value
applicationIconBadgeNumber
again here. This means one less state we have to keep track of and means we also don't need a migration to correctly set the previous badge value.
Ya that will work and I will make the change. The problem with this is that we then use a UIApplication sharedApplication
method in OSNotification
which is used in code required by our extension handlers. This means we won't be able to get rid of all sharedApplication references in an NSE only framework.
However when that transition does happen we could move this logic to an extension that only exists in the full framework.
c01359a
to
5cfe3e2
Compare
5cfe3e2
to
24e7af0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @nan-li and @rgomezp)
iOS_SDK/OneSignalSDK/Source/OSNotification.m, line 312 at r1 (raw file):
Previously, emawby (Elliot Mawby) wrote…
Ya that will work and I will make the change. The problem with this is that we then use a
UIApplication sharedApplication
method inOSNotification
which is used in code required by our extension handlers. This means we won't be able to get rid of all sharedApplication references in an NSE only framework.However when that transition does happen we could move this logic to an extension that only exists in the full framework.
We can move this logic outside of OSNotification
in the future to account for the NSE decoupling. This doesn't seems something the OSNotification class should be responsible for anyway.
Fixes OneSignal/react-native-onesignal#1256
When choosing to not display a notification with our foreground handler iOS does not increment the badge count or send the notification to the notification center, but we cache the increased value in OneSignal's user defaults. This cause a subsequent notification to include the cancelled notifications badge increment value.
In order to prevent this we need to reset the cached badge count if the notification is not displayed. In order to fix this for non-incremental badge changes we need to also store the previous badge count value to NSUserDefaults.
This change is