Call to the registerForRemoteNotifications

[maratal]

According to the issue, a call to the registerForRemoteNotifications: should be included somewhere to simplify the initial setup process for the cocoa framework. But the main "headache" and a startup "noise" is this:

[[UNUserNotificationCenter currentNotificationCenter] requestAuthorizationWithOptions:options completionHandler:^(BOOL granted, NSError * _Nullable error) {
    if (granted) {
        dispatch_async(dispatch_get_main_queue(), ^{
            [[UIApplication sharedApplication] registerForRemoteNotifications];
        });
    } else if (error) {
        [rest.logger error:@"ARTPush: request authorization failed (%@)", [error localizedDescription]];
    }
}];

So, I've introduced a new activate: method with the UNAuthorizationOptions parameter to justify that, whereas its old counterpart optionless activate method calls the new one with the default set of options.

This has raised a concern of a possible interfere of the new activate method behavior with existing apps startup logic, which is true and further discussion about it is here.

In addition to that, I've created a new handler for application:didRegisterForRemoteNotificationsWithDeviceToken which is rather than being "handler" is now a method for saving on a device and registering the new token within Ably infrastructure: registerAPNSDeviceToken:, signature of which much easier to comprehend in oppose to didFailToRegisterForRemoteNotificationsWithError:rest:.

The new one just calls the old one, because it already implements what is needed. In the following commits I'm going to flip their bodies, because the old handlers should eventually be removed.

These two changes allow to setup ably-cocoa and APNs in just two lines of code:

self.realtime.push.activate([.badges, .alert])

func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data) {
    self.realtime.push.registerAPNSDeviceToken(deviceToken)
}

Which is way more welcoming in comparing to Step 6 ad Step 7 of the tutorial https://ably.com/tutorials/ios-push-notifications#step6-register-device-for-push

[QuintinWillison]

I have a couple of questions around the changes being made to the activate and deactivate methods:

1. Is there a risk that calls to these methods, or other methods altering state, could overlap one another? I'm not familiar with this code, so there may be external factors guaranteeing this, however I can't documentation indicating that (for example: these methods aren't safe to be called from any thread, or can only be called in very specific circumstances).
2. Perhaps related, do methods on the shared application instance have to be called from the main thread? Looking at this code comment it appears that calls to the delegate might be.

[maratal]

Thanks @QuintinWillison, that's a good spot. Regarding overlapping, I've created this issue - ably/ably-flutter#345, so registerForRemoteNotifications wouldn't be called twice. And regarding thread safety, according to this SO post - https://stackoverflow.com/questions/44391367/uiapplication-registerforremotenotifications-must-be-called-from-main-thread-o one should re-assure this call is made from the main thread.

[lawrence-forooghian]

Regarding overlapping, I've created this issue - ably/ably-flutter#345, so registerForRemoteNotifications wouldn't be called twice

I think that we can’t avoid registerForRemoteNotifications being called twice. As I've mentioned in #1271 (and given examples from our own example code), all current users of our SDK who are using push notifications are currently calling registerForRemoteNotifications. Even if we were to mention in our release notes (and we possibly should) "Ably no longer requires that you call registerForRemoteNotifications yourself", we can't guarantee that people would see / pay attention to it.

In terms of consequences of registerForRemoteNotifications being called twice, I think that it will lead to a GotPushDeviceDetails event being emitted twice, meaning an extraneous PATCH /push/deviceRegistrations/:deviceId request being made (RSH3d3b). I think this is acceptable, though?

[lawrence-forooghian]

@QuintinWillison @maratal as mentioned in #1271, we need to remove the step where the user calls registerForRemoteNotifications from our example code / documentation. How do you want to coordinate this work?

[lawrence-forooghian]

@maratal in terms of testing:

• have you tested this end to end? i.e. tested in a sample app that push notifications work even without explicitly calling registerForRemoteNotifications
• can we also test (i.e. in our test suite) that registerForRemoteNotifications is called?

[maratal]

* have you tested this end to end? i.e. tested in a sample app that push notifications work even without explicitly calling `registerForRemoteNotifications`

Yes, I have simple demo app that does just that. I plan to convert it to SwiftUI according to #1318

* can we also test (i.e. in our test suite) that `registerForRemoteNotifications` is called?

I think yes, I need to investigate this.

UPD: changes here - 3cece2e

[lawrence-forooghian]

Thanks for the updates @maratal, I've resolved my code-level comments. Did you or @QuintinWillison have thoughts on this question and this one?

[QuintinWillison]

@lawrence-forooghian Regarding the REST API PATCH endpoint, I'm going to find it very difficult to gain enough context to form an opinion without spending more time than I could possibly find right now to dig around. I'm not familiar with this code. The documented constraints on what fields can be updated via that endpoint suggest that calling it multiple times needs to be considered very carefully. 🤷

In respect of your question around updating examples and documentation, I don't think that needs to block this pull request from landing, especially if it does end up being decided that multiple calls to registerForRemoteNotifications are harmless. But I might be missing something.

[lawrence-forooghian]

In respect of your question around updating examples and documentation, I don't think that needs to block this pull request from landing, especially if it does end up being decided that multiple calls to registerForRemoteNotifications are harmless. But I might be missing something.

I think my uncertainty / worry was that we'd merge this PR and then forget about the documentation work. How do we usually handle this sort of thing?

[QuintinWillison]

@lawrence-forooghian In order not to forget, we would usually create an issue in ably/docs.

[maratal]

Thanks for the updates @maratal, I've resolved my code-level comments. Did you or @QuintinWillison have thoughts on this question and this one?

@lawrence-forooghian @QuintinWillison Turned out it doesn't matter how many times in a raw you call registerForRemoteNotifications, UIKit gives only one callback to UIApplicationDelegate/didRegisterForRemoteNotificationsWithDeviceToken. Also, if you call registerForRemoteNotifications on already activated device, apple gives you the same deviceToken, so ARTPush/didRegisterForRemoteNotificationsWithDeviceToken exits early without sending ARTPushActivationEventGotPushDeviceDetails to the state machine.

[maratal]

In order not to forget, we would usually create an issue in ably/docs.

@lawrence-forooghian ably/docs#1332