Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PushKit support #1458

Merged
merged 9 commits into from
Sep 4, 2017
Merged

PushKit support #1458

merged 9 commits into from
Sep 4, 2017

Conversation

morozkin
Copy link

@morozkin morozkin commented Aug 21, 2017

To test PushKit notifications on your local machine change bundle id of the project to im.vector.pushtest. Perform changes in initMatrixSession to allow calling [accountManager prepareSessionForActiveAccounts] even in background mode. Also you will need a pem certificate which can be provided to you by me or Dave. To send pushes i use Houston.

Initially PushKit must be used only for VoIP notifications, but then it was decided to use it for all types of notifications in Riot. So the purpose of this is PR to provide this level of support.

The flow of registering for notifications didn't change a lot. There're new methods which replaced the old one. All of them belongs to PKPushRegistryDelegate protocol:

pushRegistry:didUpdatePushCredentials:forType:
pushRegistry:didInvalidatePushTokenForType:
pushRegistry:didReceiveIncomingPushWithPayload:forType:

To be able to handle PushKit notifications we create a PushRegistry instance, set AppDelegate as its delegate and hold for entire life of the app.

We subscribe for events from MXNotificationCenter instance of active MXSession when a new push is received. On arriving of each event an alert body string is created and then local notification is displayed.

Since we have a deal with network i add a delay for two cases:

  1. There is a pushNotificationHandlingTimeoutBlock which is called when server sync takes a lot of time.
  2. pushNotificationHandlingCompletionBlock is called with delay after push event has been successfully processed. I added delay here to support handling of series incoming pushes. Push1->Push1 has been handled->Schedule Push1 completion block->Push2->Cancel Push1 completion block->Wait for Push2 event...

I set a random constants for these delays, so i'm glad to hear your proposals.

The list of possible cases we can meet:

  1. Receive push -> start listening for push events from server sync response -> schedule timeout block -> cancel timeout block -> handle push event -> schedule completion block -> execute completion block -> end

  2. Receive push -> start listening for push events from server sync response -> schedule timeout block -> execute timeout block -> end

  3. Receive push -> start listening for push events from server sync response -> schedule timeout block -> cancel timeout block -> handle push event -> schedule completion block -> receive new push -> cancel completion block -> schedule timeout block -> wait for new push event from server

  4. Receive push -> start listening for push events from server sync response -> schedule timeout block -> new push -> cancel timeout block -> schedule new timeout block -> cancel timeout block -> handle push event (from 1st push) -> schedule completion block -> | WAITING TIME | ->

We have two ways to continue:
-> execute completion block (1st push event) -> end (2nd push event won't be handled) PROBLEM
-> cancel completion block -> receive new push event (from 2nd push) -> schedule new completion block -> end

If we won't receive any events before completion block of first push event would be called we will lose second push event.

  1. Receive push -> start listening for push events from server sync response -> schedule timeout block -> cancel timeout block -> handle push event (contains event for future push) -> schedule completion block -> receive push -> cancel completion block -> schedule timeout block -> execute timeout block -> end

Signed-off-by: Denis Morozov dmorozkn@gmail.com

@giomfo giomfo self-requested a review August 21, 2017 22:37
dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(120.0 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
timeoutBlock();
});
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giomfo Similar situation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@morozkin But Apple guideline says you have to call reportIncoming Call in pushKit Delegate otherwise application will crash.

dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5.0 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{
completionBlock();
});
};
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giomfo We need to decide which value will be appropriate here and i think it'll be better to assign this value to a constant

@morozkin
Copy link
Author

@giomfo Maybe it also will be better to pause MXSession when we've ended with push handling?

@morozkin
Copy link
Author

morozkin commented Aug 24, 2017

Receive push -> start listening for push events from server sync response -> schedule timeout block -> new push -> cancel timeout block -> schedule new timeout block -> cancel timeout block -> handle push event (from 1st push) -> schedule completion block -> | WAITING TIME | ->

We have two ways to continue:
-> execute completion block (1st push event) -> end (2nd push event won't be handled) PROBLEM

@giomfo wdyt?

Also have a look at other scenarios

@morozkin
Copy link
Author

If #1472 will look good to you, we will need to merge it with this PR

@manuroe manuroe changed the base branch from develop to callkit September 1, 2017 13:54
@manuroe manuroe merged commit 53d635c into element-hq:callkit Sep 4, 2017
@manuroe
Copy link
Member

manuroe commented Sep 4, 2017

The PR was merged even if some issues has been detected. See https://matrix.to/#/!dBmmlOkhBOeZNEAfDK:matrix.org/$1504019660293757xpuGS:matrix.org

Step by step improvements will follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants