[Android] Fix cancelLocalNotifications

[superandrew213]

Fixes 259, 368, 338

[southerneer]

Cheers, this is definitely an improvement. It would be nice to eventually have parity between iOS and Android in the canceling interface, but this works for now.

[npomfret]

Does anyone know why the matches method isn't working? @varungupta85 i think you added this, have you any idea what the issue might be?

[superandrew213]

@npomfret because it was matching all object keys. We should only be matching ids since you are only supplying the id to cancel a notification.

[npomfret]

I can't see this in the code, i looks right to me. Its looping over the keys in the userInfo object passed in. If they all match then we've found the notification to cancel - which is the same behaviour as iOS. No?

[superandrew213]

@npomfret iOS uses the userInfo object to find a match, however userInfo is not available on Android. See here and here

Is the cancelLocalNotification feature working for you with the current implementation on Android? I don't think it has ever worked.

The objects will never match since you are comparing all the notification attributes { id: 123, message: "Hello", etc ...} with {id: 123}.

[npomfret]

This iOS UserInfo object is basically just a dictionary. Below the iOS implementation of cancelLocalNotifications, I can't see where it's different to the java implementation we have here. As far as I can see the algorithm is to loop through the keys of the passed in userInfo object, and check the values in each local notification object to see if they are all the same. If they are all the same then remove it.

  for (UILocalNotification *notification in [UIApplication sharedApplication].scheduledLocalNotifications) {
    __block BOOL matchesAll = YES;
    NSDictionary<NSString *, id> *notificationInfo = notification.userInfo;
    // Note: we do this with a loop instead of just `isEqualToDictionary:`
    // because we only require that all specified userInfo values match the
    // notificationInfo values - notificationInfo may contain additional values
    // which we don't care about.
    [userInfo enumerateKeysAndObjectsUsingBlock:^(NSString *key, id obj, BOOL *stop) {
      if (![notificationInfo[key] isEqual:obj]) {
        matchesAll = NO;
        *stop = YES;
      }
    }];
    if (matchesAll) {
      [[UIApplication sharedApplication] cancelLocalNotification:notification];
    }
  }

[superandrew213]

@npomfret there is nothing wrong with the match method. There is no userInfo object available on the Android implementation so how can you match userInfo objects? See here and here

[npomfret]

The userInfo object is just represented as a map in android (and iOS too - it's not a class, it's just a dictionary). So from your js you call something like:

cancelLocalNotifications({id: '1234'})

Which get converted into a java ReadableMap object with one key and one value in Android, and a dictionary object in iOS. From there the logic looks the same to me, so I don't understand why it isn't working.

Perhaps it's a type problem. Are you passing a string for your id in when it should be an int, or vicer-versa?

[superandrew213]

It should be a string and I am passing a string. Ok so this {id: '1234'} ReadableMap object what does it get compared to?

[npomfret]

I've got to suggest you take a look at the code, there's not much to it.

The userInfo object get's passed in here. And then it's compared to the each stored notification using the RNPushNotificationAttributes object which has the crucial matches method here.

Basically, each stored notification is treated like as a map. Each key of the passed-in userInfo map is compared to the notification map... and if they all match - bingo. Extra fields in the underlying notification should be ignored.

Have you tried stepping through in the Android Studio debugger to see if the match method is being called and why it's not matching your parameters?

[superandrew213]

Ok so {id: '1234'} is being matched against each stored notification object? Right? So it will only cancel a notification if it finds a notification object that is equal to {id: '1234'}.

However if it finds this notification object {id: '1234', message: 'Hello', etc..} then it won't cancel the notification since not all the keys match. That's the issue we have.

[npomfret]

Extra keys in the underlying notification should not used in the comparison, same as with the iOS implementation. And as far as I can see they are not. See for yourself: https://github.com/zo0r/react-native-push-notification/blob/master/android/src/main/java/com/dieam/reactnativepushnotification/modules/RNPushNotificationAttributes.java#L137

[superandrew213]

Can you confirm that it works on your end and that you can cancel a notification? My PR is essentially just doing this. So I'm not sure why the match method doesn't work for me.

[npomfret]

Aright, I think i understand why it isn't working. The confusion is with the name - cancelLocalNotifications does not (and is not supposed to) remove notifications from the notification centre. As I understand it the cancelLocalNotifications method only cancels scheduled notifications (on both iOS and android), ie. notifications that are persisted, and fire at some point in the future. And I believe it does work as intended.

The method that I think you want, which does not exist (yet). And perhaps could/should be added as an android only method, something like:

    public void clearNotification(String id) {
        NotificationManager notificationManager = notificationManager();
        notificationManager.cancel(Integer.parseInt(id));
    }

This will remove an individual alert from the notification centre for android only. I don't see why this can't be added so long as it's clear in the docs that it isn't supported in iOS.

[superandrew213]

As I understand it the cancelLocalNotifications method only cancels scheduled notifications (on both iOS and android), ie. notifications that are persisted, and fire at some point in the future.

This is what I am after. But it's ok. If it works on your end as it should then we can close this PR. I'll continue using my fork for now until I'll review this again when I'll upgrade.

[npomfret]

Closing. Lets add an android only clearLocalNotification method

[Shhzdmrz]

@npomfret are you going to add this clearLocalNotification ? because cancelLocalNotifications never worked.

[npomfret]

I'm afraid I don't have time right now. You could submit a PR if you like?