Page MenuHomePhabricator

Implement native method to create notification channel
ClosedPublic

Authored by marcin on Jan 25 2023, 12:43 AM.
Tags
None
Referenced Files
F2023986: D6377.id21572.diff
Mon, Jun 17, 4:09 AM
Unknown Object (File)
Fri, Jun 14, 12:27 AM
Unknown Object (File)
Thu, Jun 13, 10:58 PM
Unknown Object (File)
Sat, Jun 1, 8:47 AM
Unknown Object (File)
Sat, Jun 1, 8:47 AM
Unknown Object (File)
Sat, Jun 1, 8:47 AM
Unknown Object (File)
Sat, Jun 1, 8:47 AM
Unknown Object (File)
Sat, Jun 1, 8:47 AM
Subscribers

Details

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
Branch
marcin/eng-2501
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

native/push/android.js
11

The object returned here should be $ReadOnly

tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
69

What would happen for older clients? Does the fact that channels can't be created will be somehow detectable by the user?

80–83

I'd try to use Map.of but I guess this is how it's usually done

native/push/push-handler.react.js
144

Can we keep the description? Does it matter?

This revision is now accepted and ready to land.Jan 26 2023, 2:14 AM
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
69

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

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

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.

native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
80–83

It's usually done this way because Map.of is Java 9+ but RN targeted Java 8

Bring back possibility to set channel description

Make constants keys read-only

tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotifications.java
69

Great, that makes sense!

native/push/android.js
14 ↗(On Diff #21369)

I would say this is an optional prop

Brindg back channel name to Default, Default_1 was artifact from debugging I forgot to remove