Multiple features android

[varungupta85]

Restore local notifications on reboot
Fix cancel all local notifications
Cancel a specific local notification
Notifications actions
Play custom sound for notification

[vikeri]

Great work! Could you document how one may add notification actions? Saw that the method is exposed but no instructions of how to use it.

[varungupta85]

I would have added the instructions once it was reviewed. Here is how I have added notification actions

This is how the action handlers are registered. This can be called in the app componentWillMount/constructor or other appropriate method

import {DeviceEventEmitter} from 'react-native'
import PushNotificationAndroid from 'react-native-push-notification'

(function() {
  // Register all the valid actions for notifications here and add the action handler for each action
  PushNotificationAndroid.registerNotificationActions(['Accept','Reject','Yes','No']);
  DeviceEventEmitter.addListener('notificationActionReceived', function(e){
    console.log ('notificationActionReceived event received: ' + e);
    const info = JSON.parse(e.dataJSON);
    if (info.action == 'Accept') {
      // Do work pertaining to Accept action here
    } else if (info.action == 'Reject') {
      // Do work pertaining to Reject action here
    }
    // Add all the required actions handlers
  });
})();

This is how the notification is scheduled

const scheduleNotificationsForAndroid = function(time, alertBody, info) {
    if (info.type == Constants.NotificationTypes.ALARM) {
      // JSON array isn't handled properly in react native Android bridge. Pass this as a string.
      // This gets converted to JSON array from string in RNPushNotificationHelper.sendNotification().
     // Add all the actions that you want to show for this notification
      info.actions = '["Accept","Reject"]';
    }

    PushNotificationAndroid.localNotificationSchedule({
      message: alertBody,
      date: new Date(time),
      ...info
    });
}

This will cause the above notification to have the Accept and Reject actions.

[varungupta85]

cc @zo0r @npomfret

Would it be possible for someone to review the changes in this PR. It contains a ton of features mainly for android that I have been using in my app. To list the newly added features again:

1. Restore local notifications on reboot
2. Fix cancel all local notifications
3. Cancel a specific local notification
4. Notifications actions
5. Play custom sound for notification
6. Clear notifications from the status bar
7. Perform a task on receiving a remote notification even if the app is in the background which can be controlled by specifying contentAvailable field so that we don't do it for all the remote notifications. Something similar to iOS remote fetch.

I am happy to discuss/explain the changes and make any necessary edits.

Currently, the branch is not up to date with the master and I have checked that all the new commits are non-conflicting and I should be able to take care of it along with any code review comments.

Thanks!

[npomfret]

I'll take a look, maybe over the weekend. Sorry, didn't realise I was a contributor until just now. Could you first resolve the conflicts though?

[varungupta85]

@npomfret Done

[npomfret]

Looking to merge this now but having difficulty. Can you help?

I'm trying to get the Restore local notifications on reboot to work, what do I need to do? Obviously I've tried rebooting the phone but they don't reappear.

Also tried cancelling a notification but it remained in the notification centre. I did see this in the logs:

D/RNPushNotification: Didn't find a notification with 1366315647 while cancelling a local notification

It doesn't look like non-scheduled notifications are saved to the SharedPreferences? I don't know why but I've added that in. Even after this change though it looks like the SharedPreferences is always empty when it gets reloaded. Not sure why yet.

[varungupta85]

Can you add below lines to AndroidManifest.xml file

        <receiver android:name="com.dieam.reactnativepushnotification.modules.RNPushNotificationBootEventReceiver">
            <intent-filter>
                <action android:name="android.intent.action.BOOT_COMPLETED"></action>
            </intent-filter>
        </receiver>

This is required to invoke the package boot event receiver.

A little note on the implementation details for this feature: In order to restore local notifications after reboot, we need to store the notifications in a persistent storage so that they can be read again on reboot and rescheduled. In order to store the notifications, we need to convert it to serializable form. Here I am using SharedPreferences provided by Android for persistent storage and RNPushNotificationAttributes class is used to serialize and deserialize notifications to and from the SharedPreferences

[npomfret]

I've made that change but it looks like the persistence isn't working. saving the notification seems fine, and if I query the persistence immediately after saving I can see the item in there. however, when calling the cancel method for that notification, it seems that the persistence is empty and so nothing gets cancelled.

[npomfret]

Ok, found the issue. So I added regular notifications to the storage so that they can be cancelled and reloaded on boot up in the same way as scheduled notifications. BUT, I failed to notice this code in the helper in sendNotification:

        if (sharedPreferences.getString(Integer.toString(notificationID), null) != null) {
            SharedPreferences.Editor editor = sharedPreferences.edit();
            editor.remove(Integer.toString(notificationID));
            commit(editor);
        }

