This differential implements notification formatting and displaying in Java so that it can be removed from JavaScript. This allows us to remove headless JS task.
Details
Details
Build the app. Kill it. Send messages and rescinds. Notifications should appear in and disappear from NotificationCenter as they did prior to changes introduced in this differential.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 118 ↗ | (On Diff #20793) | This may be hard to grasp at first so let me explain the reason for this change. If application is in the foreground super.onMessageReceived(message); sends event to JavaScript, that we listen to and handle by displaying InAppNotification. If application is in the foreground, it starts Headless JS Task named RNFirebaseBackgroundMessage that does exactly the same thing that we now to in displayNotification method. We haven't implement machinery to send event to JS from CommNotificationsHandler (it is the matter of future differentials in this stack) yet so we differentiate here whether to call super... or not based of application state. Once we implement code to send events to JS from this service we will get rid of RNFirebaseMessagingService and send relevant event to JavaScript in this if branch. |
Comment Actions
Scope of this differential might appear to be broad for reviewers, but I consider it beneficial that changes in CommNotificationsHandler and android.js are included together since added Java code in displayNotifications mirrors deleted JavaScript code in android.js
| native/android/app/src/main/java/app/comm/android/MainApplication.java | ||
|---|---|---|
| 31 ↗ | (On Diff #20793) | When I sent messages to killed app, it was started to process notifications in CommNotificationsHandler. CommNotificationsHandler uses GlobalDBSingleton to persist notifications in SQLite. This requires some fbjni API calls. Without this change I was constantly seeing ClassNotFoundException for facebook::jni::ThreadScope::WithClassLoader. This change solved this issue since fbjni library is now present, when application is launched to process notification with CommNotificationsHandler. |
| native/android/app/src/main/java/app/comm/android/MainApplication.java | ||
|---|---|---|
| 31 ↗ | (On Diff #20793) | Why it isn't necessary to load this library by hand when GlobalDBSingleton is called from the app? |
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
| 153–157 | Is it a standard method of checking if the app is in foreground? E.g. https://medium.com/the-devops-corner/how-to-detect-android-app-foreground-status-c9443ddef260 discusses the issue in depth. It presents a slightly extended version of this function, but warns that:
We have a module AndroidLifecycleModule - maybe that could be useful here? | |
| 186–188 | It doesn't seem necessary to decode repeatedly a static resource for each notification. Maybe we can do it only once? | |
| 118 ↗ | (On Diff #20793) | Can we invert the code a bit to follow the existing flow of special cases first and calling super.onMessageReceived as the last operation? if (!this.isAppInForeground()) {
this.displayNotification(message);
return;
}
super.onMessageReceived(message);As a side note, this comment was a little confusing to read. E.g. If application is in the foreground appears twice in a row and figuring out if it was intentional or by mistake makes it harder to comprehend. |
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 153–157 | I talked to @marcin about this today. I don't think we should block the monthly goal on resolving this, but also I think it might be fairly easy to resolve. We can either try to address this in a follow-up task, or we can try to address it now if @marcin thinks there is time. Looking at the old AndroidLifecycleModule code in Java, it seems like we could replace this check with: return ProcessLifecycleOwner.get().getLifecycle().getCurrentState() == Lifecycle.State.RESUMED; When initializing the currentState in the only AndroidLifecycleModule, that was the condition we checked. However, that may only be valid at the start of the application lifecycle... in AndroidLifecycleModule we only check that value at the start, and after that we use an observer to update the value. Since we'll be checking this valid at times other than the start of the app, if we use this code it would be helpful to do a little bit of testing and research to see if it's correct. We should consider all of the possible return values of getCurrentState(). Separately, it may be possible to try to access the new Expo module version of AndroidLifecycleModule (which is written in Kotlin now), but in the past I have had trouble trying to access Expo modules from native code. @bartek may have some pointers here, but my instinct is that it would be simpler to just call the Java method I mentioned above. | |
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 153–157 | I read those docs:
It suggests that using the class that @ashoat Linked is a good idea, but it is better to use it in a slightly different way. Instead of directly querying for state we register observer which callback are called when application state changes. I tested this approach and it worked. | |
Comment Actions
- Use solution based on application lifecycle to check whether it is in the foreground.
- Save Large Icon bitmap as static variable
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 153–157 | How is this better than just calling getCurrentState()? Seems very roundabout for a "spot check"... | |
Comment Actions
Accepting, but we should probably replace the observer with calling getCurrentState on every notif.
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 153–157 | @ashoat are you referring to ProcessLifecycleOwner.get().getLifecycle().addObserver( solution? I think they work the same, but calling getCurrentState every time we get a notification seems to be more maintainable as we avoid making the class more stateful. The observer might be more performant but I don't think the difference is measurable in any scenario. | |
| 118 ↗ | (On Diff #20793) | This changes later in the stack so probably not worth updating. |
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 153–157 | I agree that calling getCurrentState is better solution. The reason I didn't opt for it is that looking at documentation of androidx.lifecycle package: https://developer.android.com/reference/androidx/lifecycle/package-summary Lifecycle class is not mentioned in the description. Moreover if we click on it (in other class documentation) we are directed to non-existing page: https://developer.android.com/reference/androidx/lifecycle/Lifecycle. Therefore I was unable to examine its API and methods. Also other android docs I linked in previous comment elaborate on getting info about app state via observers while they don't mention getCurrentState. It was really confusing to me so I decided to follow observer approach. I didn't try to compile getCurrentState approach, but I will do so shortly to check if it works as expected. getCurrentState approach is definitely shorter and more maintainable, but this documentation is a little bit weird about Lifecycle class. | |
Comment Actions
Maybe using the lifecycle observer isn't the most straightforward solution, but it's reliable and tested. It's already used in AndroidLifecycleModule and it's proven to work.
Comment Actions
Actually in AndroidLifecycleModule we are calling getCurrentState and we are using the same androidx.lifecycle version as we do in here. I will check if it works and if it does I will replace observer with this method.