ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.31k stars 206 forks source link

Add notification info to display on lock screen #127

Closed kaija closed 9 years ago

kaija commented 9 years ago

Add a message function to setup notification information. message.addNotification('Title', 'logo path', 'Text');

hypesystem commented 9 years ago

This looks interesting. I need some more explanation as to exactly what this does, maybe a link to the corresponding section in the documentation.

I think the added two lines in the README add more confusion than they help. Maybe this needs its own section in the README?

kaija commented 9 years ago

Here is the GCM server API document link https://developers.google.com/cloud-messaging/server#notifications_and_data_messages https://developers.google.com/cloud-messaging/server-ref#table1 I only choice icon, title and body field. Can you provide comment? Thanks.

kaija commented 9 years ago

Update patch and add GCM notification option table in README

ericdolson commented 9 years ago

Please merge this PR. Now that GCM has this "notification" type supported, it would be great to have. And very necessary to have too in my case! I looked over the code in this PR and it looks as though it properly supports the notification data.

hypesystem commented 9 years ago

Sorry for kind of dropping the ball on this a bit. Before I can merge it, I need a test or two showing that it works. Is that something you can do, @kaija? Thanks :)

kaija commented 9 years ago

Add test case of message and add a example. :)

hypesystem commented 9 years ago

Ok. A single note:

I think you should just remove the addNotificationWithKeyValue and addNotificationWithObject methods: there is no reason to add methods that are already deprecated. This also means you can remove the tests specific to these methods.

Do this, and I'll pull locally and run the tests, then merge :) thank you for the time you're spending on this!

kaija commented 9 years ago

Done :)

marcusmc commented 9 years ago

Great feature! Was just about to write it myself!

kentmw commented 9 years ago

+1 this feature. I would love to be able to use it. @hypesystem , will it get merged now that it has tests?

hypesystem commented 9 years ago

Yep, tests passing on my machine.

kentmw commented 9 years ago

Hmm...everything looks right, but I can't get the notifications showing while testing. If I switch to:

message.addData('title', 'My Title'));
message.addData('message', 'My message');

It shows a push notification with title and body/message, vibrates, and the whole lot, but if I use:

message.addNotification({
  'title': 'My Title',
  'body': 'My message'
});

nothing happens.

Am I missing something? Do notifications actually create messages that show up to the user?

kentmw commented 9 years ago

A clarifying point, when "nothing happens" the request body during the post is:

{"registration_ids":["APA91bEmPOMoQ ... 4ngo-sD"],"data":{},"notification":{"title":"My Title","body":"My message"}}

which looks to be correct to me.

kentmw commented 9 years ago

After adding the icon (I missed that it was required), I can catch the notification in my app, but it still doesn't create a notification in my notification tray. :(

marcusmc commented 9 years ago

I'm running an app from kaija/node-gcm, and it's working great for me. The notifications work great as well.

On Tue, Jun 30, 2015, 6:22 PM Kent Willis notifications@github.com wrote:

After adding the icon (I missed that it was required), I can catch the notification in my app, but it still doesn't create a notification in my notification tray. :(

— Reply to this email directly or view it on GitHub https://github.com/ToothlessGear/node-gcm/pull/127#issuecomment-117361270 .

hypesystem commented 9 years ago

@kentmw are you using a WakefulBroadcastReceiver in your app? In that case you need to migrate to a GCMReceiver and GcmListenerService, see the guide

kentmw commented 9 years ago

I'm using the push plugin from phonegap: https://github.com/phonegap-build/PushPlugin. Not the greatest project, but it had seemed to be working. Here's the intent service and the receiver it's using: https://github.com/phonegap-build/PushPlugin/blob/master/src/android/com/plugin/gcm/GCMIntentService.java and https://github.com/phonegap-build/PushPlugin/blob/master/src/android/com/plugin/gcm/CordovaGCMBroadcastReceiver.java

kentmw commented 9 years ago

Looks like the cordova pushplugin does:

if (PushPlugin.isInForeground()) {
    extras.putBoolean("foreground", true);
        PushPlugin.sendExtras(extras);
}
else {
    extras.putBoolean("foreground", false);

        // Send a notification if there is a message
        if (extras.getString("message") != null && extras.getString("message").length() != 0) {
                createNotification(context, extras);
        }
}

So, it won't create a notification if there isn't a data.message. Otherwise it pushes the extras (I believe the contents of data) to the client app

hypesystem commented 9 years ago

@kentmw This seems to be an issue with a different plugin, not the node-gcm library, so there might be a more fitting place to continue this discussion (or save it!).

You could upload a question to StackOverflow, and answer it yourself, if you solved the problem :)