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

Android overhaul (includes important bug fixes and new functionality) #71

Closed
wants to merge 17 commits into from

Conversation

Benjamin-Dobell
Copy link
Contributor

@Benjamin-Dobell Benjamin-Dobell commented Jun 1, 2017

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.

  • c967bff - Migrate to FCM and refactor

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 and RemoteNotification, which are slightly more limited in scope.

This is a breaking change to the Android Java API.

Additionally PushNotificationProps has been changed to NotificationProps to highlight its dual role and had its behaviour adjusted to more accurately represent Google/Firebase's concept of a notification. In particular, Firebase RemoteMessage class does not bundle end-user data arguments in with UI notification properties. Notification properties and "data" are distinct concepts.

NotificationProps contains a getData() 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 called data, 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.

  • ae36e8a - Namespace globally broadcast JS events

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.

  • 521f0c7 - Support FCM token invalidation

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.

  • 9842411 - Fixed initial intent behavior for background 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.

  • faf442e - Do not automatically dismiss all notifications

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.

  • 812ced3 - Android large icon support (URL or drawable name)

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 converts require("./some_image.jpg") syntax to in JS. So React Native bundled images can seamlessly be used in addition to remote images.

  • de8c172 - Minor code simplification/clean-up

Just a nit. Nothing particularly important or pressing.

  • f084ebd - Move getInitialNotification() NotificationsAndroid (match iOS)

Got rid of PendingNotifications, as it was an unnecessary namespace. This is a breaking change.

  • 59c6237 - Android lights/LED support

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.

  • 3c1f7a7 - Fixed native Android test compilation

Ensure the Android native tests compile correctly with the latest changes.

  • 835cd86 - Refactored ProxyService into LocalNotificationService

General clean-up and removal of dead/unnecessary code.

  • ff3ae15 - Android background queue

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.

  • a207aa4 - RN 0.47 support (handle Android breaking change)

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.

  • 9edcddf - Updated README installation instructions for Android.

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.

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Jun 2, 2017

I should probably note that this series of commits adds support for:

  • tag

  • sound

  • icon (small)

  • lightsColor, lightsOnMs, lightsOffMs (notification LED control)

  • color

    color

  • largeIcon (This example image was specified as a remote URL)

    largeicon

These are covered in the updated README.md.

@AlimovSV
Copy link

AlimovSV commented Jun 9, 2017

Looks like NotificationsAndroid.invalidateToken is not available. Also moment when I call NativeModules.WixRNNotifications.invalidateToken() directly, the handler set by setRegistrationTokenUpdateListener called twice - with empty token and with newly generated token. is it ok?

@AlimovSV
Copy link

AlimovSV commented Jun 9, 2017

My application uses a react-native-device-info. And when I migrate to your branch, i've got some conflict and:

  1. Have to use multiDexEnabled true or app:transformClassesWithDexForDebug fail with
Dex: Error converting bytecode to dex:
Cause: com.android.dex.DexIndexOverflowException: Cannot merge new index 65819 into a non-jumbo instruction!
  1. Have to use next gradle configuration:
    compile(project(':react-native-device-info')) {
        exclude group: 'com.google.android.gms', module: 'play-services-gcm'
    }
    compile(project(':react-native-notifications')) {
        exclude group: 'com.google.firebase', module: 'firebase-messaging'
    }
...
    compile 'com.google.android.gms:play-services-gcm:10.2.6'
    compile 'com.google.firebase:firebase-messaging:10.2.6'

or on receiving notification app crashes with

java.lang.NoSuchMethodError: No static method zzz ...

I'm not familiar with java world so not sure what the best solution is.

@Benjamin-Dobell
Copy link
Contributor Author

Benjamin-Dobell commented Jun 11, 2017

@AlimovSV

Looks like NotificationsAndroid.invalidateToken is not available

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.

the handler set by setRegistrationTokenUpdateListener called twice - with empty token and with newly generated token. is it ok?

This is intentional.

When you invalidate a token with Firebase the token temporarily becomes null - to signify you don't have a token available. However, Firebase then sees you don't have a token and automatically generates you a new one - which triggers a second call to the token listener (with the new token).

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 null token first to let any listeners know the old token is in fact invalid.

  1. Have to use multiDexEnabled true or app:transformClassesWithDexForDebug fail with

