-
Notifications
You must be signed in to change notification settings - Fork 40
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
Android push implementation. #308
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you're making good progress. I realise the admin API is lower priority, but we'll need to add it for completeness quite soon and then add to other select libraries too (although they can use the request method where necessary of course).
android/build.gradle
Outdated
androidTestCompile 'com.android.support.test:runner:0.5' | ||
androidTestCompile 'com.android.support.test:rules:0.5' | ||
androidTestCompile "com.crittercism.dexmaker:dexmaker:1.4" | ||
androidTestCompile "com.crittercism.dexmaker:dexmaker-dx:1.4" | ||
androidTestCompile "com.crittercism.dexmaker:dexmaker-mockito:1.4" | ||
|
||
compile 'com.google.android.gms:play-services-gcm:10.0.1' | ||
compile 'com.google.firebase:firebase-messaging:10.0.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Firebase needed or can we avoid that dependency by using native APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is needed. There's nothing available to the public behind it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
|
||
public void activate(Context context) { | ||
SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); | ||
if (prefs.getBoolean(SharedPrefKeys.ACTIVATED, false)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of interest, what if a user disables push notifications for their device. Does the token get invalidated, or are push notifications simply disabled? The reason I ask is that if I allow push, then disable push for my app, and then the dev tries to request push access again, will prefs.getBoolean(SharedPrefKeys.ACTIVATED, false)
be false? Is there any way for a dev to know if push becomes disabled or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Push.activate is about Ably, not about F/GCM. Push.activate/deactivate registers/unregisters with Ably. So the token isn't invalidated in F/GCM's servers or anything like that, it's just unregistered on Ably's so that we don't send notifications to it anymore.
The reason I ask is that if I allow push, then disable push for my app, and then the dev tries to request push access again, will prefs.getBoolean(SharedPrefKeys.ACTIVATED, false) be false?
It will be false once deactivation completes, then true again once re-activation completes.
Is there any way for a dev to know if push becomes disabled or not?
Not directly, but why would he want to do that? It can only become disabled if she disables it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. It is common to detect that push notifications are not enabled and prompt the user to enable them. Using Ably, how would a developer know if push notifications are still enabled for this app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean if push notifications are enabled on Android's part, Android apps don't need to ask for permissions to publish notifications as iOS apps do. So it's always enabled. The user can't disable them in that sense.
If you mean if the device is registered for push notifications with Ably, then... yeah, there's no way to check that. So you want a way to actively check with Ably's servers if the device is registered?
} else { | ||
rest.device = device; | ||
Http.RequestBody body = rest.http.requestBodyFromGson(device.pushMetadataJsonObject()); | ||
rest.asyncHttp.put("/push/deviceRegistrations/" + device.id, HttpUtils.defaultAcceptHeaders(rest.options.useBinaryProtocol), null, body, null, new ActivateCallback(context, "PUSH_RENEWED_TOKEN")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever retry a failed request i.e. if the HTTP request fails, is it then up to the dev to pick up the pieces, or do we retry updating Ably perhaps next time the lib is connected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's something I've considered here but I think it's preferable to just call the callback (the BroadcastReceivers in this case) with an error, just like we do with all other requests. What can we really do if all fallback hosts, etc. fail? It's probably the user who needs to fix something (auth, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What can we really do if all fallback hosts, etc. fail? It's probably the user who needs to fix something (auth, etc.).
Wait until an internet connection becomes available and try again?
this.token = token; | ||
} | ||
|
||
public static RegistrationToken persisted(Context context) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not quite sure I understand why this method is named persisted
. Mind explaining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It returns the persisted registration token. getPersisted
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, perhaps, I was a little confused what it did to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done at 0bd203b.
So, while implementing deactivate I realized the current implementation of the activation process it 1. an unclear mess and 2. broken. I'm refactoring it to follow a well-defined, exhaustive, event-based state machine that will be included in the spec. So don't look at that part just yet. |
OK, the state machine thing is done now. I'll write down a spec for it so that @ricardopereira can make sure iOS does the exact same thing. |
So there it goes the admin API and other missing pieces. I think that's all. Some of it I haven't even tested once, so expect bug fixes down the line. |
// version on users. Ideally we would specify a version range, | ||
// but the Google services Gradle plugin doesn't seem to | ||
// support that. | ||
// TODO: Make sure this works when installing from Maven! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this TODO be resolved before we merge?
Hello guys, I tried to follow your instructions...but the Android AAR can't be generated. I can generate the jars, but the AAR can't be. I keep getting Even though the android project is indeed in my ably-java folder...I am not sure why it keeps doing that, but it is stopping me from using the Push Notifications :/ Any idea ? |
Do you have the Android SDK installed? See https://github.com/ably/ably-java/blob/master/settings.gradle#L4 |
Hello paddybyers, haha of course I have the Android SDK installed, I have been developing Android app for years ;-) To be fair, I have switched to using FCM myself instead of going through Ably for Push Notifications. I will keep ably for real time, until the team has included the Push Notification in the default gradle project. Thanks for the answer and taking time to read my problem though :-) |
Well I thought I'd ask :) If |
This is needed for the push beta, since we're not releasing versions for it but telling users to just generate and include the AAR. But we still need them to install our dependencies, so they need to include our dependencies.gradle files from their own .gradle files. See #323.
build tools and configuration to latest versions; update Firebase support to newer API; remove GCM support
each tests. Add several other bugfixes.
This was merged independently. |
Installation
push
branch:git clone -b push git@github.com:ably/ably-java.git