OneSignal-Android-SDK icon indicating copy to clipboard operation
OneSignal-Android-SDK copied to clipboard

Feedback Release 4.0.0: OSRemoteNotificationReceivedHandler cannot be used with own depenencies

Open Kieninger opened this issue 3 years ago • 11 comments

Description: According to the documentation of OneSignal.OSRemoteNotificationReceivedHandler it's not possible to inject any custom dependencies easily (It has to be done in a not so nice way)

Instead of creating that class with reflections from the AndroidManifest metadata like

<meta-data android:name="com.onesignal.NotificationServiceExtension" android:value="com.company.NotificationServiceExtension" />

it would be great, if an instance of OneSignal.OSRemoteNotificationReceivedHandler can be created and set like

OSRemoteNotificationReceivedHandler handler = new MyOSRemoteNotificationReceivedHandler(...SomeDependencies...) OneSignal.setRemoteNotificationReceivedHandler(handler)

Environment Version 4.0.0

Kieninger avatar Dec 09 '20 15:12 Kieninger

Hey @Kieninger, thanks for your feedback. The design of OSRemoteNotificationReceivedHandler was in order to be similar to iOS in the way the SDKs handle the notifications.

Are you using any DI library? For example, on Koin you can have dependencies injected as class variables.

Jeasmine avatar Dec 09 '20 19:12 Jeasmine

Hey, at the moment we go with manual DI (but will sooner or later switch to Hilt or Koin). However this is why I was referring to It has to be done in a not so nice way. We are forced to do it with class variables like

class MyRemoteNotificationReceivedHandler implements OneSignal.OSRemoteNotificationReceivedHandler {

  private static MyDependency myDependency;

  static void setMyDependency(MyDependency myDependency) {...}

  @Override
  public void remoteNotificationReceived(Context context, OSNotificationReceivedEvent notificationReceivedEvent) {
    // Use myDependency
  }
}

In my opinion my propose doesn't change the design of OSRemoteNotificationReceivedHandler. I am only referring to the creation/configuration of that class with the SDK. This is Android specific anyway. It would be just a simpler way of doing it and everything regarding SDK setup would be in one place.

This is not a showstopper at all and I am fine, if you still want to go with the manifest-way.

Kieninger avatar Dec 11 '20 10:12 Kieninger

@Kieninger Thanks for the feedback on this. We will consider adding a setRemoteNotificationReceivedHandler method as you suggested as the NotificationServiceExtension is the only outlier to .set* methods available to add other handlers for events. There are some lifecycle concerns though that need to be thought through before we commit to this though has this is called from a Worker and has a very strict requirement needing to be done from the Application.onCreate to fire in all cases.

jkasten2 avatar Dec 28 '20 16:12 jkasten2

Hi @jkasten2 you had setNotificationReceivedHandler in versions 3.+ Any concerns to restore it?

Lex74 avatar Apr 07 '21 07:04 Lex74

I also would like to have such way of adding handler. We are using Dagger in our app, so it's impossible to inject any dependencies to this class, so it forces me to use some extremely ugly workarounds (like sending received notification to some standalone object that will emit some even that can be observed by any managed class). It's even harder to send back anything onesignal handler so it's possible to decide whether the notification should be swallowed or not.

It's definitely a must feature, as currently it's super hard to use OneSignal for receiving notifications with additional data that cannot be handled as stand-alone notifications with no relation to the rest of the system

AntekM avatar May 11 '21 18:05 AntekM

Any update on this? Which version setNotificationReceivedHandler will be available.

mukeshmaurya91 avatar Jun 29 '21 11:06 mukeshmaurya91

AnyUpdate?

islamarr avatar Jul 04 '21 11:07 islamarr

Upgrading from 3.x to 4.x now, we too had a setNotificationReceivedHandler to grab a piece of the payload when the notification was received.

Didn't see this in the migration guide (maybe it should be updated), but it sounds like we need to move this to the manifest and that it now need to call notificationReceivedEvent.complete(notificationReceivedEvent.notification) when we are done getting the data we want - is this correct?

jt-gilkeson avatar Sep 17 '21 02:09 jt-gilkeson

Hey @all Is anyone facing errors like the below while receiving notifications? V/OneSignal: No class found, not setting up OSRemoteNotificationReceivedHandler

FYI, I have set all the code and functions as per the SDK guide.

I am using the KOIN DI(Dependency Injection) for my application. It would be appreciable if anyone can get me out of this.

MaheshKeshvala avatar Mar 16 '22 16:03 MaheshKeshvala

Update?

EvangelosG avatar Apr 18 '22 20:04 EvangelosG

Two year have passed and still there isn't a nice way to inject dependencies with hilt in OneSignal

buntupana avatar Dec 23 '22 10:12 buntupana