Skip to content
This repository has been archived by the owner on Sep 4, 2020. It is now read-only.

[WIP] Amazon Device Message Support #815

Closed
wants to merge 5 commits into from
Closed

[WIP] Amazon Device Message Support #815

wants to merge 5 commits into from

Conversation

tf
Copy link
Collaborator

@tf tf commented Apr 19, 2016

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 a GCMAdapter. I've then added an ADMAdapter 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 the plugin.xml file. Some of these might require changes to cordova-android. So far I've added hook scripts which are included inside the example folder which bridge these gaps.

  • The ADM jar only has to be provided at compile time. It only contains stubs, which raise runtime errors if you accidentally compile the jar into the apk. To make this work, a provided item has to be added to the dependencies section of the Gradle file. At the moment it is possible to add compile items to the build.gradle file via <framework> tags inside the plugin.xml, but I couldn't find a way to make the item provided.
  • ADM requires an api_key.txt file to be placed inside the assets directory. Maybe there is a better way to have it created dynamically than with a hook script.
  • Inside the AndroidManifest.xml we need to use tags from an Amazon specific namespace. I could not find a way to add an xmlns via the plugin.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.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@fredgalvao
Copy link
Collaborator

I wonder if this could be adapted to also enable the plugin to work with some other third party push providers, like Pushy.

@macdonst
Copy link
Member

@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()) {
Copy link
Member

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.

Copy link
Collaborator Author

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?

Copy link
Member

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.

@tf
Copy link
Collaborator Author

tf commented Apr 21, 2016

@macdonst Thanks for the feedback! The amazon branch itself does not contain any commits not on master. It appears to be more of a marker for the last commit before removing the incomplete platforms. If I make a new PR against the amazon branch it will include anything that since happened on master, which will be a massive diff, right? Maybe I can do a PR to update amazon to master first?

Regarding the remaining open questions, shall I open issues for cordova-android? Do you know where I would do that? The GitHub repository does not have issues enabled.

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.

@macdonst
Copy link
Member

@tf I've just wacked the amazon branch and replaced it with a copy of master. So you should be able to send you PR to that branch without any issues. We can take discussion of the other items over to cordova-android but if you want me involved please remember to @macdonst as I'm on 11 slack channels right now.

@tf tf mentioned this pull request Apr 21, 2016
9 tasks
@tf
Copy link
Collaborator Author

tf commented Apr 21, 2016

Closed in favor of #822

@tf tf closed this Apr 21, 2016
@lock
Copy link

lock bot commented Jun 3, 2018

This thread has been automatically locked.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants