Skip to content

Added LocationManagerProtocol to support macOS and Linux#4

Open
carlos21 wants to merge 5 commits intofeature/services/gatt-servicesfrom
feature/services/add-indoor-positioning-service
Open

Added LocationManagerProtocol to support macOS and Linux#4
carlos21 wants to merge 5 commits intofeature/services/gatt-servicesfrom
feature/services/add-indoor-positioning-service

Conversation

@carlos21
Copy link
Copy Markdown
Member

@carlos21 carlos21 commented Jul 5, 2018

Take a quick look please, just to check if the implementation is going well. It would be awesome if we can separate this and create a LocationKit (or something like that) for macOS, watchOS, tvOS, iOS and Linux xD

@carlos21 carlos21 self-assigned this Jul 5, 2018
@carlos21 carlos21 requested a review from colemancda July 5, 2018 01:25
Comment thread Sources/gattserver/DarwinLocation.swift Outdated

public class DarwinLocation: NSObject, LocationManagerProtocol {

internal private(set) var internalManager: CLLocationManager!
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be let since this will never change

self.locationManager = LocationManager()

#if os(macOS)
let serviceUUID = BluetoothUUID()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is wrong, it should only be done on characteristics CoreBluetooth has blacklisted internally (because of collision with the ones a Mac advertises).

Comment thread Sources/gattserver/Location.swift Outdated

extension LocationManagerProtocol {

public var didUpdateLocation: ((Double, Double) -> Void)? {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No reason to provide default implementation, we want compiler error if this is not implemented

Comment thread Sources/gattserver/Location.swift Outdated

public protocol LocationManagerProtocol: class {

var didUpdateLocation: ((Double, Double) -> Void)? { get set }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We need a struct like CLLocation

Comment thread Sources/gattserver/Location.swift Outdated

var didUpdateLocation: ((Double, Double) -> Void)? { get set }

var locationServicesEnabled: Bool { get }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should only exist on the Darwin version, and in any case should be isEnabled

Comment thread Sources/gattserver/DarwinLocation.swift Outdated
else { assertionFailure("Array always contains at least one object representing the current location."); return }

didUpdateLocation?(location.coordinate.latitude, location.coordinate.longitude)
self.locationManager?.accessQueue.async { [weak self] in
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Don't you need to use here self.locationManager?.accessQueue.async(flags: .barrier)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sure, improve as you see fit.

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.

2 participants