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

Option to silence Android foreground notifications #1183

Merged
merged 7 commits into from
Apr 16, 2020

Conversation

andrewtremblay
Copy link
Contributor

A common use case in Android apps is to not show notifications when the app is open in front of the user as they can be distracting (this is default behavior in iOS).

This PR adds the android-only ignoreInForeground property for easier UX parity with iOS notifications.

@xxRockOnxx
Copy link

Missing some import statements

@wvicioso
Copy link

wvicioso commented Nov 5, 2019

@andrewtremblay @xxRockOnxx I don't have enough native android experience to do it myself but could you expand this PR so that the ignoreInForeground feature works for localNotificationSchedule as well?

@andrewtremblay-pear
Copy link
Contributor

localNotificationSchedule/scheduleLocalNotification was my main use case and ignoreInForeground should work with it. Are you not seeing the flag respected for some reason?

@sunweiyang
Copy link

sunweiyang commented Nov 13, 2019

@andrewtremblay Thanks for working on this! We'll find this very useful.

But @xxRockOnxx is right; import statements are needed. Your RNPushNotificationHelper.java needs the following imports:

import android.app.ActivityManager;
import android.app.ActivityManager.RunningAppProcessInfo;
import java.util.List;

@andrewtremblay-pear
Copy link
Contributor

Thanks for reviewing! I added the missing imports (not sure how those were omitted).
Let me know id there's anything else needed for approval.

@lisichka999
Copy link

lisichka999 commented Jan 26, 2020

@andrewtremblay-pear hi, looks like I really need this fix) How can I tell to package.json to install package from master directly? Thanks

@andrewtremblay
Copy link
Contributor Author

andrewtremblay commented Jan 26, 2020

@lisichka999 you can use my repo directly by following the git url format git://github.com/<user>/<project>.git#<branch> .
As of NPM version 1.1.65, you can do this:
<user>/<project>#<branch>

So in your package.json, change the react-native-push-notification dependency to
'react-native-push-notification': 'andrewtremblay/react-native-push-notification#master'

https://docs.npmjs.com/files/package.json#git-urls-as-dependencies

@lisichka999
Copy link

@andrewtremblay Thank you, now I can see your code. I just noticed that it is only related to local notifications, can it work for remote notification as well? Or should I find another fix?

@andrewtremblay
Copy link
Contributor Author

@lisichka999 Correct. I'm only concerned with scheduled local notifications. But I think remote push notifications use the same helper function that I modified. If you add ignoreInForeground to your remote push payload it might work.

If that doesn't work and you want to ignore all foreground notifications, fork my repo & change this line,
if (!(this.isApplicationInForeground(context) && bundle.getBoolean("ignoreInForeground"))) {

To this:
if (!(this.isApplicationInForeground(context)) {

@lisichka999
Copy link

@andrewtremblay Thanks for hint, will try it

StagasaurusRex added a commit to StagasaurusRex/react-native-push-notification that referenced this pull request Apr 8, 2020
@mtshv
Copy link

mtshv commented Apr 14, 2020

@Dallas62 Please, consider merging this PR.
Very useful functionality. It looks like suggested code was already partly reviewed by the community

@Dallas62
Copy link
Collaborator

Hi @mtshv
I a bit confuse about this part of the PR:

                        for (String d : processInfo.pkgList) {
                            return true;
                        }

A check of the length should be enough, no ?

@mtshv
Copy link

mtshv commented Apr 16, 2020

I think it's best if PR author takes a look. @andrewtremblay ^^

@andrewtremblay
Copy link
Contributor Author

I a bit confuse about this part of the PR:
A check of the length should be enough, no ?

That might be a better question for the maintainers who last touched isApplicationInForeground in RNPushNotitificationListenerService and/or RNPushNotitificationListenerServiceGcm. I essentially copied the isApplicationInForeground function from there.

I agree that the check is weird though. Will update.

@Dallas62
Copy link
Collaborator

Hi @andrewtremblay
I saw later the origin of those lines, thanks for the simplification !

@Dallas62 Dallas62 merged commit f07a184 into zo0r:master Apr 16, 2020
@Dallas62
Copy link
Collaborator

But I'm still waiting for an answer from the Owner of the repository to be able to release the library 😅

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.

8 participants