-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
Copy from deprecated phonegap-build/PushPlugin plugin.
I wonder if this could be adapted to also enable the plugin to work with some other third party push providers, like Pushy. |
@tf this is really cool but I'm not going to merge this immediately. Why don't you submit this PR to the amazon branch instead of master and we can iterate on it there? |
super.initialize(cordova, webView); | ||
gForeground = true; | ||
|
||
if (isAmazonDevice()) { |
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'd flip this if as it is more likely that it will be a Google Device not an Amazon one.
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 I turn this into isGoogleDevice
the detection will probably still be "manufacturer is not amazon" and flipping this by turning the condition into !isAmazonDevice
would leave with a negated if
with an else
clause which also is an antipattern. Maybe I'm missing your point?
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 just like to have the most likely option at the top of my if then else chain. What you've done is fine, just a personal preference.
@macdonst Thanks for the feedback! The Regarding the remaining open questions, shall I open issues for EDIT: Found the big jira link. @fredgalvao I guess so, as long as their API fits the life cycle of the existing adapters. Probably the adapter would have to be configured then explicitly instead of trying to auto detect. |
@tf I've just wacked the |
Closed in favor of #822 |
This thread has been automatically locked. |
Our app needs to receive push notifications via ADM. I've ported the functionality from the deprecated PushPlugin. Currently GCM and ADM both work, but there are still details to be improved. I just wanted to put this in the open early to get feedback and avoid unnecessary duplication of efforts.
Description
Since
cordova-amazon-fireos
is deprecated I've chosen to implement the ADM functionality as part of the Android plugin. I have thus extracted all GCM specific code into aGCMAdapter
. I've then added anADMAdapter
which collaborates with the imported ADM specific classes. If you look at the individual commits of the PR, you can see what actually changed in the imported files.At the moment the decision which adapter to use is based on the
android.os.Build.MANUFACTURER
property. I'm not sure this is good enough to handle all cases.Open Questions
We generate our Cordova projects in CI using
cordova prepare
. I was unable to express some of the required setup steps inside theplugin.xml
file. Some of these might require changes tocordova-android
. So far I've added hook scripts which are included inside theexample
folder which bridge these gaps.provided
item has to be added to thedependencies
section of the Gradle file. At the moment it is possible to addcompile
items to thebuild.gradle
file via<framework>
tags inside theplugin.xml
, but I couldn't find a way to make the itemprovided
.api_key.txt
file to be placed inside theassets
directory. Maybe there is a better way to have it created dynamically than with a hook script.AndroidManifest.xml
we need to use tags from an Amazon specific namespace. I could not find a way to add anxmlns
via theplugin.xml
file.More generally, I am not yet sure, whether adding Amazon specific items to the manifest might be an issue when submitting apps to the Play Store. In the light of the decision to deprecate
cordova-amazon-fireos
this might be a non-issue though?Related Issue
#300 (Amazon-Fireos support) asks for this feature.
Motivation and Context
ADM support is a requirement for us.
How Has This Been Tested?
Apart from some Javascript tests, this plugin appears to be rather untested. This doesn't change with this PR. So far I have tested my code by sending messages to Google and Amazon devices and looking at the logs. I'm open to suggestions how test coverage could be improved.
Types of changes
So far I've tried to leave the GCM components unchanged as far as possible. If the adapter approach should be agreed upon, more of those could be moved to the
gcm
package.Checklist: