Page MenuHomePhabricator

Send broadcast with new device token from CommNotificationsHandler to CommAndroidNotificationsEventEmitter broadcast receiver and forward it fo JavaScript where it is handled as previously
ClosedPublic

Authored by marcin on Jan 16 2023, 5:35 AM.
Tags
None
Referenced Files
F3486749: D6272.diff
Wed, Dec 18, 5:56 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:51 AM
Unknown Object (File)
Sat, Dec 14, 3:50 AM
Subscribers

Details

Summary

This differential overrides onNewToken method of FirebaseMessagineService to send broadcast to CommAndroidNotificationsEventEmitter with new token. CommAndroidNotificationsEventEmitter receives broadcast with token and send event to JavaScript. On the Js side we register to listen to
this type of event and react to it in the same way we used to.

Test Plan

Build the app. Notifications should be delivered, since the device token should be correctly received.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 16 2023, 5:39 AM
Harbormaster failed remote builds in B15380: Diff 20972!
tomek requested changes to this revision.Jan 18 2023, 8:53 AM
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java
84 ↗(On Diff #20980)

Should we save the broadcast manager as an instance variable just like we do e.g. with notification manager?

native/push/android.js
47 ↗(On Diff #20980)

Why do we use optional string?

native/push/push-handler.react.js
151 ↗(On Diff #20980)

Is it the only place where this const is used?

This revision now requires changes to proceed.Jan 18 2023, 8:53 AM
native/push/android.js
47 ↗(On Diff #20980)

This is a mistake - token returned from Java and sent to JavaScript as an event is never null. Is should be string.

native/push/push-handler.react.js
151 ↗(On Diff #20980)

Currently, this is the only place this variable is used. Is it a mistake?

Update typing
Save LocalBroadcastManager instance

tomek added inline comments.
native/push/push-handler.react.js
151 ↗(On Diff #20980)

It seems like the same string is used in D6271. It would be great if we could share the same const for it (but if that's too complicated, we can leave it as it is).

This revision is now accepted and ready to land.Jan 26 2023, 1:40 AM
native/push/push-handler.react.js
151 ↗(On Diff #20980)

Oh, you were talking about "commAndroidNotificationsToken" string not const commAndroidNotificationsEventEmitter. Well, it might be messy to introduce change in this (and other differentials that subscribe to events), but adding a differential on top of the stack that adds those strings to getConstants and replaces usages will be straight forward. We already did so for iOS.

native/push/push-handler.react.js
151 ↗(On Diff #20980)

This diff stack is already growing, so I will wait with implementing this feature until it is landed. I also noticed that we actually didn't implement this feature on iOS. I will create a follow up task to replace event names strings with constants exported from native modules. It should be straightforward - entire API is set, we just need to put more data in dictionaries and use them in JavaScript.