FCM Support - GCM Module #800

Jawnnypoo · 2018-04-12T07:26:17Z

This ended up being a pretty large effort and a substantial set of changes.

The good news is that now that GCM is not anywhere in the core SDK, it removes quite a bit of code and allows us to lean more on the Google Play Services library to do the implementation.

So, this update will not be backwards compatible, and will require us to solve the problem of shipping multiple artifacts with the library. I think maybe you have more experience with this process, @rogerhu ? I once again would advocate for switching over to jitpack.io to make the releases easy and doable by anyone who can tag a release. But I digress.

I have tested the gcm and fcm modules in a sample project with a standard setup. Everything seems to work fine. Note that the instructions for setting up can be found in the README of the gcm and fcm modules respectively. We will need to update http://docs.parseplatform.org/tutorials/android-push-notifications/ to either reflect the changes, or point users to the FCM and GCM docs, leaning them more toward FCM since GCM is going away.

Somewhat unrelated, but I think with these changes bumping us to a 1.17.0, we should consider removing or deprecating any other things we think need to be removed or deprecated. I for one think that we need to get rid of parsing the server url and application id and client key from the Android Manifest. It seems like as we add more and more methods to the Parse.Configuration.Builder, we would just want people to use this instead.

Jawnnypoo · 2018-04-12T07:29:34Z

Parse/src/main/java/com/parse/PLog.java

+/**
+ * Parse Logger
+ */
+public class PLog {


Had to make this public to allow for logging in the other modules

codecov · 2018-04-12T07:31:25Z

Codecov Report

Merging #800 into master will increase coverage by 1.22%.
The diff coverage is 25%.

@@             Coverage Diff              @@
##             master     #800      +/-   ##
============================================
+ Coverage     53.35%   54.57%   +1.22%     
+ Complexity     1749     1716      -33     
============================================
  Files           132      124       -8     
  Lines         10268     9874     -394     
  Branches       1426     1384      -42     
============================================
- Hits           5478     5389      -89     
+ Misses         4337     4042     -295     
+ Partials        453      443      -10

Impacted Files	Coverage Δ	Complexity Δ
...in/java/com/parse/ParsePushChannelsController.java	`4.34% <ø> (-2.11%)`	`1 <0> (-1)`
Parse/src/main/java/com/parse/PushRouter.java	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
Parse/src/main/java/com/parse/PLog.java	`60% <0%> (ø)`	`10 <0> (ø)`	⬇️
Parse/src/main/java/com/parse/ParseFileUtils.java	`36.58% <0%> (ø)`	`19 <0> (ø)`	⬇️
...rse/src/main/java/com/parse/ParseInstallation.java	`69.52% <0%> (-5.72%)`	`35 <0> (-4)`
Parse/src/main/java/com/parse/Parse.java	`48.59% <100%> (-0.94%)`	`22 <0> (ø)`
...ain/java/com/parse/ParsePushBroadcastReceiver.java	`0% <0%> (-4.84%)`	`0% <0%> (-3%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ecbff...5b9f4ee. Read the comment docs.

rogerhu · 2018-04-12T07:40:22Z

fcm/README.md

+
+You will then need to register some things in your manifest, specifically:
+```xml
+<service


The parse-android-fcm should include this service in its AndroidManifest.xml file. There should be manifest merging when building it as an AAR.

The problem is the balance between registering these services and allowing users to have their own. If we register it in the app module, it will not allow users to override and potentially send tokens to another server or save them elsewhere. Its similar to the reason why we do not automatically register the ParsePushReceiver in the Parse SDK library. Does that make sense? I think a user would be able to use tools:remove in the manifest to remove the library ones, but that might be more complex them just asking the user to add the registration (which is what you do in the normal FCM setup)

rogerhu · 2018-04-12T07:40:39Z

fcm/README.md

+
+```xml
+<service
+    android:name=".MyFirebaseMessagingService">


same here too?

rogerhu · 2018-04-12T07:43:11Z

gcm/README.md

+    android:permission="com.google.android.c2dm.permission.SEND" >
+    <intent-filter>
+        <action android:name="com.google.android.c2dm.intent.RECEIVE" />
+        <category android:name="com.your.package.name.here" />


You should be able to use so you don't need to worry about mistyping. Gradle swaps in the correct package name.

 <permission android:protectionLevel="signature" android:name="${packageName}.permission.C2D_MESSAGE" /> <uses-permission android:name="${packageName}.permission.C2D_MESSAGE" />```

rogerhu · 2018-04-12T07:43:54Z

gcm/src/main/AndroidManifest.xml

+
+    <application>
+
+        <service


move ParseGCMListenerService and ParseGCMInstanceIDListenerService here too?

rogerhu · 2018-04-12T07:44:51Z

@Jawnnypoo you can setup to build artifacts for different projects, this happens already with https://github.com/parse-community/ParseUI-Android

rogerhu · 2018-04-12T07:45:24Z

Thanks for working on this!

rogerhu · 2018-04-12T07:48:52Z

fcm/src/main/java/com/parse/fcm/ParseFCM.java

+                        Constraint.ON_ANY_NETWORK
+                )
+                .setService(ParseFirebaseJobService.class) // the JobService that will be called
+                .setTag("upload-token")        // uniquely identifies the job


