OneSignal / OneSignal-Android-SDK

OneSignal is a free push notification service for mobile apps. This plugin makes it easy to integrate your native Android or Amazon app with OneSignal. https://onesignal.com
Other
594 stars 368 forks source link

[question | Android]: MessagingStyle notifications update pattern #2114

Closed CalamityDeadshot closed 2 weeks ago

CalamityDeadshot commented 1 month ago

How can we help?

Is there, or are there plans to implement, developer control over notification IDs?
Our use-case is to display MessagingStyle conversation notifications, which requires:

  1. Somehow finding the previous notification from the same conversation;
  2. Copying its style and associated messages via NotificationCompat.MessagingStyle.extractMessagingStyleFromNotification(), or creating a new one if none has been found;
  3. Applying this style to the notification and adding the newest message to it;
  4. Removing the previous notification;
  5. Posting the new one, with the most up-to-date conversation.

At first we tried removing the previous notification via:

previousNotification?.id?.let {
    OneSignal.Notifications.removeNotification(it)
}

But it was very visible in the UI - one group of messages would disappear, and an almost identical one, apart from a new message, would appear a perceivable amount of time later. We couldn't find a way to mutate androidNotificationId using INotificationServiceExtension, so we tried making use of the collaspe_id prop, setting it equal to the chat id. Works as indented on Android - no visible "remove-add" routines were present, although androidNotificationId would still be something random, e.g. -1518917439. But this solution completely broke iOS - every "new message" notification would replace the previous one, as they are not grouped the same way.
This problem is fixed by either somehow manually setting platform notification IDs equal to chatId from INotificationServiceExtension Android-side (which appears to be impossible), so the system correctly replaces them, or somehow ignoring collapse_id iOS-side (which also appears impossible).

Does OneSignal allow for such use-cases? Could you recommend an established pattern, if there is one?
Thanks.

Code of Conduct

jkasten2 commented 1 month ago

@CalamityDeadshot If you want to ensure the device only has one notification per your chatId I would recommend collaspe_id. The SDK / device will handle this for your without needing any extra code.

so we tried making use of the collaspe_id prop, setting it equal to the chat id. Works as indented on Android - no visible "remove-add" routines were present, although androidNotificationId would still be something random, e.g. -1518917439. But this solution completely broke iOS - every "new message" notification would replace the previous one, as they are not grouped the same way.

collaspe_id should behave the same on both Android and iOS, can you explain the issue you are seeing in more detail?

CalamityDeadshot commented 1 month ago

@jkasten2

collaspe_id should behave the same on both Android and iOS, can you explain the issue you are seeing in more detail?

A single Notification object on Android holds more than one message (platform Conversations behaviour), whereas on iOS the mapping is 1:1 - one message per one notification. If we use collapse_id, this necessarily means that each new message would be visually added to an existing notification on Android provided we implement the required logic, but on iOS each new message would replace the previous one, so only the most recent message would be visible.
collapse_id works as intended in both cases, the problem is platform differences that seem unadressable without some manual control over notification IDs on Android.

jkasten2 commented 1 month ago

@CalamityDeadshot I see, just to confirm on Android you plan to use MessagingStyle(user).addMessage to combine the contains of each notification that is received into on. Where iOS communication notifications doesn't have such a concept. Each message received has to be its own notifications.

One way you could do this is to not use collapse_id, so you get the iOS behavior you want. Then for Android if you want to use addMessage you will need to do some tracking yourself. In your INotificationServiceExtension.onNotificationReceived when the first notification is received, store the event.notification().androidNotificationId() value somewhere on disk. Then when another notification is received with the same chatId look up the androidNotificationId you stored. You can then use this to update the existing Android notification and use addMessage to add to it. You will then need to call event.preventDefault(true) to prevent the OneSignal SDK from showing a 2nd notification.

CalamityDeadshot commented 1 month ago

@jkasten2 Thank you for the suggestion. It works in theory, but I can't get it to work in practice.
I'm calling INotificationReceivedEvent.preventDefault(true) from INotificationServiceExtension$onNotificationReceived, but notifications are still shown. I double-checked that SDK acknowledges the call. NotificationReceivedEvent logs it:

    override fun preventDefault(discard: Boolean) {
        Logging.debug("NotificationReceivedEvent.preventDefault()")
        isPreventDefault = true
        this.discard = discard
    }

https://github.com/OneSignal/OneSignal-Android-SDK/blob/main/OneSignalSDK/onesignal/notifications/src/main/java/com/onesignal/notifications/internal/NotificationReceivedEvent.kt#L19 And I see this log in my logcat.

[DefaultDispatcher-worker-14] NotificationReceivedEvent.preventDefault()

Is there a particular place I must call it from, or is it a bug?
I'm using com.onesignal:OneSignal:5.1.15

jkasten2 commented 1 month ago

@CalamityDeadshot I tested this out and I am not able to reproduce an issue. I also reviewed the SDK code around this feature and didn't find any potential issues either.

Can you ensure you are calling event.preventDefault(true); before getting to the bottom of your onNotificationReceived method?

public class NotificationServiceExtension implements INotificationServiceExtension {

   @Override
   public void onNotificationReceived(INotificationReceivedEvent event) {
      Log.v("OneSignalTest", "TOP onNotificationReceived");
      event.preventDefault(true);
      Log.v("OneSignalTest", "BOTTOM onNotificationReceived");
   }
}
CalamityDeadshot commented 3 weeks ago

@jkasten2 You are right, it was my mistake. I called it from setExtender lambda, which is obviously called after onNotificationReceived finished. It works as intended now, thank you.
I will edit the title of this question to better reflect its discussion for future readers.