-
Notifications
You must be signed in to change notification settings - Fork 762
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
Android overhaul (includes important bug fixes and new functionality) #71
Conversation
d2e0d0f
to
b3d2675
Compare
ea83788
to
c02c5bd
Compare
Looks like |
My application uses a
or on receiving notification app crashes with
I'm not familiar with java world so not sure what the best solution is. |
Thanks for pointing that out. I must have missed it when splitting things up into feature commits. I've added an additional commit exposing the method.
This is intentional. When you invalidate a token with Firebase the token temporarily becomes Although unlikely, if you had connectivity issues it is possible that receiving the new token could take some time - so it's worth reporting the
This kind of has nothing to do with The issue itself is simply caused by the fact your application is very large (lots of Java methods). That's mostly due to React Native being large, however linking in other libraries results in more methods and eventually they can't all fit in one "Dex file" (an Android JVM construct). So you need to enable multidex. This is simply a limitation of the Android platform. More details:
Are you saying you needed to use both GCM and FCM, or else the app crashed? That sounds peculiar, if anything I would expect the two libraries to conflict. If you're using FCM you really should remove GCM. That crash isn't very descriptive unfortunately. Are you able to recreate the crash with a debug build (or even release with proguard disabled)? If so please provide the stack trace and a description of exactly what state the app was in and what actions were required to trigger the crash. |
this is very much needed especially fcm |
if (Looper.getMainLooper() == Looper.myLooper()) { | ||
emitEvent(event); | ||
} else { | ||
mHandler.post(new Runnable() { |
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.
If anybody is wondering why this is necessary, it is because our events are coming from IntentService
(not regular Service
) sub-classes. IntentService
run on a dedicated background thread rather than the main thread. This becomes a problem when the app is launched in response to push notification being received. In particular, no ReactInstanceManager
exists yet, and attempting to retrieve/create it leads to a crash inside React Native. React Native expects ReactInstanceManager
to only be created on the main thread (although it can be retrieved and utilised from any thread), internally attempting to create an Android Handler
on an "unprepared" Looper
leads to a runtime exception.
5ca189b
to
a207aa4
Compare
Rebased on master due to Otherwise, I'm still awaiting feedback/review/merge 😄 |
@Benjamin-Dobell I am using React-Native-Navigation which implements Native Navigation and to integrate that with this library, I was referring to this guide (https://github.com/wix/react-native-notifications/wiki/Android:-working-with-RNN) But as there's is no @d4vidi it would be really great if this PR gets merged. |
@rohanraarora I haven't gotten around to integrating Native Navigation in my RN app yet. However, I have tried to ensure my changes to The new
You'll want to inherit from Please let me know how you go. |
@Benjamin-Dobell Hi, great work with this one, i just have a small question, does this PR addresses the following?:
Thanks in advanced. |
@batusai513 I'm using this fork in production and it is working on cold, warm and hot boots from notifications without any issues. |
@tommeier Thanks for your answer, I'm trying to install this branch but i'm failing to make it work, I can't receive the deviceToken, it would be to much if i ask for your help? here I share my config: <manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="com.strivehub"
android:versionCode="1"
android:versionName="1.0">
<uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.WAKE_LOCK" />
<uses-permission android:name="android.permission.VIBRATE" android:maxSdkVersion="18" />
<uses-sdk
android:minSdkVersion="16"
android:targetSdkVersion="22" />
<application
android:name=".MainApplication"
android:allowBackup="true"
android:label="@string/app_name"
android:icon="@mipmap/ic_launcher"
android:theme="@style/AppTheme">
<activity
android:name=".SplashActivity"
android:label="@string/app_name"
android:configChanges="keyboard|keyboardHidden|orientation|screenSize"
android:windowSoftInputMode="adjustResize"
android:exported="true"
android:theme="@style/SplashTheme">
<intent-filter>
<action android:name="android.intent.action.MAIN" />
<category android:name="android.intent.category.LAUNCHER" />
</intent-filter>
</activity>
<activity
android:hardwareAccelerated="true"
android:name=".MainActivity"
android:exported="true"
android:configChanges="keyboard|keyboardHidden|orientation|screenSize"
/>
<activity android:name="com.facebook.react.devsupport.DevSettingsActivity" />
<service
android:name="com.wix.reactnativenotifications.fcm.FcmMessageHandlerService">
<intent-filter>
<action android:name="com.google.firebase.MESSAGING_EVENT"/>
</intent-filter>
</service>
<service
android:name="com.wix.reactnativenotifications.fcm.FcmInstanceIdListenerService">
<intent-filter>
<action android:name="com.google.firebase.INSTANCE_ID_EVENT"/>
</intent-filter>
</service>
<service
android:name="com.wix.reactnativenotifications.fcm.FcmTokenService"
android:exported="false" />
<meta-data
android:name="com.wix.reactnativenotifications.gcmSenderId"
android:value="725801312638\0"/>
</application>
</manifest> // Top-level build file where you can add configuration options common to all sub-projects/modules.
buildscript {
repositories {
jcenter()
maven { url 'https://maven.fabric.io/public' }
}
dependencies {
classpath 'com.android.tools.build:gradle:2.2.3'
classpath 'io.fabric.tools:gradle:1.+'
classpath 'com.google.gms:google-services:3.1.0'
// NOTE: Do not place your application dependencies here; they belong
// in the individual module build.gradle files
}
}
allprojects {
repositories {
mavenLocal()
jcenter()
maven {
// All of React Native (JS, Obj-C sources, Android binaries) is installed from npm
url "$rootDir/../node_modules/react-native/android"
}
maven { url 'https://maven.google.com' }
}
}
Thanks in advanced 👍 |
@Benjamin-Dobell Thanks for the info, I tried that but I am having a weird issue implementing this branch with React Native Navigation. The issue is that my app would stuck on the startup if it's being launched first time, minimising it and resuming again would launch the app properly. I also tried creating a new project with just React Native Navigation and this branch but I got the same weird behaviour. Then I replaced this branch with the master and that weird behaviour is gone but I am not able to push data-only message (which my app should control). It would show a blank notification if there's no title or body in the push message. Do you have any idea what might be issue here? I am having a hard time integrating this library with React Native Navigation. Thanks in advance. [Edit] @d4vidi Can you shed some light on the React Native Navigation part or why that's behaviour might be happening? Thanks. |
@batusai513 |
Did anyone try to customize local android notifications layout? Wiki is depreacted after all those changes |
Hi, guys. Is this going to be merged? Would be great. |
updated: Check if you have a working internet connection in your android emulator. |
Is this PR still compatible with GCM? I tried the PR but was running into errors. Calling refreshToken() crashes because Firebase wasn't initialized. And the registering the token was never invoked. Can we still use the PR without google-services.json? Can an example for the AndroidManifest be provided? |
@hkung77 This PR is not GCM compatible as GCM is deprecated. You need to use FCM instead. An example project: https://github.com/Benjamin-Dobell/ReactNativeNotificationsExample |
Thanks @Benjamin-Dobell for the response! I was able to get it working setting up firebase. Perhaps |
It is covered: Although perhaps I need to rearrange the README a bit. I'm open to suggestions 😄 |
Yea Sorry about that! I saw that section after I posted the comment. Hence I tried to update it... but you beat me to it haha |
@Benjamin-Dobell thanks a lot for this awesome PR! It solved a lot of problems I had with using this together with FCM! I had one problem though: When the app is in the background (not completely closed) and I get a notification (that is not data-only), the notification automatically goes to the system tray (just like you describe in the documentation). However, when I tap on it, my RN App is not relaunched completely, but just woken up. (In your documentation you write: "When a user taps on an system tray generated notification the Android OS will actually re-launch your app's primary/launcher activity. If you're using stock React Native this effectively restarts your app."). Because it is not re-launched, but just woken up, the code, where I would usually call Maybe this happens because I'm using Anyway, I solved this by adding Maybe you could add something about this to the documentation just in case others will run into the same problem as me. PS: I really hope this can be merged soon! Until then, I'm using your fork instead. |
Just a heads up to everyone, I'm probably going to properly fork I'm also thinking about separating the local notification stuff out into a separate package, so that other projects (e.g. https://github.com/invertase/react-native-firebase) can take advantage if they desire. However, if anyone is interested, I've another branch going in my repo (https://github.com/Benjamin-Dobell/react-native-notifications/commits/firebase) that has a lot of additional functionality beyond this PR. In particular there's proper Android channel support (i.e. you can create/configure channels from JS), support for expandable notification styles, Android O fixes and WIP support for Android actions. It's a WIP so I don't recommend pointing your |
@Benjamin-Dobell are you still considering creating your own npm package? If you are, it would be awesome if you could include my PRs #88 and #90 or equivalent feature set. It would be amazing to use npm again. |
Absolutely. Although I must admit, it's not a huge priority for me at the moment as I've been kept busy with some unrelated work. If someone else beats me to it, then I'll happily jump on board as a maintainer though. |
@Benjamin-Dobell Thanks for your work on this, helped a great deal with some of our issues. We are currently working around a few small remaining issues and just wondered if you knew of potential causes. We are sending push notifications to our app to make a member aware they have received a new message. Current problems: (1) (2) Any help would be tremendously appreciated! |
@kanso-michael That does sound like a bit of a doozy. Off the top of my head I'm not much use unfortunately.
Your application should be started up to process the remote notification (i.e. so you can present a local notification etc.), this is the advantage of sending a data notification. However, although your app starts it should be doing so in the background. If it's not there may indeed be something slightly odd going with React Native Navigation, I'm not using React Native Navigation at present so unfortunately I'm not much help in that regard.
This isn't anything The only suggestion I can make, at least for testing purposes, is to try a lower priority when creating your local notification. If your notification isn't high priority then fullscreenIntents will have no impact. So if lowering the priority fixes the issue, then you're onto something. If it's happening and your notifications aren't high priority, then this is probably not the issue - I suspect the latter.
I'm inclined to think these issues are related, if not one and the same. It sounds like when your app is starting it's pushing an activity - that might be something React Native Navigation is doing. Unfortunately without debugging your application I probably can't be of much more help. I do actually offer consulting services, although little contracts for fixing a bug or two generally aren't particularly economical. Nonetheless, if you're totally blocked by this it's an option. |
@Benjamin-Dobell Thanks for taking the time to read my wall of ramble. calcazar/react-native-push-notification-CE#18 (comment) This seems to have prevented the app from automatically opening and correctly resuming from clicking on a push notification. The last remaining issue we have is that we can't seem to get the event listeners to be correctly hit after receiving a message from a dead start.
We are seeing this being hit in
Which then results in the event being fired: Now, I am of the understanding that if we implement:
We should be seeing that event handler hit.
And we can use that notification data. If my understanding is correct as to that being what we need to do to receive the notifications from a cold start, then it is a navigation stack setup/order/timing issue that is causing this last problem. Thanks for reading my wall of text and greatly appreciate your previous help. |
@kanso-michael It's mentioned a little earlier in this thread, but my recommendations for handling remote (push) notifications (such that they work from dead state) and creating local notifications is available as an example project with the most relevant code being: https://github.com/Benjamin-Dobell/ReactNativeNotificationsExample/blob/master/js/root.js The basic principles are also covered in the README of my fork, but seeing it in practice ought to give the most clarity. Generally speaking I don't think you actually ever need to use |
@Benjamin-Dobell Thank you so much for assisting, that code example is awesome. I have successfully got everything working in Android and iOS using your fork. |
@kanso-michael |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
The issue has been closed for inactivity. |
This is a mammoth pull request, and I know that's a pain for maintainers. So I've tried to make this as easily digestible as possible. I've elaborated on the motivation for the changes below, and although I didn't write it that way, I've gone back and split this into smaller feature commits so they can each be evaluated some-what individually.
This is the largest commit of the lot, but mostly does what it says. The motivation for upgrading from GCM (deprecated) to FCM is pretty obvious but some of the refactoring probably needs further explanation/justification.
In particular I've completely done away with the concept of
PushNotification
and split its functionality into two classes,LocalNotification
andRemoteNotification
, which are slightly more limited in scope.This is a breaking change to the Android Java API.
Additionally
PushNotificationProps
has been changed toNotificationProps
to highlight its dual role and had its behaviour adjusted to more accurately represent Google/Firebase's concept of a notification. In particular, FirebaseRemoteMessage
class does not bundle end-user data arguments in with UI notification properties. Notification properties and "data" are distinct concepts.NotificationProps
contains agetData()
method to return the data only, the notification properties can be accessed by their corresponding accessor methods. This flows over into JS where there is a property calleddata
, which should take a JS object and all user/custom data should be part of that object, not as part of the top-level notification. This is also a breaking change.These breaking changes weren't made arbitrarily or even to keep the project neat. They're actually required to facilitate correct function. In particular we need to be able to reliably determine whether a notification is "data only" so that JS consumers know under which context a notification is being delivered to them. I believe this is something that was perhaps missed by the original author of this functionality, but there are very distinct rules under which notifications are delivered to running applications. I've actually explained these rules in a README update (later commit) but the official Firebase reference explains in detail.
The final breaking change is that
RemoteNotification
does not automatically post a local notification when a push/remote notification is received (in the foreground). This is important as it gives JS consumers control over when they'd like to display notifications and also what information they include in said notifications. Firebase does not support several options (large icons, LED lighting) etc.Android now supports
cancelAllLocalNotifications
.Fairly self explanatory, this is necessitated by the fact Android's JS events are broadcast "globally", so it's best to avoid any accidental interference by name-spacing.
A simple mechanism to invalidate push tokens. This can be used for example when a user logs out to ensure the app stops receiving notifications.
I've covered this pretty thoroughly in the README and it relates to the "data-only" changes mentioned above (first commit) - or in this case, when the notification isn't data-only. If the system tray receives our push notification because the app was backgrounded, at the moment there's no way to tell when the user launched the app by tapping that notification - this commit corrects that situation.
This is about giving control to JS consumers. At present any time the app is launched all notifications are dismissed. That's likely to be undesirable for many use cases, particularly when a user has received multiple push notifications pertaining to different components of the app. The support that was added (first commit) for
cancelAllLocalNotifications()
means users can still achieve this effect if it's desirable. This is a breaking change with respect to default expected behaviour.This is actually pretty dang nifty. It's now trivial to specify
largeIcon
appears in the notification (typically used in messaging apps as profile pictures). Fresco (React Native's image manager) is used to download the images if they're specified as remote URLs - it takes care of caching and what-not.Additionally,
file://
URLs are supported, which is what React Native convertsrequire("./some_image.jpg")
syntax to in JS. So React Native bundled images can seamlessly be used in addition to remote images.Just a nit. Nothing particularly important or pressing.
Got rid of
PendingNotifications
, as it was an unnecessary namespace. This is a breaking change.Added support for controlling the LED notification light i.e. What Facebook and similar apps do.
Firebase (system tray) remote notifications are actually incapable of achieving this, so it's one of many very good reasons to send "data-only" notifications and generate local notifications from the content.
7ff0123 - Android README updates
dc3e462 - Expose invalidateToken() to JS
Expose invalidateToken() implemented in 521f0c7 to JS.
Ensure the Android native tests compile correctly with the latest changes.
General clean-up and removal of dead/unnecessary code.
Fixed (or rather added) proper background notification support. Android needs a background queue, for precisely the same reasons iOS needs one. Therefore, I've added
NotificationsAndroid.consumeBackgroundQueue()
.Events won't be delivered at all unless this is called, which is a breaking change, but the same behaviour as iOS. I've updated the documentation accordingly. In doing so I've also addressed some misleading advice / sample code. In particular the (iOS) documentation showed adding and removing event listeners inside components. This is incorrect, "cold start" background notifications will not immediately be received by your listener if they're added inside a component, as the component hierarchy does not exist at all until your app is foregrounded.
Probably easiest way to explain this is just to refer to the discussion/PR for the same change merged into
react-native-maps
- react-native-maps/react-native-maps#1481.Prior to this commit, there were issues with multiple versions of Firebase being included due to a bug in the Google Services plugin. This commit provides updated installation instructions that work-around the bug in the Google Services plugin.
--
If you made it all the way through that then your time was hugely appreciated. You deserve a gold star! ⭐️
I'm certainly open to feedback and happy to correct code style issues and what-not.