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

Handle notification callback in the background #902

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

ochakov
Copy link

@ochakov ochakov commented Oct 29, 2024

PR Type

New feature - Hold notifications when the app is in background and and call the app callback once back in foreground.

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Documentation changes
  • Other... Please describe:

PR Checklist

Please check your PR fulfills the following requirements:

Bugfixes:

  • Regression testing has been carried out using the example project to ensure the intended bug is fixed and no regression bugs have been inadvertently introduced.

New features/enhancements:

  • Exhaustive testing has been carried out for the new functionality
  • Regression testing has been carried out to ensure no existing functionality is adversely affected
  • Documentation has been added / updated
  • The example project has been update to validate/demonstrate the new functionality.

What is the purpose of this PR?

New message notifications, call the onMessageReceived callback. This might be important to handle new notifications by the app, especially data notifications. In both Android and iOS, the plugin is calling the onMessageReceived callback but when the application is in the background for more than 20-30 seconds, the callback does not execute.
In case of Android, when the app is not active (the main activity is closed), the callback is not valid as the WebView does not exist.

The plugin already implements a queue to hold any pending notifications until the app calls onMessageReceived for the first time or the plugin has finished initializing.
This PR extends the functionality of this queue to hold any pending notifications when the app is in the background or inactive. As soon as the application resumes, the plugin will send all pending notifications back to the onMessageReceived callback.

Does this PR introduce a breaking change?

  • Yes
  • No

The default behavior of the plugin when sending notifications to the onMessageReceived callback when the app is in the background is not consistent and depends on the device type and the amount of time the application is in the background. The new behavior must be documented and properly explained.
In case some applications rely on notifications being sent while the app is in the background, this change might break this behavior as notifications will never be calling onMessageReceived callback when the app is in the background.
There should be minimal or no impact on visible notifications.

What testing has been done on the changes in the PR?

Multiple Android and iOS devices tested in all states for processing data notifications.

What testing has been done on existing functionality?

Other notification types seem to not being impacted. No other functionality of the plugin has been changed.

Other information

Bringing this PR to the public discussion to understand if this is something that should be integrated in the plugin. If this is something valuable or could break the existing behavior or something we couldn't think about.

Hold notifications when the app is in background and and call the app callback once back in foreground.
@dpa99c dpa99c changed the base branch from master to dev November 1, 2024 09:03
@dpa99c dpa99c merged commit 7515306 into dpa99c:dev Nov 1, 2024
@dpa99c
Copy link
Owner

dpa99c commented Nov 1, 2024

Thanks, this is a valuable contribution. I'll test this on the dev branch, document the behaviour, and bump the major version of the plugin before releasing due to the breaking change.

dpa99c added a commit that referenced this pull request Nov 1, 2024
@dpa99c
Copy link
Owner

dpa99c commented Nov 1, 2024

I have made an attempt to document the new behaviour: 980c6e9

I'd be most grateful if you have time to quickly review and confirm this is correct.

@dpa99c
Copy link
Owner

dpa99c commented Nov 1, 2024

I'm just wondering if, to aid with backward-compatibility for any applications currently using this plugin and depending on notification payloads being immediately delivered while the app is in the background (rather than queued), we could add support for an optional property in the notification payload e.g. immediate_delivery: true which would bypass the queue and deliver the payload right away after the notification arrives?

@ochakov
Copy link
Author

ochakov commented Nov 1, 2024

to aid with backward-compatibility
notification payload e.g. immediate_delivery: true

This can be a safer option, but I think this option should not involve the server side (sender), but rather be a client side option instead. Can be added as an optional parameter to onMessageReceived function or even a plugin variable.
From our testing, no modern device was found that is receiving data notifications while the app is in the background. Therefore, not sure if anyone can rely on receiving messages in the background at all.

@dpa99c
Copy link
Owner

dpa99c commented Nov 1, 2024

to aid with backward-compatibility
notification payload e.g. immediate_delivery: true

This can be a safer option, but I think this option should not involve the server side (sender), but rather be a client side option instead. Can be added as an optional parameter to onMessageReceived function or even a plugin variable. From our testing, no modern device was found that is receiving data notifications while the app is in the background. Therefore, not sure if anyone can rely on receiving messages in the background at all.

OK, we can make it a plugin variable set at plugin install time which is false by default but if anyone wants to risk using it (maybe they have a specific use case) then they have the option

@dpa99c
Copy link
Owner

dpa99c commented Nov 5, 2024

I have implemented the plugin variable for iOS as well and tested on both platforms (iOS was missing a call to send pending notifications on app resume which is also fixed). It passes my set of manual tests now, so I've merged it to master branch and we'll see what users make of the new 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.

2 participants