search

inloop / easygcm

Android Library for easy GCM integration in few lines of code.
Apache License 2.0
261 stars 43 forks source link

Application update correct handling #17

Closed DanielNovak closed 9 years ago

DanielNovak commented 9 years ago

Add support for MY_PACKAGE_REPLACED (https://medium.com/@murki/gcm-push-notifications-registration-done-right-7aba759d1d55). This fixes a case where the application is updated, the registration ID changes and the application stops receiving push messages until the application is started again.

A check was added to prevent double registration on GCM if the process is already running.

DanielNovak commented 9 years ago

@martinadamek or @destil - if you have some free time you can take a look at this. This adds support for MY_PACKAGE_REPLACED and also prevents double-registration by adding a check if the registration is already running. I had to convert the GCMHelper into a singleton to share the AtomicBoolean variable which is used to detect if the registration is running (I didn't want to use static variables).

Also there is a change in the public API - you have to provide the senderId in your application class - the GcmListener interface will force you to implement getSenderId(). I think this is also a cleaner approach than having the senderId all over the activitities in the init() calls.

To test this - uncomment the easygcm-sample module in the settings.gradle file. Increase the versionCode in the sample to test the application update handling. And you will have to clean/rebuild the application and deploy the latest easy-gcm library version into your repository by calling gradlew clean uploadArchives

davidvavra commented 9 years ago

I have looked at the code, it is OK. I might consider renaming getSenderId() to getGcmSenderId(), it might not be obvious what the method does inside Application class.

Yes setting senderId in Application is much cleaner than in Activites - alternative approach is to use a string resource like R.string.easygcm_sender_id and you require developers to add it into res/values/config.xml for example. I personally prefer this approach.

Btw: Is the init() method needed for all activites? From your documentation I figured that it's needed only for the first (main) activity.

DanielNovak commented 9 years ago

@destil Thanks for taking a look at this. The method renaming makes sense. I don't have a strong opinion on the senderId string resource versus method overriding. But the string resource approach saves some lines of code and is more readable. I think I will take this approach.

init() is only needed for the first activity - I think we should update the documentation to make it more clear. Calling it in every activity will not do any harm though.

davidvavra commented 9 years ago

I (and others at ex-Inmite) usually put all third-party service IDs, API keys, public keys etc. into one file config.xml and then we can access it from both xml and java. So this approach would be with no work :)

davidvavra commented 9 years ago

@DanielNovak I looked at your commit - it's better to move it from strings.xml to config.xml to teach developers it should be separated from app texts. And don't forget to add translatable=false to the string, otherwise it will cause Lint warnings.

DanielNovak commented 9 years ago

@destil Thanks for the very fast feedback. This is definitely a better approach - and it reduces the code complexity (no need to supply a senderId to the singleton and no need to check if the user has tried to change the senderId during runtime).

I have also noticed that we need to have a better error handling if the registration fails - right now we only log the exception message, but we need to provide a callback to the developer so that he can retry the request again (with exponential back-off). I will create a separate issue for this.