This kind of has nothing to do with react-native-notifications or react-native-device-info. I'm using both these libraries myself and haven't run into this (yet).

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:
https://developer.android.com/studio/build/multidex.html

  1. Have to use next gradle configuration: ...

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.

@jjdp
Copy link

jjdp commented Jun 23, 2017

this is very much needed especially fcm

if (Looper.getMainLooper() == Looper.myLooper()) {
emitEvent(event);
} else {
mHandler.post(new Runnable() {
Copy link
Contributor Author

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 ReactInstanceManagerexists 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.

@Benjamin-Dobell
Copy link
Contributor Author

Rebased on master due to README.md conflicts. Initial comment has been updated with the most recent commit references.

Otherwise, I'm still awaiting feedback/review/merge 😄

@rohanraarora
Copy link

@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 PushNotification class in your wha would be the best way to integrate with React Native Navigation and thanks a lot for the efforts and FCM integration.

@d4vidi it would be really great if this PR gets merged.

@Benjamin-Dobell
Copy link
Contributor Author

@rohanraarora I haven't gotten around to integrating Native Navigation in my RN app yet. However, I have tried to ensure my changes to react-native-notifications remain compatible.

The new INotificationsApplication method to override is:

ILocalNotification getLocalNotification(Context context, NotificationProps notificationProps, AppLifecycleFacade facade, AppLaunchHelper defaultAppLaunchHelper);

You'll want to inherit from LocalNotification instead of PushNotification.

Please let me know how you go.

@batusai513
Copy link

batusai513 commented Jan 17, 2018

@Benjamin-Dobell Hi, great work with this one, i just have a small question, does this PR addresses the following?:

  • Not firing the corresponding events when the application is in background on IOS and Android?
  • Not receiving any notifications when the app is killed on Android? (also notification banner is removed when closing the app)

Thanks in advanced.

@tommeier
Copy link

@batusai513 I'm using this fork in production and it is working on cold, warm and hot boots from notifications without any issues.

@batusai513
Copy link

batusai513 commented Jan 18, 2018

@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' }
    }
}
apply plugin: "com.android.application"
apply plugin: 'io.fabric'

repositories {
    maven { url 'https://maven.fabric.io/public' }
}
import com.android.build.OutputFile

/**
 * The react.gradle file registers a task for each build variant (e.g. bundleDebugJsAndAssets
 * and bundleReleaseJsAndAssets).
 * These basically call `react-native bundle` with the correct arguments during the Android build
 * cycle. By default, bundleDebugJsAndAssets is skipped, as in debug/dev mode we prefer to load the
 * bundle directly from the development server. Below you can see all the possible configurations
 * and their defaults. If you decide to add a configuration block, make sure to add it before the
 * `apply from: "../../node_modules/react-native/react.gradle"` line.
 *
 * project.ext.react = [
 *   // the name of the generated asset file containing your JS bundle
 *   bundleAssetName: "index.android.bundle",
 *
 *   // the entry file for bundle generation
 *   entryFile: "index.android.js",
 *
 *   // whether to bundle JS and assets in debug mode
 *   bundleInDebug: false,
 *
 *   // whether to bundle JS and assets in release mode
 *   bundleInRelease: true,
 *
 *   // whether to bundle JS and assets in another build variant (if configured).
 *   // See http://tools.android.com/tech-docs/new-build-system/user-guide#TOC-Build-Variants
 *   // The configuration property can be in the following formats
 *   //         'bundleIn${productFlavor}${buildType}'
 *   //         'bundleIn${buildType}'
 *   // bundleInFreeDebug: true,
 *   // bundleInPaidRelease: true,
 *   // bundleInBeta: true,
 *
 *   // whether to disable dev mode in custom build variants (by default only disabled in release)
 *   // for example: to disable dev mode in the staging build type (if configured)
 *   devDisabledInStaging: true,
 *   // The configuration property can be in the following formats
 *   //         'devDisabledIn${productFlavor}${buildType}'
 *   //         'devDisabledIn${buildType}'
 *
 *   // the root of your project, i.e. where "package.json" lives
 *   root: "../../",
 *
 *   // where to put the JS bundle asset in debug mode
 *   jsBundleDirDebug: "$buildDir/intermediates/assets/debug",
 *
 *   // where to put the JS bundle asset in release mode
 *   jsBundleDirRelease: "$buildDir/intermediates/assets/release",
 *
 *   // where to put drawable resources / React Native assets, e.g. the ones you use via
 *   // require('./image.png')), in debug mode
 *   resourcesDirDebug: "$buildDir/intermediates/res/merged/debug",
 *
 *   // where to put drawable resources / React Native assets, e.g. the ones you use via
 *   // require('./image.png')), in release mode
 *   resourcesDirRelease: "$buildDir/intermediates/res/merged/release",
 *
 *   // by default the gradle tasks are skipped if none of the JS files or assets change; this means
 *   // that we don't look at files in android/ or ios/ to determine whether the tasks are up to
 *   // date; if you have any other folders that you want to ignore for performance reasons (gradle
 *   // indexes the entire tree), add them here. Alternatively, if you have JS files in android/
 *   // for example, you might want to remove it from here.
 *   inputExcludes: ["android/**", "ios/**"],
 *
 *   // override which node gets called and with what additional arguments
 *   nodeExecutableAndArgs: ["node"],
 *
 *   // supply additional arguments to the packager
 *   extraPackagerArgs: []
 * ]
 */

project.ext.react = [
    entryFile: "index.js"
]

apply from: "../../node_modules/react-native/react.gradle"

/**
 * Set this to true to create two separate APKs instead of one:
 *   - An APK that only works on ARM devices
 *   - An APK that only works on x86 devices
 * The advantage is the size of the APK is reduced by about 4MB.
 * Upload all the APKs to the Play Store and people will download
 * the correct one based on the CPU architecture of their device.
 */
def enableSeparateBuildPerCPUArchitecture = false

/**
 * Run Proguard to shrink the Java bytecode in release builds.
 */
def enableProguardInReleaseBuilds = false

project.ext.vectoricons = [
    iconFontNames: [ "FontAwesome.ttf" ]
]

apply from: "../../node_modules/react-native-vector-icons/fonts.gradle"

android {
    compileSdkVersion 23
    buildToolsVersion '23.0.1'

    defaultConfig {
        applicationId "com.strivehub"
        minSdkVersion 16
        targetSdkVersion 22
        versionCode 3
        versionName "1.1"
        ndk {
            abiFilters "armeabi-v7a", "x86"
        }
    }
    signingConfigs {
        release {
            if (project.hasProperty('MYAPP_RELEASE_STORE_FILE')) {
                storeFile file(MYAPP_RELEASE_STORE_FILE)
                storePassword MYAPP_RELEASE_STORE_PASSWORD
                keyAlias MYAPP_RELEASE_KEY_ALIAS
                keyPassword MYAPP_RELEASE_KEY_PASSWORD
            }
        }
    }
    splits {
        abi {
            reset()
            enable enableSeparateBuildPerCPUArchitecture
            universalApk false  // If true, also generate a universal APK
            include "armeabi-v7a", "x86"
        }
    }
    buildTypes {
        debug {
            manifestPlaceholders = [excludeSystemAlertWindowPermission: "false"]
        }
        release {
            manifestPlaceholders = [excludeSystemAlertWindowPermission: "true"]
            minifyEnabled enableProguardInReleaseBuilds
            proguardFiles getDefaultProguardFile("proguard-android.txt"), "proguard-rules.pro"
            signingConfig signingConfigs.release
        }
    }
    // applicationVariants are e.g. debug, release
    applicationVariants.all { variant ->
        variant.outputs.each { output ->
            // For each separate APK per architecture, set a unique version code as described here:
            // http://tools.android.com/tech-docs/new-build-system/user-guide/apk-splits
            def versionCodes = ["armeabi-v7a":1, "x86":2]
            def abi = output.getFilter(OutputFile.ABI)
            if (abi != null) {  // null for the universal-debug, universal-release variants
                output.versionCodeOverride =
                        versionCodes.get(abi) * 1048576 + defaultConfig.versionCode
            }
        }
    }
}

dependencies {
    compile project(':bugsnag-react-native')
    compile project(':react-native-keychain')
    compile project(':react-native-video')
    compile project(':react-native-orientation')
    compile project(':react-native-vector-icons')
    compile(project(':react-native-notifications')) {
        exclude group: 'com.google.firebase', module: 'firebase-messaging'
    }
    compile 'com.google.android.gms:play-services-base:11.0.4'
    compile 'com.google.firebase:firebase-messaging:11.0.4'
    compile fileTree(dir: "libs", include: ["*.jar"])
    compile "com.android.support:appcompat-v7:23.0.1"
    compile "com.facebook.react:react-native:+"  // From node_modules
    compile('com.crashlytics.sdk.android:crashlytics:2.6.8@aar') {
        transitive = true;
    }
}

// Run this once to be able to run the application with BUCK
// puts all compile dependencies into folder libs for BUCK to use
task copyDownloadableDepsToLibs(type: Copy) {
    from configurations.compile
    into 'libs'
}

buildscript {
  repositories {
    maven { url 'https://maven.fabric.io/public' }
  }
  dependencies {
    classpath 'io.fabric.tools:gradle:1.22.1'
  }
}

apply plugin: 'com.google.gms.google-services'

Thanks in advanced 👍

@rohanraarora
Copy link

rohanraarora commented Jan 18, 2018

@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]
I have also created a demo app using this branch as well as the current release of this library to show this behaviour.
App using this branch: https://github.com/rohanraarora/navnoti/tree/rnnavigation-rnnotification-fcm
App using the current release of this lib (1.2.0): https://github.com/rohanraarora/navnoti/tree/rnnav-rnnoti

