Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Simplification #17

Closed
wants to merge 7 commits into from
Closed

Simplification #17

wants to merge 7 commits into from

Conversation

edewit
Copy link
Member

@edewit edewit commented Feb 28, 2014

as discussed on the mailing list a simplification of the push plugin API

var pushConfig = {
    pushServerURL: "<pushServerURL e.g http(s)//host:port/context >",
    alias: "<alias e.g. a username or an email address optional>",
    android: {
      senderID: "<senderID e.g Google Project ID only for android>",
      variantID: "<variantID e.g. 1234456-234320>",
      variantSecret: "<variantSecret e.g. 1234456-234320>"
    },
    ios: {
      variantID: "<variantID e.g. 1234456-234320>",
      variantSecret: "<variantSecret e.g. 1234456-234320>"
    }
};
@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Looks like the README needs to be updated

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Or the real documentation...

@edewit
Copy link
Member Author

edewit commented Mar 11, 2014

moving the readme to the site was part of aerogear/aerogear.org#264 and #16

@sebastienblanc
Copy link
Member

And does these PR reflects the changes that are present in this PR ?

On Tue, Mar 11, 2014 at 10:50 AM, Erik Jan de Wit
notifications@github.comwrote:

moving the readme to the site was part of aerogear/aerogear.org#264https://github.com/aerogear/aerogear.org/pull/264and
#16 #16

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-37278356
.

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

moving the readme to the site was part of aerogear/aerogear.org#264

Oh, ok - was not aware of that PR, while reading this one

@edewit
Copy link
Member Author

edewit commented Mar 11, 2014

No, we need to update the documentation once we release and include a migration document as well

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Looks like a link to this file is missing from the guides index file:

http://staging.aerogear.org/docs/guides/aerogear-cordova/AerogearCordovaPush/

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Reading the referenced discussion thread, from this PR, I think this (inside of the staged documentation) is now incorrect:

//badge and sound are iOS specific, and ignored on Android push.register(successHandler, errorHandler, {"badge": "true", "sound": "true", "ecb": "onNotification", pushConfig: pushConfig});

Reading the thread, I have the impression the following is the latest:

``
var pushConfig = {
pushServerURL: ".....",
alias: ".....",
android: {},
iOS: {}
};

    push.register(onNotification, errorHandler, pushConfig);

``

But that can be easily fixed in another documentation PR

@edewit
Copy link
Member Author

edewit commented Mar 11, 2014

This document is linked from the guides index file under User Guides > AeroGear Cordova PushPlugin and from Home > Cordova > Learn More

@edewit
Copy link
Member Author

edewit commented Mar 11, 2014

Like I said before we need to update the documentation for this change in API once we release a new version the documentation is still referencing the current release!

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

@ doc link: Awesome! I guess I just missed it

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

The following index.js file is failing (for Android)

https://gist.github.com/matzew/dabc10c19d0e146dfae1

it causes the alert("Something went wrong"); to be invoked

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

very funny;
re-running (cordova android run) now makes it work

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Tried a few more apps: they all work and did register w/ the UPS.

However on the first two apps, I got the errorhandler be invoked with "SERVICE_NOT_AVAILABLE"
-> Re-running the same app made it go away.

Apps nr. 3,4,5 worked all immediately out of the box

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

👍

let's :shipit:

@matzew
Copy link
Contributor

matzew commented Mar 11, 2014

Not sure this is a good test idea, but I gave it a shot on iOS 7.1 (w/ Xcode 5.1)

The build reports these errors:


