This changes our notification handling so that event about received notifications is sent to JS when the app is backgrounded as well.
Details
Details
- Build the app prior to applying this diff.
- Background it, send a notif.
- Kill keyserver.
- Open the app.
Message is not visible in the thread.
- Repeat the steps above after applying this diff.
Message is visible in the thread even though keyserver is down.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-3365
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
This was easier than I realized!! Thanks for renaming everything. Will leave the diff on others' queues to get some other perspectives
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 115–117 ↗ | (On Diff #24190) | What happens if the Android app is killed, then receives a notif and starts again. Will the JS thread be started? Will this Intent be triggered even if the JS thread is not running? I think ideally, we would avoid starting the JS thread in that scenario. We only need to inform the JS thread about the message if the JS thread is already running; otherwise, when it starts it will pull the latest data from SQLite |
| native/push/push-handler.react.js | ||
| 533 ↗ | (On Diff #24190) | That's interesting – so currently, this would get called if the app receives a message while in the foreground? The name of onPushNotifBootsApp implies it should not get called if the app has been in the foreground recently, but I guess it works today... |
Comment Actions
This looks good to me, but full disclosure I have limited familiarity with this side of the codebase... so please keep that in mind before landing (aka maybe consider having someone else take a final look)
| native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java | ||
|---|---|---|
| 115–117 ↗ | (On Diff #24190) | This intent doesn't have capability of starting JS thread. When JS thread starts it registers (in CommAndroidNotificationsEventEmitter) relevant broadcast receiver that can receive this intent. However if there are not broadcast receivers registered this line is a no op. For sure there must be a way to check if JS thread is already running in native Java, but my intuition is that it is not worth the effort considering high priority of this issue. |
| native/push/push-handler.react.js | ||
| 533 ↗ | (On Diff #24190) | Method onPushNotifBootsApp sets detectUnsupervisedBackground field of RootContext to false. The last differential that altered this field was: https://phab.comm.dev/D928 (which isn't particularly insightful). If we aren't 100% sure whether it is ok to call onPushNotifBootsApp in case when app wasn't actually booted by notification we can easily introduce additional event type that would just trigger JS to call saveMessageInfos and nothing else. |