Enable compatibility with other Android GCM providers #166
Conversation
| @Override | ||
| protected void onHandleIntent(Intent intent) { | ||
| if (intercomPushClient.isIntercomPush(intent.getExtras())) { | ||
| intercomPushClient.handlePush(getApplication(), intent.getExtras()); |
There was a problem hiding this comment.
Can we return; after this line and not have the else on the next line?
jtreanor
force-pushed
the
jtreanor/gcm_interoperability
branch
from
8e25bcc
to
850c460
Compare
| Intent passThroughIntent = new Intent(intent); | ||
| passThroughIntent.setComponent(null); | ||
|
|
||
| List<ResolveInfo> services = getPackageManager().queryIntentServices(passThroughIntent, 0); |
There was a problem hiding this comment.
It seems to be the default value. Just checking the docs to confirm my understanding of this.
There was a problem hiding this comment.
This is an @IntDef flag, so 0 means "no flags". (It is 0 because 0 | x == x).
jtreanor
force-pushed
the
jtreanor/gcm_interoperability
branch
from
850c460
to
84d2fb2
Compare
| } catch (ClassNotFoundException e) { | ||
| // Class not found. Try the next service | ||
| } | ||
| } |
There was a problem hiding this comment.
is there something to be done here if there are services and none of them have been found?
There was a problem hiding this comment.
No. If there are no services there is nothing to be done (e.g. only Intercom is setup for push).
There was a problem hiding this comment.
If there is no services in the services list that's true, I'm wondering about the situation where that is not empty and we still exit the loop (i.e. an exception was thrown and caught in the loop)
There was a problem hiding this comment.
Discussed offline. The behaviour here is that if Class.forName() somehow fails, we will move on and try to pass to the next highest priority service.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Any tests we could write for this?
There was a problem hiding this comment.
Unfortunately not really. There isn't a way to run native Android tests for a Cordova plugin.
We could set up an Android project in the repo that includes this file (and others) so that they can be unit tested. Something to consider in the future perhaps.
|
Thanks everyone. I'll push a new release in the morning. |
Since we built this plugin, there have been many instances of people experiencing issues with Android push notifications when using other push providers (such as phonegap-plugin-push, OneSignal or UrbanAirship). The cause of this has been that Android will only notify one
GcmListenerServiceinstance of incoming notifications. Here are some of the issues: https://github.com/intercom/intercom-cordova/issues/165, https://github.com/intercom/intercom-cordova/issues/164, https://github.com/intercom/intercom-cordova/issues/139, https://github.com/intercom/intercom-cordova/issues/19, https://github.com/intercom/intercom-cordova/issues/3 and phonegap/phonegap-plugin-push#1640.The fix until now has been to fork
phonegap-plugin-push. This is less than ideal and does not address the issue for other plugins.These changes introduce
IntercomIntentServicewhich acts as an entry point for allcom.google.android.c2dm.intent.RECEIVEintents. If the Intent is an Intercom notification, we handle it. Otherwise it is passed to the next highest priority service.This change will stop the need to use a forked version of
phonegap-plugin-pushas well as allowing Intercom to work alongside other push plugins.Note: this does not fix the issue for FCM. It may be possible to use a similar solution for that in the future.