Ld /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/jug.app/jug normal arm64
    cd /Users/matzew/Work/Conferences/2014/Examples/Simple6/platforms/ios
    export IPHONEOS_DEPLOYMENT_TARGET=5.0
    export PATH="/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/usr/bin:/Applications/Xcode.app/Contents/Developer/usr/bin:/usr/bin:/bin:/usr/sbin:/sbin"
    /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS7.1.sdk -L/Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos -L/Users/matzew/Work/Conferences/2014/Examples/Simple6/platforms/ios/jug/Plugins/org.jboss.aerogear.cordova.push -F/Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos -filelist /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Intermediates/jug.build/Debug-iphoneos/jug.build/Objects-normal/arm64/jug.LinkFileList -dead_strip -weak_framework CoreFoundation -weak_framework UIKit -weak_framework AVFoundation -weak_framework CoreMedia -weak-lSystem -force_load /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/libCordova.a -ObjC -fobjc-arc -fobjc-link-runtime -miphoneos-version-min=5.0 -framework AssetsLibrary /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/libCordova.a -framework CoreGraphics -framework MobileCoreServices -framework CoreLocation -lpush-sdk -framework Security -framework Foundation -framework SystemConfiguration -Xlinker -dependency_info -Xlinker /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Intermediates/jug.build/Debug-iphoneos/jug.build/Objects-normal/arm64/jug_dependency_info.dat -o /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/jug.app/jug

ld: warning: ignoring file /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/libCordova.a, missing required architecture arm64 in file /Users/matzew/Library/Developer/Xcode/DerivedData/jug-bgfghezxgbxgroemfrugdjxyrusi/Build/Products/Debug-iphoneos/libCordova.a (2 slices)
ld: warning: ignoring file /Users/matzew/Work/Conferences/2014/Examples/Simple6/platforms/ios/jug/Plugins/org.jboss.aerogear.cordova.push/libpush-sdk.a, missing required architecture arm64 in file /Users/matzew/Work/Conferences/2014/Examples/Simple6/platforms/ios/jug/Plugins/org.jboss.aerogear.cordova.push/libpush-sdk.a (3 slices)
Undefined symbols for architecture arm64:
  "_OBJC_METACLASS_$_CDVPlugin", referenced from:
      _OBJC_METACLASS_$_PushPlugin in PushPlugin.o
  "_OBJC_CLASS_$_AGDeviceRegistration", referenced from:
      objc-class-ref in PushPlugin.o
  "_OBJC_CLASS_$_CDVViewController", referenced from:
      _OBJC_CLASS_$_MainViewController in MainViewController.o
  "_CDVLocalNotification", referenced from:
      -[AppDelegate application:didReceiveLocalNotification:] in AppDelegate.o
  "_OBJC_METACLASS_$_CDVCommandDelegateImpl", referenced from:
      _OBJC_METACLASS_$_MainCommandDelegate in MainViewController.o
  "_OBJC_CLASS_$_CDVPluginResult", referenced from:
      objc-class-ref in PushPlugin.o
  "_OBJC_CLASS_$_CDVCommandDelegateImpl", referenced from:
      _OBJC_CLASS_$_MainCommandDelegate in MainViewController.o
  "_OBJC_CLASS_$_CDVCommandQueue", referenced from:
      _OBJC_CLASS_$_MainCommandQueue in MainViewController.o
  "_OBJC_CLASS_$_CDVPlugin", referenced from:
      _OBJC_CLASS_$_PushPlugin in PushPlugin.o
  "_OBJC_METACLASS_$_CDVViewController", referenced from:
      _OBJC_METACLASS_$_MainViewController in MainViewController.o
  "_OBJC_METACLASS_$_CDVCommandQueue", referenced from:
      _OBJC_METACLASS_$_MainCommandQueue in MainViewController.o
  "_CDVPluginHandleOpenURLNotification", referenced from:
      -[AppDelegate application:handleOpenURL:] in AppDelegate.o
ld: symbol(s) not found for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)


@keithdmoore
Copy link
Contributor

Sorry, I know I am late to this party. Just saw the discussion thread for these changes.
As for the removal of the register success handler, I think it should stay. For me, I want to make a call to my backend to associate the alias to the user. I dont want to assume that everything was fine. Having the success/error handlers is pretty standard.

I love the rest of the changes. Great simplifications.

@matzew
Copy link
Contributor

matzew commented Mar 13, 2014

@success handler: fair point

On Thursday, March 13, 2014, Keith D. Moore notifications@github.com
wrote:

Sorry, I know I am late to this party. Just saw the discussion thread for
these changes.

As for the removal of the register success handler, I think it should
stay. For me, I want to make a call to my backend to associate the alias to
the user. I dont want to assume that everything was fine. Having the
success/error handlers is pretty standard.

I love the rest of the changes. Great simplifications.

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-37493855
.

Sent from Gmail Mobile

@sebastienblanc
Copy link
Member

Yes, this is a valid use case for the success handler

@edewit
Copy link
Member Author

edewit commented Mar 13, 2014

@matzew seems like there are issues with cordova and xcode 5.1
http://shazronatadobe.wordpress.com/2014/03/12/xcode-5-1-and-cordova-ios/

@matzew
Copy link
Contributor

matzew commented Mar 13, 2014

yep, saw the blog last night already

On Thu, Mar 13, 2014 at 11:39 AM, Erik Jan de Wit
notifications@github.comwrote:

@matzew https://github.com/matzew seems like there are issues with
cordova and xcode 5.1
http://shazronatadobe.wordpress.com/2014/03/12/xcode-5-1-and-cordova-ios/

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-37519041
.

Matthias Wessendorf

blog: http://matthiaswessendorf.wordpress.com/
sessions: http://www.slideshare.net/mwessendorf
twitter: http://twitter.com/mwessendorf

@keithdmoore
Copy link
Contributor

How did you guys find out about this? The hard way? I am reverting back to 5.0.2. Thanks for sharing this!

On Mar 13, 2014, at 5:40 AM, Matthias Wessendorf notifications@github.com wrote:

yep, saw the blog last night already

On Thu, Mar 13, 2014 at 11:39 AM, Erik Jan de Wit
notifications@github.comwrote:

@matzew https://github.com/matzew seems like there are issues with
cordova and xcode 5.1
http://shazronatadobe.wordpress.com/2014/03/12/xcode-5-1-and-cordova-ios/

Reply to this email directly or view it on GitHubhttps://github.com//pull/17#issuecomment-37519041
.

Matthias Wessendorf

blog: http://matthiaswessendorf.wordpress.com/
sessions: http://www.slideshare.net/mwessendorf
twitter: http://twitter.com/mwessendorf

Reply to this email directly or view it on GitHub.

@sebastienblanc
Copy link
Member

@keithdmoore in the blog that @edewit mentionned some workarounds mentioned. In ther sametime there is some pressure on Cordova's mailing list to ship quickly a 3.4.1 version that contains some fixes.

@matzew
Copy link
Contributor

matzew commented Mar 13, 2014

Hey @keithdmoore

How did you guys find out about this?

Via Twitter :-) I follow @brianleroux there - and he retweeted this:
https://twitter.com/shazron/status/443620588779630592

@matzew
Copy link
Contributor

matzew commented Mar 18, 2014

Should we merge this now ?

@edewit
Copy link
Member Author

edewit commented Mar 18, 2014

Let's add the success handler again, seems that it's needed

@edewit
Copy link
Member Author

edewit commented Mar 19, 2014

Now added the successCallback again API looks like

push.register(onNotification, successCallback, errorHandler, pushConfig);

function successCallback() {
  console.log('registration successful');
}

@keithdmoore
Copy link
Contributor

Great

Pecked out from my iPhone

On Mar 19, 2014, at 4:08 AM, Erik Jan de Wit notifications@github.com wrote:

Now added the successCallback again API looks like

push.register(onNotification, successCallback, errorHandler, pushConfig);

function successCallback() {
console.log('registration successful');
}

Reply to this email directly or view it on GitHub.

@sebastienblanc
Copy link
Member

Is this PR good to merge ?
I can't wait to have the onNotification being a "real" function and not a String anymore.

@edewit
Copy link
Member Author

edewit commented Mar 24, 2014

landed 9d16f15

@edewit edewit closed this Mar 24, 2014
@edewit edewit deleted the simplification branch March 24, 2014 16:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants