timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

Simple notification if .Body is the only Alert field non-empty #35

Closed themartorana closed 9 years ago

themartorana commented 9 years ago

Following Apple's specs, this changes the marshaled JSON for apns.Alert to set the alert key to the text of Alert.Body if all other fields of the alert are empty. Also updated the .isZero() check to check the .Title and .Action properties of the alert.

From: https://developer.apple.com/library/ios/documentation/NetworkingInternet/Conceptual/RemoteNotificationsPG/Chapters/ApplePushService.html

Note: If you want the device to display the message text as-is in an alert that has both the Close and View buttons, then specify a string as the direct value of alert. Don’t specify a dictionary as the value of alert if the dictionary only has the body property.

nathany commented 9 years ago

This looks good to me. @bdotdub

nathany commented 9 years ago

There was some talk of making Alert a pointer but what you have here looks like a good step in the interim.

nathany commented 9 years ago

Oh. See #26.

themartorana commented 9 years ago

The only problem with using a series of pointers in structs is they can end in race conditions if any goroutines mutate the underlying pointer.

For that reason, I took the same approach here for omitempty badges: https://github.com/joekarl/go-libapns/pull/8

It's not "pretty" but it's thread-safe. (I can move this discussion to #26 if you like.)

nathany commented 9 years ago

That is a good point. We aren't wanting to share the data (pointers). I wonder if we should do something like that for badges too?

themartorana commented 9 years ago

(I'd be happy to code that up and submit a pull request, although it would break the API.)

For this, though, I'm hesitant to make Alert a pointer for the same reason. If Alert were type interface{} though, wouldn't that get us 90% of the way there? You can check for nil, and it will pass by value.

Edit: Test: http://play.golang.org/p/1OwVR_fTzT

nathany commented 9 years ago

I'd wait for some feedback from @bdotdub, who is the maintainer of the project.

taylortrimble commented 9 years ago

:+1:. I was a proponent of pointers before, but this is semantically nice. Also like the badge approach from joekarl/go-libapns#8. I wish they were more convergent, but I'll live.

taylortrimble commented 9 years ago

If this beats #36 then #36 becomes unnecessary. If #36 beats this in then it's a simple merge conflict.

nathany commented 9 years ago

@themartorana Sorry, there is a merge conflict due to my changes in #36. Should be easy to resolve.

themartorana commented 9 years ago

@nathany fixed!

nathany commented 9 years ago

LGTM