Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

iBeacon plugin support #270

Merged
merged 7 commits into from
Jul 11, 2016
Merged

iBeacon plugin support #270

merged 7 commits into from
Jul 11, 2016

Conversation

justinschuldt
Copy link
Contributor

I didn't want to assign individual functions to the delegate properties, so I assigned functions that return observables. You can subscribe to all the events of the delegate now.

I tested all the methods on iOS and Android

@ihadeed
Copy link
Collaborator

ihadeed commented Jul 7, 2016

Hey

Thanks for the PR.. great work.

Just a question, I see you are using @Cordova({ sync: true }) with a return type of a promise. Does the original plugin's method return a promise?

@justinschuldt
Copy link
Contributor Author

Hey,

This plugin's methods were written to return Q promises, which do not seem to return if the method is wrapped in the wrapPromise() function.

Invoking a method with the synchronous option just returns the result of the plugin's method, which in this case is a promise, so I think that usage is alright?

I'm not sure that setting @Cordova({ sync: true }) is the best way handle these methods. This plugin is written differently than others I have seen. Most of the class methods return promises that resolve when the native layer acknowledges the request. The promises' success does not return any data, it just signals that the request reached the native layer and was understood. The delegate methods are used as callbacks for native events (didEnterRegion, etc).
Using it looks something like this:

this.delegate = IBeacon.Delegate();
this.delegate.didDetermineStateForRegion()
  .subscribe(
    data => console.log('didDetermineStateForRegion: ', data)
  );
this.beaconRegion = IBeacon.BeaconRegion('deskBeacon', 'F7826DA6-ASDF-ASDF-8024-BC5B71E0893E');
IBeacon.startMonitoringForRegion(this.beaconRegion)
  .then(
    result => console.log('request to start monitoring accepted: ', result),
    error => console.error('request to start monitoring failed: ', error)
  );
IBeacon.requestStateForRegion(this.beaconRegion)
  .then(
    result => console.log('request for region state accepted: ', result),
    error => console.error('request for region state failed: ', error)
  );

The code above would output this to the console:

request to start monitoring accepted: OK
request for region state accepted: OK
didDetermineStateForRegion: {"eventType":"didDetermineStateForRegion","region":{"identifier":"deskBeacon","typeName":"BeaconRegion","uuid":"F7826DA6-ASDF-ASDF-8024-BC5B71E0893E"}}

The promises that the class methods return aren't really important, but that's how the author wrote it, so I just tried to match his api. I'm open to suggestions, let me know what you think of all that.

@ihadeed
Copy link
Collaborator

ihadeed commented Jul 11, 2016

Perfect, thanks!

@ihadeed ihadeed merged commit dd97df1 into danielsogl:master Jul 11, 2016
@ihadeed
Copy link
Collaborator

ihadeed commented Jul 17, 2016

Hey @justinschuldt

I just looked at the code again and noticed a problem.

I'm not sure how the Delegate object is supposed to work. From the look of it I think it's not working as you expect it to do so. The methods that return observables are not making sense to me.

I think a better way to implement it is to create a whole new class just for Delegate.

Should be something like this:

export class Delegate {
  private _objectInstance: any;
  constructor(){
    this._objectInstance = new cordova.plugins.locationManager.Delegate();
  }

  didChangeAuthorizationStatus(): Observable<PluginResult> {
    return new Observable<PluginResult>((observer)=>{
      // bind to observer on subscribe
      this._objectInstance.didChangeAuthorizationStatus = observer.next.bind(observer);
      // bind to noop on unsubscribe
      return () => this._objectInstance.didChangeAuthorizationStatus = () => {};
    });
  }

// add other events the same way
}

@ihadeed
Copy link
Collaborator

ihadeed commented Jul 17, 2016

@Ritzlgrmft any thoughts on my comment above ^ ?

@Ritzlgrmft
Copy link
Contributor

@ihadeed At least one important statement is missing in your proposal:

cordova.plugins.locationManager.setDelegate(this._objectInstance);

But this should be called not before all needed events are subscribed. So it would be a little bit ugly to add it to your approach.

An advantage of your approach is the un-subscription of the events. But to be honest, I only subscribe to the events once. I have no need to unsubscribe.

And moreover: Justin's implementation just works.

@ihadeed
Copy link
Collaborator

ihadeed commented Jul 18, 2016

Thanks for the feedback Markus.

I wasn't sure since I don't use this plugin and to be honest I never even
worked with iBecons.

Just wanted to point it out to make sure that the plugin is fully
functional and is implemented the best way possible.

Thanks again!

On Jul 18, 2016 3:28 AM, "Markus Wagner" notifications@github.com wrote:

@ihadeed https://github.com/ihadeed At least one important statement is
missing in your proposal:

cordova.plugins.locationManager.setDelegate(this._objectInstance);

But this should be called not before all needed events are subscribed. So
it would be a little bit ugly to add it to your approach.

An advantage of your approach is the un-subscription of the events. But to
be honest, I only subscribe to the events once. I have no need to
unsubscribe.

And moreover: Justin's implementation just works.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#270 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ANJ8dOv1yjlwN2PnXu6sL_FPef60rHi_ks5qWys7gaJpZM4JFl2o
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants