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

Send notification #9

Open
AdriVanHoudt opened this issue Sep 17, 2015 · 8 comments
Open

Send notification #9

AdriVanHoudt opened this issue Sep 17, 2015 · 8 comments

Comments

@AdriVanHoudt
Copy link

I am using Incus https://github.com/Imgur/incus and trying to send a notification, so far everything works except the notification itself, meaning the phone recieves the message and the data but does not show a notification.
I was looking at https://developers.google.com/cloud-messaging/concept-options#notifications_and_data_messages and https://github.com/alexjlockwood/gcm/blob/master/message.go#L7-L15 and is it possibe it is not at all possible to do this atm? Or am I missing something.

cc @jacobgreenleaf (dev for Incus)

@alexjlockwood
Copy link
Owner

Hi, thanks for the report! I've confirmed that this is also an issue in Google's official Java library as well (see google/gcm#80). I haven't been keeping up-to-date with the changes Google has been making to GCM lately... I assume this must have been added since I initially wrote this library.

My one concern with your pull request is that I believe it will break others code who are depending on the code as well (Go doesn't have optional arguments, so changing the NewMessage method signature will cause compile-time errors I believe). I think you might be able to just add only the Notification field to the struct w/o modifying the method at all... the only thing is you'd have to build the Message using the standard struct notation (as opposed to using the NewMessage convenience method). I'd have to confirm that wouldn't break anything either though since TBH I have no idea how many people are using this library right now and haven't used Go myself in about a year. :)

Sorry for the inconvenience... I'll try my best to figure this out soon. Until then you can always feel free to fork the library and modify it yourself.

@AdriVanHoudt
Copy link
Author

Hi,
Thanks for the response.
I have just started with gcm so I don't know when this was added.
I am not experienced with Go at all, I just did this PR to allow this behaviour on Incus see Imgur/incus#71
If there is a better solution I am willing to help/update this PR.

@AdriVanHoudt
Copy link
Author

Any progress on this?

@dnldd
Copy link

dnldd commented Oct 12, 2015

@AdriVanHoudt you can work around this in the mean time by getting the message in your GCM receiver, getting the data in the message and creating a notification with the details.

@AdriVanHoudt
Copy link
Author

@dnldd not sure what you mean. You want to pass along the notification inside the data and then extract it?

@dnldd
Copy link

dnldd commented Oct 12, 2015

Yeah exactly, the notification k/v is pretty new so you can do this until it's implemented via your PR or Alex's solution.

@AdriVanHoudt
Copy link
Author

then why not just merge my PR? Your proposal is confusing, taking out keys, if you already have a notifications key it will break for example

@0x7061
Copy link

0x7061 commented Nov 1, 2015

This library definitely needs the new keys/options. Recently I started using GCM also for iOS and there the notification key is very important. Without it APNS sends no notification.

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 a pull request may close this issue.

4 participants