Merge v2.0.x into master #1736

macdonst · 2017-05-24T14:56:43Z

Hey all,

I want to move the 2.0.x branch into master as I'm getting tired of all of these rebases and I want to release this thing soon. Please take a look over this PR.

I've pushed the latest master branch to v1.x so we can do bug fixes there if required.

* Modified plugin.xml to include FCM changes * Plugin.xml changes for FCM * Java changes for FCM * Increased plugin version to 2.0.0 * Increased plugin version in package.json * Added topic subscription/unsubscription * Removed some empty lines

…s.json

…ction call for action buttons

…-rc3 (#1715)

fredgalvao · 2017-05-24T23:19:15Z

One of the forks of the fcm-node package now mentions the existence of a module officialy from Firebase that provides the functionality offered by fcm-node:

Warning: on February 2, 2017, the Firebase Team released the admin.messaging() service to their node.js admin module. This new service makes this module kind of deprecated (source)

Should the examples on payload be changed accordingly?

Docs on the new official node.js API is here

fredgalvao · 2017-05-24T23:23:17Z

Should we subtly advise use of let instead of var on our examples? Both our codebase and the internet has moved on to a new level of ES now, it feels right to me (but not needed, of course).

shazron · 2017-05-24T23:28:01Z

@fredgalvao ES6 is only baked in on iOS 10, unfortunately (without transpilers). cordova-ios still supports ios 9.

fredgalvao · 2017-05-24T23:28:54Z

@shazron The examples are in the context of a node.js server to send the payloads, not in the webview itself.

shazron · 2017-05-24T23:32:31Z

@fredgalvao gotcha, I thought you were talking about the client side example

fredgalvao · 2017-05-24T23:55:41Z

I can't read Objective-C without it hurting my brain, so I can't help on that.

Overall, there seems to be a few merge-originated regressions on plugin.xml and that one TODO that has apparently left the registration event without it's tokenRefresh counterpart on the native side.

Most of the stuff I commented on on the docs are chore, and can be left for another PR.

tlaverdure · 2017-05-25T02:29:19Z

I'm getting this error when submitting to the Apple App store:

Non-public API usage:

The app references non-public selectors in: _terminateWithStatus:

Found in:
platforms/ios/Pods/GoogleToolboxForMac/UnitTesting/GTMIPhoneUnitTestDelegate.m

shazron · 2017-05-25T07:05:52Z

@tlaverdure Looks like we can't include the full GTM suite, and just use subspecs of it. We will have to discover which ones, but definitely none of the UnitTesting ones.

$ pod search GoogleToolboxForMac
> GoogleToolboxForMac (2.1.1)
   Google utilities for iOS and OSX development.
   pod 'GoogleToolboxForMac', '~> 2.1.1'
   - Homepage: https://github.com/google/google-toolbox-for-mac
   - Source:   https://github.com/google/google-toolbox-for-mac.git
   - Versions: 2.1.1, 2.1.0, 2.0.1, 2.0.0 [master repo]
   - Subspecs:
     - GoogleToolboxForMac/Defines (2.1.1)
     - GoogleToolboxForMac/Core (2.1.1)
     - GoogleToolboxForMac/AddressBook (2.1.1)
     - GoogleToolboxForMac/DebugUtils (2.1.1)
     - GoogleToolboxForMac/GeometryUtils (2.1.1)
     - GoogleToolboxForMac/KVO (2.1.1)
     - GoogleToolboxForMac/Logger (2.1.1)
     - GoogleToolboxForMac/Regex (2.1.1)
     - GoogleToolboxForMac/StackTrace (2.1.1)
     - GoogleToolboxForMac/StringEncoding (2.1.1)
     - GoogleToolboxForMac/SystemVersion (2.1.1)
     - GoogleToolboxForMac/URLBuilder (2.1.1)
     - GoogleToolboxForMac/NSData+zlib (2.1.1)
     - GoogleToolboxForMac/NSDictionary+URLArguments (2.1.1)
     - GoogleToolboxForMac/NSFileHandle+UniqueName (2.1.1)
     - GoogleToolboxForMac/NSScanner+JSON (2.1.1)
     - GoogleToolboxForMac/NSString+HTML (2.1.1)
     - GoogleToolboxForMac/NSString+URLArguments (2.1.1)
     - GoogleToolboxForMac/NSString+XML (2.1.1)
     - GoogleToolboxForMac/NSThread+Blocks (2.1.1)
     - GoogleToolboxForMac/iPhone (2.1.1)
     - GoogleToolboxForMac/RoundedRectPath (2.1.1)
     - GoogleToolboxForMac/UIFont+LineHeight (2.1.1)
     - GoogleToolboxForMac/UnitTesting (2.1.1)
     - GoogleToolboxForMac/UnitTestingAppLib (2.1.1)

Looks like it could just be Logger: https://github.com/CocoaPods/Specs/blob/23b07bd380c1e829caf2663182e3be87ace02785/Specs/2/d/6/FirebaseMessaging/1.2.1/FirebaseMessaging.podspec.json#L35

shazron · 2017-05-25T07:36:23Z

I removed the <framework> GoogleToolboxForMac in plugin.xml and it appears that FirebaseMessaging will download the appropriate dependency in platforms/ios/Pods so GTM is there already. This might be an issue in the workspace settings possibly. Strange that including the pod explicitly makes it work.

Lots of similar issues: https://www.google.com/search?q=library+not+found+for+lGoogleToolboxForMac&oq=library+not+found+for+lGoogleToolboxForMac&aqs=chrome..69i57.3654j0j7&sourceid=chrome&ie=UTF-8

shazron · 2017-05-25T08:07:46Z

I tested this again on the v2.0.x plugin without the GoogleToolboxForMac <framework> tag and it builds fine, so the original problem disappeared? Weird. Using Xcode 8.3.2. Note that it installs FirebaseMessaging 1.2.2 due to the way the spec is specified. Not sure why it didn't pick up 1.2.3

PODS:
  - FirebaseAnalytics (3.7.0):
    - FirebaseCore (~> 3.5)
    - FirebaseInstanceID (~> 1.0)
    - GoogleToolboxForMac/NSData+zlib (~> 2.1)
  - FirebaseCore (3.5.1):
    - GoogleToolboxForMac/NSData+zlib (~> 2.1)
  - FirebaseInstanceID (1.0.9)
  - FirebaseMessaging (1.2.2):
    - FirebaseAnalytics (~> 3.7)
    - FirebaseInstanceID (~> 1.0)
    - GoogleToolboxForMac/Logger (~> 2.1)
    - Protobuf (~> 3.1)
  - GoogleToolboxForMac/Defines (2.1.1)
  - GoogleToolboxForMac/Logger (2.1.1):
    - GoogleToolboxForMac/Defines (= 2.1.1)
  - GoogleToolboxForMac/NSData+zlib (2.1.1):
    - GoogleToolboxForMac/Defines (= 2.1.1)
  - Protobuf (3.2.0)

DEPENDENCIES:
  - FirebaseMessaging (~> 1.2.1)

SPEC CHECKSUMS:
  FirebaseAnalytics: 0d1b7d81d5021155be37702a94ba1ec16d45365d
  FirebaseCore: 225d40532489835a034b8f4e2c9c87fbf4f615a2
  FirebaseInstanceID: 2d0518b1378fe9d685ef40cbdd63d2fdc1125339
  FirebaseMessaging: df8267f378580a24174ce7861233aa11d5c90109
  GoogleToolboxForMac: 8e329f1b599f2512c6b10676d45736bcc2cbbeb0
  Protobuf: 745f59e122e5de98d4d7ef291e264a0eef80f58e

PODFILE CHECKSUM: 61ad6041bfe0d7b053a1f23890ab2fd59004e3c7

COCOAPODS: 1.2.0

jcesarmobile · 2017-05-25T08:19:59Z

I was planning on testing FirebaseMessaging 2.0.0

macdonst · 2017-05-25T11:51:55Z

@fredgalvao can you create an issue to update the docs for ES6 and using the supported way of sending FCM messages from node? Tag it to the v2.0.0 release.

macdonst · 2017-05-25T20:40:41Z

Okay master is now officially the 2.0.0 working branch and v1.x is where we'll do bug fixes for older releases.

fredgalvao · 2017-05-24T23:05:22Z

docs/API.md

 `android.icon` | `string` | | Optional. The name of a drawable resource to use as the small-icon. The name should not include the extension.
 `android.iconColor` | `string` | | Optional. Sets the background color of the small icon on Android 5.0 and greater. [Supported Formats](http://developer.android.com/reference/android/graphics/Color.html#parseColor(java.lang.String))
 `android.sound` | `boolean` | `true` | Optional. If `true` it plays the sound specified in the push data or the default system sound.
 `android.vibrate` | `boolean` | `true` | Optional. If `true` the device vibrates on receipt of notification.
 `android.clearBadge` | `boolean` | `false` | Optional. If `true` the icon badge will be cleared on init and before push messages are processed.
 `android.clearNotifications` | `boolean` | `true` | Optional. If `true` the app clears all pending notifications when it is closed.
 `android.forceShow` | `boolean` | `false` | Optional. Controls the behavior of the notification when app is in foreground. If `true` and app is in foreground, it will show a notification in the notification drawer, the same way as when the app is in background (and `on('notification')` callback will be called *only when the user clicks the notification*). When `false` and app is in foreground, the `on('notification')` callback will be called immediately.
-`android.topics` | `array` | `[]` | Optional. If the array contains one or more strings each string will be used to subscribe to a GcmPubSub topic. Note: you should omit the `/topics/` prefix from each element of the array as the plugin will handle that for you.
+`android.topics` | `array` | `[]` | Optional. If the array contains one or more strings each string will be used to subscribe to a FcmPubSub topic.
+`/topics/` prefix from each element of the array as the plugin will handle that for you.


This looks like an accidental line break. (edit) Or maybe this line was supposed to be removed too?

It was supposed to be removed. I will fix this in a commit shortly.

fredgalvao · 2017-05-24T23:07:26Z

docs/API.md

-`ios.topics` | `array` | `[]` | Optional. If the array contains one or more strings each string will be used to subscribe to a GcmPubSub topic. Note: only usable in conjunction with `senderID`. Note: you should omit the `/topics/` prefix from each element of the array as the plugin will handle that for you.
+`ios.fcmSandbox` | `boolean` | `false` | Whether to use prod or sandbox GCM setting.  Defaults to false.
+options
+`ios.topics` | `array` | `[]` | Optional. If the array contains one or more strings each string will be used to subscribe to a FcmPubSub topic.


Either the /topics thing needs to come back here, or it needs to go away on the android section above.

fredgalvao · 2017-05-24T23:08:59Z

docs/INSTALLATION.md

-If you are not creating an Android application you can put in anything for this value.
+```
+<platform name="android">
+  <resource-file src="google-services.json" target="google-services.json" />


Is this target needed in this example?

Doesn't hurt :)

fredgalvao · 2017-05-24T23:21:05Z

docs/PAYLOAD.md

+        title: 'AUX Scrum',
+        message: 'Scrum: Daily touchbase @ 10am Please be on time so we can cover everything on the agenda.',
+        actions: [
+            { "icon": "emailGuests", "title": "EMAIL GUESTS", "callback": "app.emailGuests", "foreground": true},


No need to quote those nested keys, might as well leave unquoted for homogeneity.

fredgalvao · 2017-05-24T23:24:23Z

docs/PAYLOAD.md

+    data: {
+        title: 'Force Start',
+        message: 'This notification should restart the app',
+        force-start: '1'


This key actually needs to be quoted, otherwise this won't compile.

fredgalvao · 2017-05-24T23:27:06Z

package.json

-        "cordova": ">100"
+        "cordova-ios": ">=4.4.0",
+        "cordova-android": ">=6.2.1",
+        "cordova": ">=6.4.0"
      }


Having a block pointing to the distant future "cordova": ">100" is healthy afaik, we should create a new one for 3.0 or 2.1 on that.

fredgalvao · 2017-05-24T23:34:33Z

plugin.xml

  <platform name="android">
+    <hook type="before_plugin_install" src="scripts/copyAndroidFile.js"/>


Do we still need this? If I read that script right, it only adds a framework dependency, which can be done through framework tags themselves.

Yeah, it's still needed as the classpath "com.google.gms:google-services:3.0.0" has to end up in the buildscript dependencies section. This will screw us up for PGB.

Isn't this something that can be achieved with a custom framework tag to gradle?

fredgalvao · 2017-05-24T23:39:51Z

plugin.xml

    </config-file>
    <config-file target="AndroidManifest.xml" parent="/manifest/application">
-      <activity android:name="com.adobe.phonegap.push.PushHandlerActivity" android:exported="true" android:permission="${applicationId}.permission.PushHandlerActivity"/>
+      <activity android:name="com.adobe.phonegap.push.PushHandlerActivity" android:exported="true"/>


This might be a bit too late or mistaken, but why was the custom permission removed? It fixed conflicts of duplicated providers when multiple applications with the same plugin were present, and afaik that issue can still happen on 2.0.

fredgalvao · 2017-05-24T23:44:15Z

plugin.xml

    <js-module src="src/windows/PushPluginProxy.js" name="PushPlugin">
-      <runs/>
+      <merges target=""/>


This and the setToastCapable above seem to have been lost accidentally. Losing those changes are a regression afaik.

fredgalvao · 2017-05-24T23:49:03Z

src/android/com/adobe/phonegap/push/PushInstanceIDListenerService.java

+        String refreshedToken = FirebaseInstanceId.getInstance().getToken();
+        Log.d(LOG_TAG, "Refreshed token: " + refreshedToken);
+        // TODO: Implement this method to send any registration to your app's servers.
+        //sendRegistrationToServer(refreshedToken);


Is this TODO supposed to have survived? How is the tokenRefresh event being cascaded into webview-land now?

Yeah, that todo should stick around for now. We should fire the registration event if this happens.

By "we should" you mean when this TODO gets implemented, right? I'm trying to understand what's happening and what should happen with this TODO in place. Otherwise we need to update the docs to mention that tokenRefresh won't trigger a registration event anymore.

fredgalvao · 2017-05-25T20:44:45Z

I misunderstood how github reviews acted on the comments left on changes, and it seems all the comments I had made were being held until I submitted the review. Sorry for that.

@macdonst can you take a look at them? It's too late with the merge, but we can always create new issues for the ones that are agreed upon.

macdonst · 2017-05-25T21:12:53Z

@fredgalvao I went through your comments. Lots of good catches there, thanks! Will push a commit in a moment.

lock · 2018-06-03T01:34:44Z

This thread has been automatically locked.

jcesarmobile and others added 30 commits May 23, 2017 12:10

Added partial Android FCM support (#975)

aca6011

* Modified plugin.xml to include FCM changes * Plugin.xml changes for FCM * Java changes for FCM * Increased plugin version to 2.0.0 * Increased plugin version in package.json * Added topic subscription/unsubscription * Removed some empty lines

Fixed empty token on android first run (#1008)

06aa1a7

Removed GCM files and added FCM files

c2f4954

Added .framework files as custom frameworks

18b2723

Changed code to work with FCM

458a025

Added hook and resource file to copy GoogleService-Info.plist

5635651

fixed registration and removed unused code

32333ce

Use CocoaPods

4bbc818

Fix rebase errors

684a07c

Fix topic subscription and unsubscription on FCM

0a0521f

Refactor GCMIntentService to FCMService

67069af

Issue #689: Remove sender id from PushNotification init Android options

bef73be

Issue #689: Remove sender id from PushNotification init iOS options

c8394e2

📝 Update to using fcm-node

d0bc2a8

🐛 Issue #1188: Strings.xml google_app_id conflict with google-service…

4645efc

…s.json

Add empty google services plist

4270a50

Pin FCM to 9.8.0

531de4e

🔧 Add browser platform back for FCM branch

8544202

🔨🔧⬆️ Use Babel to transpile ES2015 code

00a38d7

🔧 add tern to gitignore

52d54c4

🔖 Bumping plugin version to 2.0.0-rc1

6df9706

2.0.0-rc1

368de77

Updating CHANGELOG

6c58cee

Fixing my merge error for PR #1378 Optional event emit instead of fun…

706dcbf

…ction call for action buttons

🐛🍎 Issue #1461: App crashes when initialising with topics (v2.0.0-rc1)

dbe3964

🐛🐧📝⬆️ #1460: Build Issue Android

41be377

🐛🍎🔧 Fixing a merge issue where aps-environment was accidentally removed

96d6bc7

🐧📝🐛 Issue #1470: Cannot install phonegap-plugin-push on master

107e600

📝 Using a newer version on the examples

1d05980

🍎🐛 Issue #1497: App crashes after refreshing when using FCM (v2)

5ee535a

macdonst and others added 3 commits May 24, 2017 10:53

Updating CHANGELOG

6e9ea9a

Fixes 'framework not found GoogleToolboxForMac' linker error in 2.0.0…

e5739b8

…-rc3 (#1715)

Remove unused files (#1732)

09344ef

macdonst requested review from shazron, fredgalvao, jcesarmobile and surajpindoria May 24, 2017 14:56

shazron mentioned this pull request May 25, 2017

Remove GTM framework. #1739

Merged

3 tasks

fredgalvao mentioned this pull request May 25, 2017

Keep docs up-to-date (es6, fcm-node, json/quote) #1740

Open

Remove GTM framework. (#1739)

c2b15d8

macdonst merged commit d07fdf0 into master May 25, 2017

fredgalvao reviewed May 25, 2017

View reviewed changes

pwbs mentioned this pull request May 26, 2017

Non-public API usage: The app references non-public selectors: _terminateWithStatus google/google-toolbox-for-mac#138

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge v2.0.x into master #1736

Merge v2.0.x into master #1736

macdonst commented May 24, 2017

fredgalvao commented May 24, 2017

fredgalvao commented May 24, 2017

shazron commented May 24, 2017

fredgalvao commented May 24, 2017

shazron commented May 24, 2017

fredgalvao commented May 24, 2017

tlaverdure commented May 25, 2017

shazron commented May 25, 2017 •

edited

shazron commented May 25, 2017 •

edited

shazron commented May 25, 2017

jcesarmobile commented May 25, 2017

macdonst commented May 25, 2017

macdonst commented May 25, 2017

fredgalvao May 24, 2017

macdonst May 25, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

macdonst May 25, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

macdonst May 25, 2017

fredgalvao May 25, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

fredgalvao May 24, 2017

macdonst May 25, 2017

fredgalvao May 25, 2017

fredgalvao commented May 25, 2017 •

edited

macdonst commented May 25, 2017

lock bot commented Jun 3, 2018

		<platform name="android">
		<hook type="before_plugin_install" src="scripts/copyAndroidFile.js"/>

Merge v2.0.x into master #1736

Merge v2.0.x into master #1736

Conversation

macdonst commented May 24, 2017

fredgalvao commented May 24, 2017

fredgalvao commented May 24, 2017

shazron commented May 24, 2017

fredgalvao commented May 24, 2017

shazron commented May 24, 2017

fredgalvao commented May 24, 2017

tlaverdure commented May 25, 2017

shazron commented May 25, 2017 • edited

shazron commented May 25, 2017 • edited

shazron commented May 25, 2017

jcesarmobile commented May 25, 2017

macdonst commented May 25, 2017

macdonst commented May 25, 2017

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

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

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

Choose a reason for hiding this comment

fredgalvao commented May 25, 2017 • edited

macdonst commented May 25, 2017

lock bot commented Jun 3, 2018

shazron commented May 25, 2017 •

edited

shazron commented May 25, 2017 •

edited

fredgalvao commented May 25, 2017 •

edited