Its removing the notifications immediately after my new code puts them in! Aaaargh!! been looking at this for hours. I think we need to remove data from the sharedPreferences when its removed from the notification centre, no?

Why is this code in the sendNotification method?

[npomfret]

Ok, I think i finally understand what you've done. I've had to change it in a few places, and done some clean up.

You didn't quite fix 'cancel all local notifications'. You made it possible to cancel scheduled notifications only. I've changed the code so it does in fact cancel all notifications. There's a code block in sendNotification that really needs a comment as I think it's not clear why its there.

"Restore local notifications on reboot" - I've updated my manifest, deleted the app completely and reinstalled. I can see the logging from the RNPushNotificationBootEventReceiver so am happy this loads. And I can now see from the code that this only works for scheduled notifications (because only scheduled notifications are persisted), but this wasn't clear at all and took me a few hours to figure out :(

Why did you decided to only persisting scheduled notifications?

"Notifications actions" - what is this? I can't see any code relating to it or how to test it. Can you please advise?

"Play custom sound for notification" - this didn't work in the way I expected it to, or at least the docs weren't clear on how it should work. I've changed it so that it loads a sound from a resources folder and updated the docs for both Android and iOS. The idea is to make the iOS and android javascript interfaces the same, where possible.

[varungupta85]

@npomfret Thanks for spending time to review the changes and making some changes of your own.

The reason I am persisting only scheduled notifications is because I as far as I understood, the other type of notification will be shown immediately to the user, so I thought there is no point in storing it. Please let me know if I am incorrect in that assumption. Same applies for cancelAllNotifications. I think that they user would only want to cancel scheduled notifications which is why I just cancel all the currently scheduled notifications.

I will add comments in the sendNotification method but that piece of code is supposed to remove the notification from shared preferences because if it were not removed, then it would be shown again to the user when the phone is rebooted because, on reboot, we will try to schedule all the notifications that are present in the shared preferences. So, we need to remove the notification from the shared preferences once it has been shown to the user.

I also noted that you have remote the clearAllNotifications method as part of your commit. I am not sure why. That method is supposed to only clear the notifications from the status bar but it won't affect any other scheduled notification. The cancelAllLocalNotifications method is supposed to clear and also cancel any future notifications.

Please refer to an earlier comment in the PR which explains how to configure notification actions for notifications. #151 explains the notification actions in general.

I am not sure what exactly didn't work for you while playing the custom sound but I saw your changes and they are fine.

Could you take a look at the clearAllNotifications method and let me know if that was removed by mistake?

Small note: I think you have some Save actions configured in Android Studio (actually I am not sure what are they called in Android Studio but they are called Save Actions in Eclipse) which changes the formatting of the code and introduces a lot of false positives in the differences as other people may not have the same formatting actions configured in the editor which makes it a bit difficult to review the changes.

[varungupta85]

@npomfret Forgot to mention, I will update the README soon with the all these changes.

[npomfret]

I'll add the missing method back in. Sorry, was working on this for hours and didn't mean to remove it exactly - it had the wrong name. I was getting confused between cancelAllLocalNotifications (which is supposed to cancel only scheduled notifications - and is very badly named in RN), and cancelLocalNotifications. But your method clearAllNotifications had the wrong name - I think it should be cancelLocalNotifications, no?

I understood that support for using eclipse for Android dev was dropped years ago. Android Studio is very much the industry standard and all formatting really should follow it. Its free, I don't see any reason not to use it.

The custom sound change, well all I can say was that your implementation didn't play a custom sound. It might have worked if you'd provided a URL to the resource I suppose. Did you test it? It needed to have the same interface as the iOS implementation which is to supply the name of a resource in the native project file. Which is what I implemented.

[npomfret]

It seems unclear from the RN docs whether or not cancelLocalNotifications should cancel scheduled notifications or not.

If not, there's no way to cancel a single scheduled notification. But if it should, then what is cancelAllLocalNotifications for?

[npomfret]

I've committed another change. I hope one that's in agreement with the documented RN behaviour in iOS. Would you be able to take a look please?

[varungupta85]

@npomfret I think there is still some confusion regarding the naming of methods to cancel/clear notifications. The cancelLocalNotifications and cancelAllLocalNotifications methods in PushNotificationIOS in react-native are only supposed to work on scheduled notifications because the other type of notifications which is shown using presentLocalNotification is shown immediately to the user and there isn't any point in cancelling it. The cancelAllLocalNotifications will cancel all the scheduled local notifications and cancelLocalNotifications will cancel notifications which match the provided userInfo object. I think the reason that the second method is also plural maybe confusing but I guess the reason behind the name may be that more than one notification can have the same userInfo object in which case all such notifications will be cancelled. It's a bit different in the case of Android in this package where only one notification will be cancelled which matches the provided notificationId.

The other method clearAllNotifications was supposed to only clear the notifications (both local and remote which is why I omitted local from the name) from the status bar in Android. There is no such method in PushNotificationIOS in react-native at the moment but one can achieve it by setting the applicationBadgeNumber to 0 using method setApplicationBadgeNumber which will clear all the notifications from the notification center for that app.

I reviewed your changes and I think you have now merged the clearAllNotifications and cancelLocalNotifications method into one and one of the operations is performed based whether an id is provided or not. I think that is confusing and also not in line with the behavior in PushNotificationIOS in react-native. I think we should have three exposed methods cancelAllLocalNotifications, cancelLocalNotifications and clearAllNotifications and their counterparts in iOS would be cancelAllLocalNotifications, cancelLocalNotifications and setApplicationBadgeNumber. I think once the docs are updated, it will become clear.

If you want, I can submit a fresh PR with these changes as I don't want you to overload with work.

Regarding the formatting changes, maybe I misunderstood. When I was reviewing your changes, there were a lot of formatting changes and I thought maybe some configuration in your Android Studio is doing that. I also use Android Studio for development. The reason I mentioned Eclipse was because I know there is a feature in Eclipse called Save Actions which can do such thing. But that's not important. Sorry that I mixed it up with the PR comments.

[varungupta85]

@npomfret Regarding the custom, yes I am using that feature daily at least a dozen times as I am still in the process of implementing my app and the notifications are a centerpiece in my app. I set the sound using

sound: 'android.resource://com.xyz/raw/ringtone1',

which plays the ringtone1.mp3 sound present in the android/app/src/main/res/raw folder. I guess it's possible that you may have the package name com.xyz incorrect. This should match the base package name of your app which I have found is same as the package name for MainApplication.java class. I guess you have made the soundName a bit less strict and as long as the sound plays as expected, it's fine.

[npomfret]

@varungupta85 thanks for your reply - will get to the bottom of this soon I'm sure!

After testing the difference between cancelAllLocalNotifications and cancelLocalNotifications (with no parameter) in iOS, it seems that the RN documentation is wrong.

cancelAllLocalNotifications cancels scheduled notifications AND removes existing notifications from the notifications centre. Which is NOT what the current implementation does.

cancelLocalNotifications with no parameters doesn't remove anything from the notification centre, and the optional parameter allows you to pick one or more - but looking at the iOS implementation I don't think it's optional. If you don't specify a parameter nothing happens. And, its poorly documented.

So - I'm going to change the behaviour once again to agree with the observed iOS behaviour, which is not what is documented. Although its confusing it important that the Android and iOS behaviour is as similar as possible. I'll raise a question about the confusing wording in RN about this and if it becomes more clear in the future we'll adapt if necessary. Does that sound ok to you?

What I would like from you if possible, is a PR with a comment about the bit of code in sendNotification that removes stuff from the schedule. And instructions in README on how to use Actions.

Many thanks for your efforts.

[npomfret]

@varungupta85 I've updated the code again after reading though the iOS implementation. I think cancelAllLocalNotifications now does exactly the same - it cancels all scheduled notifications AND removes all alerts from the notification centre.

However, my current implementation of cancelLocalNotifications does not match iOS. It should take a userInfo object which can be used to match one or more scheduled notifications. Do you have any objections to me making this change? I think will achieve feature parity at last.

[npomfret]

@varungupta85 ok, i went ahead and made the change. If I've read the iOS code properly the current implementation of both cancelAllLocalNotifications and cancelLocalNotifications now matches.

Please can you review ?

[varungupta85]

@npomfret I am travelling for a few days and will be back on the weekend. I will review your changes and submit the PR then. I appreciate your efforts in getting it right. Thanks!

[npomfret]

@zo0r I thin this could be pushed to npm now. Had a couple of people do some extra testing and so far it seems ok. Needs a bit more documentation but I thing @varungupta85 can provide that soon.

[npomfret]

@varungupta85 still waiting on some docs from you regarding this PR.

[varungupta85]

@npomfret I am sorry for the delay. Will get them in as soon as possible.

[varungupta85]

@npomfret Sent a PR with the updated docs and comments