can we set as a constant?

rogerhu · 2018-04-12T07:49:39Z

gcm/README.md

+    implementation 'com.parse:parse-android:latest.version.here'
+    implementation 'com.parse:parse-android-gcm:latest.version.here'
+
+    implementation "com.google.android.gms:play-services-gcm:latest.version.here"


this shouldn't be necessary given you declared play-services-gcm as an api dependency

Jawnnypoo · 2018-04-13T02:30:52Z

Okay @rogerhu Feedback has been addressed. Let me know if my comment above makes sense about not registering the services and receivers in the library projects. I think the reasoning would be the same as why it is not done automatically in the normal FCM implementation either, outlined here: https://firebase.google.com/docs/cloud-messaging/android/client

Basically, only one instance ID and messaging service is allowed per app, and forcing the Parse ones to already be registered would not allow users to define their own.

Jawnnypoo · 2018-04-13T02:32:34Z

Something else to note is that I cannot update the build to use Gradle 4.4, with gradle build tools com.android.tools.build:gradle:3.1.1, there are errors being thrown when I build with those versions that I think have to do with either the artifact generation or the javadoc generation. I am not too familiar with these parts of the build process, so maybe you can look into this too @rogerhu ?

rogerhu · 2018-04-13T13:51:11Z

@Jawnnypoo you'll just need to add

https://github.com/rogerhu/Parse-SDK-Android/blob/070c3afd1aec31efaf466da6877fd6cb6f0c84dc/Parse/build.gradle#L214-L262 to both the fcm and gcm directories.

https://github.com/parse-community/Parse-SDK-Android/pull/804/files simplifies the Gradle statements

rogerhu · 2018-04-13T14:09:52Z

Rebase too :)

I suggest we use a common version share between the fcm and gcm version as well

Jawnnypoo · 2018-04-13T16:25:47Z

I suggest we use a common version share between the fcm and gcm version as well

Do you mean for the FCM/GCM dependencies?

rogerhu · 2018-04-13T16:27:30Z

Yeah see commonLibVersion in https://github.com/parse-community/ParseUI-Android/blob/master/build.gradle

Jawnnypoo · 2018-04-14T00:02:51Z

Okay, added the common lib version to all modules. I am not sure which parts of the Parse/build.gradle I need to apply to the gcm and fcm modules? Does it require all of the bintray plugin configuration, all of the artifactory info, etc? It seems like this would be a lot of duplication of the same thing if so. Is there a way we could pull this out into its own script, similar to what Square does here? https://github.com/square/sqlbrite/blob/master/gradle/gradle-mvn-push.gradle

rogerhu · 2018-04-14T00:06:20Z

Parse/build.gradle

@@ -5,7 +5,7 @@ apply plugin: 'com.github.kt3k.coveralls'
 apply plugin: 'com.jfrog.bintray'

 group = 'com.parse'
-version = '1.16.8-SNAPSHOT'
+version = rootProject.ext.commonLibVersion


keep this version? i think we don't need to have the same version tied to the gcm/fcm plugins...

Do you plan to distribute the plugins separately? having everything at the same version helps understanding issues :)

Yeah good point. We can tie the versions together as it's done with the OkHttp3 library...

I believe it will be simpler to keep track of changes. Also that's what we do on the iOS SDK now that we moved everything as a mono repo.

rogerhu · 2018-04-14T00:13:03Z

Yeah feel free to extract it out and use the apply from approach...

natario1 · 2018-04-14T00:16:08Z

Nice work @Jawnnypoo !

You should now remove PushServiceApi26 from Parse/manifest.

Speaking about cleanup, I remember being tempted to rip off PushRouter and PushHistory. PushHistory seems to be doing legacy operations, and PushRouter seem to exist only to deal with the history. Is it worth to take a look?

flovilmart · 2018-04-14T00:18:06Z

@natario1 they are used for de-duplication (https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/PushRouter.java#L28)

flovilmart · 2018-04-14T00:21:58Z

@mmimeault feel free to have a look at it too :) we could beta test on our side as well.

Jawnnypoo · 2018-04-14T01:18:59Z

For the PushRouter and PushHistory, I think maybe previously there was duplication due to the fact that the Parse Android SDK was registering for GCM push tokens via a custom implementation instead of relying on the Google Play Services version. I would venture to guess that now that we use the Google Play Services library for registration and message receiving, it probably takes care of these nuances.

rogerhu · 2018-04-14T01:29:50Z

I don't think so...I've seen cases of dup pushes. I'd leave it in.

…

