This differential implements public methods defined in CommIOSNotifications header. All method implementations apart from "clearNotificationFromNotificationsCenter" are identical with Ashoats fork. Implementation of "clearNotificationFromNotificationsCenter" was altered to match what we
currently do in rescindg code in AppDelegate
Details
- Reviewers
tomek atul ashoat - Commits
- rCOMM9986d601968a: Implement native public notifications API
build the app. Subsequent diffs will use this API and hence will provide comprehensive test plan.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
154 ↗ | (On Diff #20216) | It is also called for remote background notifications that carry key-value pair "backgroundNotifType": "CLEAR" (rescinds). They also carry notificationID to be rescinded. It is not called here but in AppDelegate in further diffs in place where we rescind. |
155 ↗ | (On Diff #20216) | I don't understand this question. This method checks whether completionHandler is null, and calls is only if it is not. Moreover there is no completion handler instance provided with arguments of didReceiveLocalNotification so I am not sure how could we obtain one to pass to this method. Finally in further diffs there is an example of using this method in AppDelegate callback didReceiveRemoteNotification, where completionHandler instance is passed with with callback arguments and passed further to this method. |
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
155 ↗ | (On Diff #20216) | I'm confused why we're defining a method with an optional parameter that we never set. If we never use the completionHandler, can we simply remove it? |
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
155 ↗ | (On Diff #20216) | In https://phab.comm.dev/D6071 we pass completionHandler in AppDelegate in didReceiveRemoteNotification:withCompletionHandler: callback. |
All methods implemented in this differential are copied from Ashoat's fork https://github.com/Ashoat/react-native-notifications/blob/master/RNNotifications/RNNotifications.m unless explicitly annotated otherwise. Methods with different implementations have comments that should explain the difference.
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
164 ↗ | (On Diff #20216) | Implementation here differs from Ashoat's fork. In form completionHandler here was a Obj-C block (can be understood as lambda) that took no arguments. It was called at the beginning of this method it it was not null https://github.com/Ashoat/react-native-notifications/blob/master/RNNotifications/RNNotifications.m#L394. We believed it was incorrect behaviour and our current master repeats implementation of this method with different type of completionHandler that is also called in different place https://github.com/CommE2E/comm/blob/master/native/ios/Comm/AppDelegate.mm#L205. Therefore for the sake of refactoring I changed implementation of this method to take different type of completion handler and call it in different place. The only usages of this method in this diff stack are in D6071 and in didReceiveLocalNotification where completionHandler is nil. So I consider this change to be safe. |
272 ↗ | (On Diff #20216) | Implementation here differs from Ashoats fork. In fork we checked if notification carries "CLEAR" tag (rescind) and did rescinding https://github.com/Ashoat/react-native-notifications/blob/master/RNNotifications/RNNotifications.m#L338. Now we rescind in AppDelegate so the line was removed. |
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
164 ↗ | (On Diff #20216) | This is the tricky part to be honest, apple docs are quite vague here: https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1623013-application?language=objc. We call clearNotificationFromNotificationsCenter:withCompletionHandler: in didReceiveRemoteNotification:fetchCompletionHandler: if remote notification is a rescind. Clearing notification from notification center is some "operation" we do as a reaction to notification. The docs I linked suggest that we call completion handler after we are done processing notification. In AppDelegate we had code that firstly cleared notification from notification center and then called completion handler. In the fork the order was opposite - we first called completion handler and then rescinded. Some time ago I talked to @tomek privately about this. He explained that apple docs suggest to call completion handler after you are done processing notif. Since clearing notification from notification center is our way of processing the notif we must call completion handler afterwards. This was the reason we didn't use the fork to rescind but coded it by hand in AppDelegate. Now that we are replacing the fork with our code I decided to move implementation from AppDelegate here and use it in AppDelegate. |
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
164 ↗ | (On Diff #20216) | Thanks for explaining. I think I did it wrong originally – glad you caught this! |
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
140 ↗ | (On Diff #20216) | Current code on master overrides didReceiveLocalNotification callback in AppDelegate by passing local notification to react-native-noitifications library code - https://github.com/CommE2E/comm/blob/master/native/ios/Comm/AppDelegate.mm#L227. That is why I included functionality related to local notifications. But perhaps I have been shortsighted about this and if we do not use local notifications in our App then I will remove this callback from AppDelegate and related functionality from CommIOSNotifications. |
Please remove UNUserNotificationCenter check before landing
native/ios/Comm/CommIOSNotifications/CommIOSNotifications.mm | ||
---|---|---|
171 ↗ | (On Diff #20690) | We can remove this conditional now, as UNUserNotificationCenter is available as of iOS 10, and we are supporting a minimum of iOS 13 |
- Consequently use dot syntax.
- Remove code related to events that no JS code subscribes to.
- Remove check for [UNNotificationCenter] since we support iOS 13+.