Replace notifications library with using platform APIs directly, on Android

[gnprice]

After some time studying how we're doing notifications, I think the wix/react-native-notifications library isn't doing us a great deal of good, and on Android in particular it's getting in our way more than it's being useful.

Specifically, it's all a bunch of pretty thin wrappers around the underlying Android and GCM APIs. Which could have value, if it meant we got to write JS code that was the same for iOS and Android... but for handling a GCM message that just came in, the RN startup cost would be unacceptable, so we're going to have to write Java code anyway. Plus, the library doesn't even do a good job of making the APIs that we do call from JS actually be similar.

And conversely, it sticks its own set of APIs between us and the platform APIs; and those APIs aren't well documented, sometimes don't make a lot of sense, and don't stay up to date with useful features of the last few years.

So, let's replace the use of this library on Android with just invoking the underlying Android and GCM APIs directly. In particular, let's do this as a prerequisite step before #2691; I think it will make that task quite a bit easier.

PS - We'll want to do the same thing eventually on iOS, but there's no rush. Currently we don't take advantage of any of the features Apple provides for doing something fancier than just displaying each notification one after the other, like we do on Android; so there's very little code of ours on the iOS side. When we do decide it's time to add those features, we'll want to do this as a prerequisite step then, for the same reasons as on Android.

[gnprice]

Since I've just spent some more time studying this code in the course of reviewing #2874, here's a more detailed plan:

1. Replace the use of the library's ProxyService for our "Clear" action on the notification. Instead, just have our own little service.
   • First, just add the new service, declare it in the manifest, and have the dismissIntent point to it instead -- but change as little as possible about the interface, so send it the same bundle with ACTION_NOTIFICATIONS_DISMISS.
   • Then, now that we control that code, we get to clean up how it works! The sad block-comment just above that dismissIntent code of ours gets to go away, because we make the intent look how we want it to, with e.g. dismissIntent.setAction(ACTION_CLEAR), and have our service interpret it appropriately.
   • This means the nasty conditional in getPushNotification gets to disappear, too; the logic in the body moves somewhere reasonable and gets invoked by our proxy's handler in the ACTION_CLEAR case.
2. Take control of the manifest: Pull the library's manifest into our own source manifest, and stop including the one from the library.
   • Something like tools:node="replace" tools:selector="com.wix.reactnativenotifications" on our <manifest> element should do it. See https://developer.android.com/studio/build/manifest-merge .
3. Replace the use of the library's ProxyService for the notification's main action, too.
   • I think we only have to say builder.setContentIntent, after constructing our own Intent, again using Intent#setAction to distinguish.
   • Now we can cut that from the manifest.
4. Replace the library's GcmMessageHandlerService. Just follow upstream GCM docs; this for the manifest blob, and this for the service implementation.
   • Now our getPushNotification can disappear entirely.
5. Replace the library's services for handling GCM Instance IDs.
   • Here, the combination of this library and how we're currently using it is actually quite broken already. We might be better off writing this from scratch.
   • We'd follow the upstream GCM docs: this one and some it links to, notably the example app.
   • For the bits where the GCM docs say to store information in your app, we'll of course want to send it to our JS code. So we'll follow the upstream RN docs for sending an event to JS.
   • Some key conceptual observations from the GCM docs:
     • The app has one Instance ID token. There's no need to keep asking for it.
     • Occasionally the system may decide the token needs to change. This is what InstanceIDListenerService is for, and its onTokenRefresh method; the system will call it in that rare situation. There's no reason for us to call it. (This is one thing the library is pretty confused about.)
     • In our case, after we get the token once and send it to one Zulip server, there may be other Zulip servers later that the user logs into and we want to send the token to. So we should remember the token locally, and do that on login.
6. At this point, there's nothing from the library left that we're still using! We can remove it entirely.

[anoukruhaak]

Hello! My community just started using Zulip and though we love it, we have noticed a host of issues around notifications for mobile. Android seems to be worst (no notifications show up for several users), but iOS also only shows notifications when someone is specifically mentioned (and there's never a red badge shown). I am happy to investigate and find a solution, but the notes above seem to potentially fix a lot. What is the progress on this?

[borisyankov]

@anoukruhaak these problems sound serious but are likely not addressed directly by this issue. We have started working actively on the list above but implementing all of the listed tasks will probably take a few months.

We consider the notifications to be a core functionality and any issue we find and can resolve is of highest priority. So, if let's continue in a separate GitHub issue. We can also continue in our #Mobile stream

[armaanahluwalia]

Another strategy might be to fork a wrapper library and use a hack like this for adding the group notifications feature.

Also want to think about updating to fcm since gcm is being deprecated.

[borisyankov]

We don't even need to fork it. As long as we have confidence in the library and it is actively developed we can contribute code to it.
This option is worth evaluating 👍

[gnprice]

Also want to think about updating to fcm since gcm is being deprecated.

Yeah definitely. I think they've made it a pretty easy upgrade, so we just need to do it sometime.

Another strategy might be to fork a wrapper library and use a hack like this for adding the group notifications feature.

Hmm -- taking someone else's low-quality wrapper library is how we got notifications to the state they're currently in. 😉 If a given library doesn't support notification grouping except by hacking it into a fork, that's a clear sign that it is, at best, not really maintained and hasn't been for several years. (That feature was new in Android 7, IIRC, over two years ago.)

I'm open to considering a third-party library, but it has to be good enough that it's less work for us to get to a high-quality result with it than without it. For this task, the job these libraries are doing isn't hard -- it's really just a matter of reading the upstream docs (for the several different upstreams involved) and writing what they say to write. So the bar is pretty high for how well made one of them would have to be; otherwise we'd end up spending more time studying the upstream APIs while debugging how the library is broken than we would just writing against the same upstream APIs ourselves.