Details

Reviewers

tomek
bartek

Commits

rCOMMbaa9ec05e92a: Implement native method to create notification channel

Summary

This differential implements native method to create android notification channel, updates JavaScript type and uses it JavaSCript code.

Test Plan

Since API level 26 all notifications must be assigned to a channel: https://developer.android.com/develop/ui/views/notifications/channels, I presume that simply ensuriing notifications are
delivered both in the foreground and background is enough to test this differential.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Jan 25 2023, 12:43 AM

Herald added subscribers: atul, ashoat. · View Herald TranscriptJan 25 2023, 12:43 AM

marcin added a parent revision: D6366: Implement native method to get initial notification and use it in JavaScript.Jan 25 2023, 12:43 AM

Harbormaster completed remote builds in B15616: Diff 21290.Jan 25 2023, 12:56 AM

marcin requested review of this revision.Jan 25 2023, 12:56 AM

marcin added a child revision: D6378: Implement native method to update badge count and use it in JavaScript.Jan 25 2023, 12:57 AM

ashoat added inline comments.Jan 25 2023, 4:53 AM

native/push/android.js
11 ↗	(On Diff #21290)	The object returned here should be `$ReadOnly`

tomek accepted this revision.Jan 26 2023, 2:14 AM

tomek added inline comments.

native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
69 ↗	(On Diff #21290)	What would happen for older clients? Does the fact that channels can't be created will be somehow detectable by the user?
80–83 ↗	(On Diff #21290)	I'd try to use `Map.of` but I guess this is how it's usually done
native/push/push-handler.react.js
144 ↗	(On Diff #21290)	Can we keep the description? Does it matter?

This revision is now accepted and ready to land.Jan 26 2023, 2:14 AM

marcin added inline comments.Jan 26 2023, 4:32 AM

native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
69 ↗	(On Diff #21290)	Looking at the Figure no.1 here: https://developer.android.com/develop/ui/views/notifications/channels we can conclude that channels are intended to create various types of notifications that can have different settings - like whether they make sound or just pop on the screen etc. User can then modify settings for whichever channel they want - for instance they can block notifications from one channel while other channels of the same app are delivering notifications. Also when user grants or removes privilege for a channel our app can't modify those settings. It looks like it is a significant UI feature of newer android versions. Older clients will still have notifications functionality but without channels - so they turn off/on all notifications for an application at once. Important note: In `CommNotificationsHandler` we set channel ID for a notification without checking which build version we are running. According to docs: https://developer.android.com/reference/android/app/Notification.Builder#setChannelId(java.lang.String) it is not an error since it is a no-op (if running API < 26).
80–83 ↗	(On Diff #21290)	This is how react-native guides wrote this code snippet: https://reactnative.dev/docs/native-modules-android#exporting-constants
native/push/push-handler.react.js
144 ↗	(On Diff #21290)	I will bring it back. It is not fatal mistake, but it is description that appears under channel's name in application notification settings. I will bring it back, by extending arguments list in this native function.