@d4vidi Can you shed some light on the React Native Navigation part or why that's behaviour might be happening? Thanks.

@tommeier
Copy link

tommeier commented Jan 28, 2018

@batusai513 Sorry - been away - from a quick scan, I had a similar thing where it needs to have compile 'com.google.android.gms:play-services-base:11.+' to work correctly. See if that helps? Looks like you have that, sorry - I'm not sure why it wouldn't be working for you - perhaps something on your JS side.

@jankowskip
Copy link

jankowskip commented Feb 1, 2018

Did anyone try to customize local android notifications layout? Wiki is depreacted after all those changes
Maybe you can help @Benjamin-Dobell

@kyryloz
Copy link

kyryloz commented Feb 23, 2018

Hi, guys. Is this going to be merged? Would be great.

@alexismoreau
Copy link

alexismoreau commented Mar 8, 2018

updated:
if you have that problem :
Firebase has a new token: FirebaseInstanceId:'something', token=null

Check if you have a working internet connection in your android emulator.

@hkung77
Copy link

hkung77 commented Mar 27, 2018

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?

@Benjamin-Dobell
Copy link
Contributor Author

@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

@hkung77
Copy link

hkung77 commented Mar 28, 2018

Thanks @Benjamin-Dobell for the response! I was able to get it working setting up firebase. Perhaps
the consumeBackgroundQueue() could be added to the setting up token registration section in the README. (https://github.com/wix/react-native-notifications/pull/71/files#diff-04c6e90faac2675aa89e2176d2eec7d8R202)?

@Benjamin-Dobell
Copy link
Contributor Author

I did notice you did this in your example repository. But perhaps this should be included in the actual README for setting up token registration.

It is covered:

https://github.com/Benjamin-Dobell/react-native-notifications/tree/android-overhaul#background-queue-important

Although perhaps I need to rearrange the README a bit. I'm open to suggestions 😄

@hkung77
Copy link

hkung77 commented Mar 28, 2018

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

@VSchlattinger
Copy link

@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 NotificationsAndroid.getInitialNotification() is not run. (The listener for NotificationOpened isn't called either, because the App was in the background.)

Maybe this happens because I'm using react-native-navigation?

Anyway, I solved this by adding AppState.addEventListener("change", …) to my code and calling NotificationsAndroid.getInitialNotification().then(…) whenever the state changes from "inactive" or "background" to "active". That solved this problem.

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.

@Benjamin-Dobell
Copy link
Contributor Author

Just a heads up to everyone, I'm probably going to properly fork react-native-notifications and publish to NPM so that we're not all pointing to arbitrary git commits.

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 package.json there (as I might blast away your references), but feel free to fork and point your package.json to your own fork.

@quinton-j
Copy link

@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.

@Benjamin-Dobell
Copy link
Contributor Author

@Benjamin-Dobell are you still considering creating your own npm package?

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.

@kanso-michael
Copy link

@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.
I doubt it's your fork, but your knowledge of the Android side of things would be most helpful. I'll also mention we're using React Native Navigation.

We are sending push notifications to our app to make a member aware they have received a new message.
These push notifications are being sent as data only notifications.

Current problems:

(1)
When sending a notification and the app is dead, the push notification is actually starting up the app. Do you know why this from happening?
Or is this a side effect of using data only push notifications?
I looked for usages of setFullScreenIntent, which seems to be what is occurring, but couldn't find any.
https://developer.android.com/reference/android/app/Notification.Builder#setFullScreenIntent(android.app.PendingIntent,%20boolean)

(2)
When the app is running in the background we successfully create a LocalNotification in the notification centre.
However, on clicking through to be taken into the app we see a white screen on top of the current stack. If you press the back button this screen is successfully popped off the stack and the app continues with what it needs to do.
This is likely to be a RNN issue, but just thought i'd ask if you knew of anything obvious that I might be missing.

Any help would be tremendously appreciated!

@Benjamin-Dobell
Copy link
Contributor Author

@kanso-michael That does sound like a bit of a doozy. Off the top of my head I'm not much use unfortunately.

(1)
When sending a notification and the app is dead, the push notification is actually starting up the app. Do you know why this from happening?

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.

I looked for usages of setFullScreenIntent, which seems to be what is occurring, but couldn't find any.
https://developer.android.com/reference/android/app/Notification.Builder#setFullScreenIntent(android.app.PendingIntent,%20boolean)

This isn't anything react-native-notifications is doing. Actually setFullScreenIntent isn't supported by react-native-notifications.

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.

(2)
When the app is running in the background we successfully create a LocalNotification in the notification centre.
However, on clicking through to be taken into the app we see a white screen on top of the current stack.

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.

@kanso-michael
Copy link

kanso-michael commented Nov 27, 2018

@Benjamin-Dobell Thanks for taking the time to read my wall of ramble.
I seem to have found the issue in-between posting and you replying!
I have narrowed it down to being an issue with react native navigation and seem to have alleviated both described issues by adding the following code:

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.
I'm assuming it is probably something to do with RNN once again but just wanted to check that my understanding is sound.

    @Override
    public void onMessageReceived(RemoteMessage message) {
        Log.d(LOGTAG, "New message from firebase");
        final NotificationProps notificationProps = NotificationProps.fromRemoteMessage(this, message);
        new RemoteNotification(this, notificationProps).onReceived();
    }

We are seeing this being hit in adb logcat which in turn should hit..

    private void sendReceivedEvent() {
        mJsIOHelper.sendEventToJS(NOTIFICATION_RECEIVED_EVENT_NAME, mNotificationProps.asBundle());
    }

Which then results in the event being fired:
com.wix.reactnativenotifications.notificationReceived

Now, I am of the understanding that if we implement:

  NotificationsAndroid.setNotificationReceivedListener((notification) => {
    console.log('NOTIFICATION_RECEIVED_LISTENER', notification)
  })

We should be seeing that event handler hit.
We would then schedule a new LocalNotification and the user can then press it to load up the app:

  NotificationsAndroid.getInitialNotification()
    .then((notification) => {
      console.log('INITIAL_NOTIFICATION', notification)
      if (notification) {
        component.onNotificationOpened(notification)
      }
    })
    .catch((err) => {
      console.error(err)
    })

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.

@Benjamin-Dobell
Copy link
Contributor Author

@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 NotificationsAndroid.getInitialNotification(). Seems as I've added a proper background event queue (which you need to consume on start-up with NotificationsAndroid.consumeBackgroundQueue()), any listener you set with NotificationsAndroid.setNotificationOpenedListener(onNotificationOpened) ought to fire just fine, in which case you can handle launching from notifications there.

@kanso-michael
Copy link

kanso-michael commented Nov 29, 2018

@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:

Benjamin-Dobell/ReactNativeNotificationsExample:js/root.js@master

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 NotificationsAndroid.getInitialNotification(). Seems as I've added a proper background event queue (which you need to consume on start-up with NotificationsAndroid.consumeBackgroundQueue()), any listener you set with NotificationsAndroid.setNotificationOpenedListener(onNotificationOpened) ought to fire just fine, in which case you can handle launching from notifications there.

@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.
Hopefully this helps someone in the future if they get stuck!
Hope the maintainers get this merged at some point, fantastic work and cheers for explaining the bits I missed. 👍

@ywongweb
Copy link

@kanso-michael
It's been 4 months since you got this fork working on your app. How is it so far and have you encounter any other problems?

@stale
Copy link

stale bot commented Jul 24, 2019

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.

@stale stale bot added the 🏚 stale label Jul 24, 2019
@stale
Copy link

stale bot commented Jul 31, 2019

The issue has been closed for inactivity.

@stale stale bot closed this Jul 31, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.