On Fri, Apr 13, 2018, 6:19 PM John Carlson ***@***.***> wrote: For the PushRouter and PushHistory, I think maybe previously there was duplication due to the fact that the Parse Android SDK was registering for GCM push tokens via a custom implementation instead of relying on the Google Play Services version. I would venture to guess that now that we use the Google Play Services library for registration and message receiving, it probably takes care of these nuances. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#800 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT8yRASVff5uPA0F3CjIHXHh7HcshJjks5toU6EgaJpZM4TRRR6> .

Jawnnypoo · 2018-04-14T07:57:07Z

@rogerhu Can you take a look at the maven setup I did within the gradle folder? I am not sure if it is correct. (Latest commit)

Jawnnypoo · 2018-04-17T21:38:24Z

Any ideas why the build may have failed here @rogerhu ? Looks like it is related to the codecov setup?

rogerhu · 2018-04-19T15:43:39Z

@Jawnnypoo no it's the Gradle 3.1.1 upgrade that caused stuff to break. If I downgrade your current stuff seems to build.

rogerhu · 2018-04-19T16:53:36Z

If you do gradlew build --stacktrace:

Caused by: groovy.lang.GroovyRuntimeException: Conflicting module versions. Module [groovy-all is loaded in version 2.4.7 and you are trying to load version 2.4.12

https://issuetracker.google.com/issues/77236219

It seems like com.android.tools.lint:lint-gradle:26.1.1 that adds groovy-all..

Jawnnypoo · 2018-04-25T14:30:17Z

Nice @rogerhu Looks like that worked. Anything else we need to get this merged?

rogerhu · 2018-04-25T14:45:20Z

Can we update the readme with upgrade instructions? (Also the build.gradle should put a big fat warning) Also the parse docs draft too

Is there a way to detect if either package is included and throwing a parse debug log that push is not enabled?

rogerhu · 2018-04-25T22:22:29Z

build.gradle


-    supportLibVersion = '27.0.1'
+    commonLibVersion = "1.17.0"


this should be snapshot

downgrade for now change to snapshot delete this line add back in update tab

Jawnnypoo requested review from rogerhu, flovilmart and natario1 April 12, 2018 07:26

Jawnnypoo commented Apr 12, 2018

View reviewed changes

rogerhu reviewed Apr 12, 2018

View reviewed changes

fcm/README.md

```xml

<service

android:name=".MyFirebaseMessagingService">

Copy link

Contributor

rogerhu Apr 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here too?

rogerhu reviewed Apr 12, 2018

View reviewed changes

rogerhu reviewed Apr 14, 2018

View reviewed changes

flovilmart assigned mmimeault Apr 14, 2018

rogerhu reviewed Apr 25, 2018

View reviewed changes

build.gradle Outdated

supportLibVersion = '27.0.1'

commonLibVersion = "1.17.0"

Copy link

Contributor

rogerhu Apr 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be snapshot

Jawnnypoo added 13 commits April 25, 2018 15:23

Initial FCM setup

3b812ee

Log some things

5dff0a4

compileOnly

8d052df

Update FCM build to pass FCM message to the push receiver

536998d

Move GCM out into its own module

c30c854

Remove obsolete code

4713755

Add notes about calling register

5fd4cf8

Add deprecation notice to GCM module README

d3d4f10

Use global SDK versions

c147359

Update docs and make job tags constants

7a2ca03

Apply common lib version to all modules

a9ca085

downgrade for now change to snapshot delete this line add back in update tab

Remove PushService from manifest

3b3eb99

Apply maven plugin to all libraries

5b9f4ee

rogerhu merged commit 12968d5 into parse-community:master Apr 25, 2018

Jawnnypoo deleted the fcm branch April 30, 2018 04:41

FCM Support - GCM Module #800

FCM Support - GCM Module #800

Conversation

Jawnnypoo commented Apr 12, 2018

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Apr 12, 2018

rogerhu commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jawnnypoo commented Apr 13, 2018

Jawnnypoo commented Apr 13, 2018

rogerhu commented Apr 13, 2018

rogerhu commented Apr 13, 2018

Jawnnypoo commented Apr 13, 2018 • edited Loading

rogerhu commented Apr 13, 2018

Jawnnypoo commented Apr 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogerhu commented Apr 14, 2018

natario1 commented Apr 14, 2018

flovilmart commented Apr 14, 2018

flovilmart commented Apr 14, 2018

Jawnnypoo commented Apr 14, 2018

rogerhu commented Apr 14, 2018 via email

Jawnnypoo commented Apr 14, 2018

Jawnnypoo commented Apr 17, 2018

rogerhu commented Apr 19, 2018

rogerhu commented Apr 19, 2018

Jawnnypoo commented Apr 25, 2018

rogerhu commented Apr 25, 2018

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2018 •

edited

Loading

Jawnnypoo commented Apr 13, 2018 •

edited

Loading