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

Add iOS10 thread-id to notification #41

Merged
merged 1 commit into from Sep 24, 2016
Merged

Add iOS10 thread-id to notification #41

merged 1 commit into from Sep 24, 2016

Conversation

sideshow
Copy link
Owner

When displaying notifications, the system visually groups notifications
with the same thread identifier together.

Fixes #40

When displaying notifications, the system visually groups notifications
with the same thread identifier together.

Fixes #40
@coveralls
Copy link

coveralls commented Sep 24, 2016

Coverage Status

Coverage decreased (-13.6%) to 86.395% when pulling 94a3a3c on feature-thread-id into 0ef66aa on master.

@sideshow sideshow merged commit 741a20d into master Sep 24, 2016
@sideshow sideshow deleted the feature-thread-id branch September 24, 2016 01:09
@jchambers
Copy link

Hello! I'm the main author of Pushy, an APNs library for Java. We're just getting around to adding support for thread-id (oops), but are having a hard time verifying that our changes are actually having an effect. If you have a moment and wouldn't mind providing some advice, I had a couple questions for you:

  1. I notice that you've implemented thread-id as a header instead of a field inside the aps section of the payload. The "Payload Key Reference" section of the APNs docs certainly makes it sound like it ought to be in the payload; did you find the docs to be incorrect?
  2. What behavior change do you observe when sending notifications with a thread-id header? I haven't been able to spot any changes to message ordering or appearance in my phone's Notification Center in testing. In particular, I've been alternating sending messages with and without a thread ID, but they all seem to just appear serially in the order in which they were received. Have you seen something different?

Thanks very much for your time!

@sideshow
Copy link
Owner Author

Hey @jchambers - You're absolutely right and I interpreted the docs incorrectly. I based this implementation of an early version of the docs and obviously got it wrong.

I have patched locally to send via payload and can confirm it is working when sent as aps.thread-id as per the latest docs.

I was also able to confirm it was working in iOS by putting a breakpoint on the UNNotificationContentExtension didReceive method.

Payload:

{
  "aps" : {
    "thread-id" : "my-thread-id",
    "alert" : "Hello!",
    "category": "my-category"
   }
} 

Breakpoint:

screen shot 2016-12-15 at 11 01 13 pm

With regards to the apple docs which read:

"When displaying notifications, the system visually groups notifications with the same thread identifier together.", as you picked up, this doesn't seem to be the case for iOS.

A better reference and explanation:

"The system calls this method to deliver notifications to your Notification Content app extension. Use this method to configure the contents of your view controller or to incorporate content from a new notification. This method may be called multiple times while your view controller is visible. Specifically, it is called again when a new notification arrives whose threadIdentifier value matches the thread identifier of the notification already being displayed. The method is called on the main thread of your extension".

https://developer.apple.com/reference/usernotificationsui/unnotificationcontentextension/1648525-didreceive

Thanks! Adam

@jchambers
Copy link

🤘

Thanks for the detailed explanation, and thanks for the indeed-much-better-explanation of what we're looking for. Hope I can return the favor some time!

sideshow added a commit that referenced this pull request Dec 16, 2016
Fixes issue #61 by removing thread-id from the Notification struct.
You now add a thread-id to the aps payload as per Apple docs.
See #41 (comment)
for discussion.
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.

None yet

3 participants