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

Aligned iOS local fg notification behavior with Android (localNotificationReceived only triggers when tapped, not when shown) #3348

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

javieranton-zz
Copy link
Contributor

As of today, after @shannah latest changes that enabled iOS foreground notifications, iOS triggers a call to localNotificationReceived as soon as a local notification is received

Android however only does this when the local foreground notification is tapped, which makes more sense

This change makes iOS behave same as Android

…ationReceived only triigers when tapped, not when shown)
@javieranton-zz javieranton-zz changed the title Aligned iOS local fg notification behavior with Android (localNotificationReceived only triigers when tapped, not when shown) Aligned iOS local fg notification behavior with Android (localNotificationReceived only triggers when tapped, not when shown) Jan 18, 2021
No need to check for Active; that would prevent local foreground notifications being actionable when the app has gone back to background
@shai-almog shai-almog requested a review from shannah January 18, 2021 02:17
Copy link
Contributor Author

@javieranton-zz javieranton-zz left a comment

Choose a reason for hiding this comment

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

You're absolutely right, I had a feeling it would be the case but couldn't test because I only have iOS 10+ devices (iPad 4 & iPhone 6s)

Ports/iOSPort/nativeSources/CodenameOne_GLAppDelegate.m Outdated Show resolved Hide resolved
@javieranton-zz
Copy link
Contributor Author

Hi, I must have missed your 13 day old comment. Apologies, I was convinced there wasn't a reply. To answer your question: yes, I have tested this extensively. Like thousands of times, I also have released builds with these modifications and tested in production. I always end up having to edit the delegate so all merging this will do is just save me having to do that. But I thought it would make sense for everyone too if it behaved this way

I have tested on iOS 10 and iOS 13, so if you can test on lower versions that would be great too (I don't have any device with lower than iOS 10)

In older iOS versions, local notifs will still go straight to localNotifReceived, but I guess that's fine

@shai-almog
Copy link
Collaborator

This will have to wait to next week as we're in code freeze for 7.0. Please ping us again in the middle of next week and we'll try to merge it assuming @shannah has no objections.

@javieranton-zz
Copy link
Contributor Author

This will have to wait to next week as we're in code freeze for 7.0. Please ping us again in the middle of next week and we'll try to merge it assuming @shannah has no objections.

No worries Shai, I'll try to remember

@javieranton-zz
Copy link
Contributor Author

Ping. TBH, I'm not sure this should be merged. There is a clear user benefit but unless someone can fully test this with CN1's push it might break things. I implement my own push so no matter how much I test it won't cover all scenarios. OK to close

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