invertase / react-native-notifee

Moved to https://github.com/invertase/notifee
https://invertase.io/blog/open-sourcing-notifee
Other
466 stars 31 forks source link

Android channels notification sounds not playing after app update #341

Closed lucadegasperi closed 3 years ago

lucadegasperi commented 3 years ago

Hello, A lot of our users are having problems with our notifications not playing any sounds after they download a new version of our app from the play store. When our app boots we create the notification channels as follows:

await notifee.createChannels([
        {
            id: "notification",
            name: I18n.t("messages"),
            importance: AndroidImportance.HIGH,
            sound: "notification",
            lights: true,
            vibration: true
        }
]);

While trying to debug the issue I've noticed in the android settings of our app that instead of the sound named "notification" we had a random number like "123456789". This got me thinking that maybe that ID changes when the app gets rebuilt but since Notification channels are mostly read-only once created, that ID is no longer the actual sound ID, thus playing no sound when a notification is received after the app has been updated. I then stumbled across this article and It looks like he has a very similar issue to what we are experiencing. I was wondering, does Notifee create the notification channels in a similar way to what the article is reporting? Is the problem somewhere else?

Thank you again for your great work!

mikehardy commented 3 years ago

@lucadegasperi I think you have just won my internet points for the day, though you may not want them since this looks like a real issue. Your hunch and that article look like a real problem in the library that you have exposed with clear detail.

Behold the way the library constructs the sound URI for notification channel creation:


  public static @Nullable Uri getSoundUri(String sound) {
    Context context = ContextHolder.getApplicationContext();
    if (sound == null) {
      return null;
    } else if (sound.contains("://")) {
      return Uri.parse(sound);
    } else if (sound.equalsIgnoreCase("default")) {
      return RingtoneManager.getDefaultUri(RingtoneManager.TYPE_NOTIFICATION);
    } else {
      int soundResourceId = getResourceIdByName(sound, "raw");
      if (soundResourceId == 0 && sound.contains(".")) {
        soundResourceId = getResourceIdByName(sound.substring(0, sound.lastIndexOf('.')), "raw");
      }

      if (soundResourceId == 0) {
        return null;
      }

      return Uri.parse("android.resource://" + context.getPackageName() + "/" + soundResourceId);
    }
  }

Looks to me like we are using resource IDs, and I'm familiar with how gradle handles resources so this does look like a clear problem to me.

It appears:

I'd like help confirming I'm on the right path. Does that match your understanding?

lucadegasperi commented 3 years ago

@mikehardy it indeed appears that the sound URI should be referenced by name because a gradle clean might change a resource's ID. A note in the changelog on how to upgrade is definitively needed. This behavior should probably be reflected for phones with android < 8 if it applies.

On a side note: I've noticed that some android manufacturers allow for the user to change a channel sound, but then there's no way to revert the sound to the one the developer intended to use. Is the only workaround to this to allow the user to create new channels inside the app and delete the old ones?

If you need a userbase to test these changes I'm more than happy to help you!

mikehardy commented 3 years ago

The user ability to change the sound on a channel (which I can do in stock AOSP on a Google nexus emulator) is one of the specific goals of the system granting user control over the channels. If you channel recreate to override the chose, it seems you are specifically going against a user choice which could be considered an antipattern

Doing it once to fix a library bug, that's different though I think.

On Android 7 and lower the sound reference is not persistent across app builds so even a changing R.id value would not be a problem. That said the fix here will apply to all references of sound files

I'm examining this for now with intention to have a fix, your willingness to help test is greatly appreciated! We can usually generate a test branch alpha release for the purpose and that may be the path here

mikehardy commented 3 years ago

Maybe relevant to your observation on user choice of sound, added in API30 you can see whether the user has done so at the native level, though it's not exposed in the JS API here - https://developer.android.com/reference/android/app/NotificationChannel#hasUserSetSound()

Note that there is some other information available in the theme of "interrogating app notification channels for current status" that the library does not expose, canBubble/canBypassDnd/canShowBadge - and some new "conversation" related features - a lot of new APIs in this area in the underlying system.

lucadegasperi commented 3 years ago

@mikehardy we already have a group of users on an alpha release which all have issues with the notifications. My main concern with the users changing the notification sound is not that the user is free to do so, but that he cannot revert back to the channel's original sound if he later regrets the choice. I've tried it myself and there's no settings on Android to do so. In that case we should offer the user a way to recreate the channels without resorting to them uninstalling/reinstalling the app.

mikehardy commented 3 years ago

Interesting - I think I understand what you mean. I just went through the settings on notification channels on my API30 emulator and I see no support for automating returning to the original sound. The only possible path would be a user going to the channel settings, going to set the sound again, choosing "my sounds", then adding a file manually. It's possible you could offer a way to export your custom sound, so it was at least available but this is not a supportable process.

The only other way I can imagine would be to offer a way to "reset channels to default" but this would involve a delete / recreate, and if I understand correctly, an attempt to delete/recreate with the same name will just resurrect the channel (otherwise this could be abused to attempt to reset important / unmute, right?). So this would have to be an app level process where you "remembered" the name (async storage?) of the channel, and somehow altered it (incremented a counter used as a suffix?) so you had a new name when "recreating"/"resetting to defaults". That's a rough sketch and pretty ugly but I do not see any other way besides documentation + exposing the sound for user export + manual sound library addition + manual channel sound config.

I've got the reported issue here though (sound by resource id vs sound by name) dissected in my editor and I'm working through the code change now

mikehardy commented 3 years ago

I've got PRs pending internal on core module and here for an added type, to fix this

mars-lan commented 3 years ago

users with custom sounds need a clear and direct call to action in the changelog / release announcement directing users of the feature to create a new notification channel (with a new ID! so they really get a new channel) so they may experience reliable sound file reference even after rebuilds

The release note didn't mention that a new notification channel ID is needed.

mars-lan commented 3 years ago

Also based on my early testing it seems that #342 actually broke custom notification sounds on physical devcies (tested on Galaxy S9 & Pixel 3). Will follow up with more details afte more testing & debugging.

mikehardy commented 3 years ago

I'm just catching up with everything after being offline all weekend (a rare event!) and I noticed the release notes did not mention it either. I'm very very interested @mars-lan in any insight you have with regard to any collateral damage in this fix. It may be that it's unrelated to this but related more the other notification channel work. They don't actually interact but they are in the same area. The fix I put in here was very targeted and was 90% test-related code and only 10% change to real code so if I got it wrong I'm going to be surprised (but also at least have the test harness set up to close whatever thing was left undone). Please feel free to @ me, I do not want any regression unhandled longer than necessary, and thanks in advance!

mikehardy commented 3 years ago

quick follow-up: release note has been updated - publish of same in process (but not out yet)

mars-lan commented 3 years ago

Actually looks like I was hitting a different bug: https://github.com/notifee/react-native-notifee/issues/349.

mars-lan commented 3 years ago

Found the root cause of sound not playing. Previously the custom sound should include the file extension as shown in the doc, e.g. custom_sound.mp3. Starting 1.10.0 the extension shouldn't be included, e.g. custom_sound. Please update the documentation & releate note to reflect that.

mikehardy commented 3 years ago

@mars-lan I'm not sure that was an intentional change - I'm going to peel your issue off to a new separate thing - the previous issue was very focused on an android-specific resources identifier vs name issue, the new (possibly follow-on) thing you report is now just down to the name itself