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

Use tunnel manager delegate for changing styles #1489

Merged
merged 8 commits into from
Jun 12, 2018
Merged

Conversation

bsudekum
Copy link
Contributor

As pointed out in #1486, calling styleManager.timeOfDayChanged() is expensive because we have to re-init Solar on every call to check.

Now with this change, we're only changing the style from/to day/night when the tunnel manager tells us we have entered/exited a tunnel.

/cc @mapbox/navigation-ios

@bsudekum
Copy link
Contributor Author

bsudekum commented Jun 10, 2018

Actually, this is still called on every location update:

func checkForTunnelIntersection(at location: CLLocation, for manager: CLLocationManager) {
guard tunnelSimulationEnabled, let tunnelIntersectionManager = tunnelIntersectionManager else { return }
let tunnelDetected = tunnelIntersectionManager.didDetectTunnel(at: location, for: manager, routeProgress: routeProgress)
if tunnelDetected {
tunnelIntersectionManager.delegate?.tunnelIntersectionManager?(manager, willEnableAnimationAt: location)
} else {
tunnelIntersectionManager.delegate?.tunnelIntersectionManager?(manager, willDisableAnimationAt: location)
}
}

We should figure out a way to call these delegate methods once per tunnel.

@@ -586,7 +581,9 @@ extension RouteController: CLLocationManagerDelegate {
self.rawLocation = lastLocation

// Check for a tunnel intersection at the current step we found the bad location update.
checkForTunnelIntersection(at: lastLocation, for: manager)
if tunnelSimulationEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bsudekum we should probably let the TunnelIntersectionManager maintain the status of the tunnelSimulationEnabled. This will definitely come in handy for future maintenance efforts.


// `currentIntersection` is basically the intersection that you have last passed through.
// While the upcoming intersection is the one you will be approaching next.
// The user is essentially always between the current and upcoming intersection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 🎉

@@ -98,7 +109,7 @@ open class TunnelIntersectionManager: NSObject {
isAnimationEnabled = true
}

@objc public func suspendTunnelAnimation(for manager: CLLocationManager, at location: CLLocation, routeController: RouteController) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincethecoder noting, I removed the manager from many functions here as they were unused.

@bsudekum
Copy link
Contributor Author

@1ec5 there are a few name changes that occur in here that would be great to get your eyes on.

@@ -47,31 +47,42 @@ open class TunnelIntersectionManager: NSObject {
/**
The flag that indicates whether simulated location manager is initialized.
*/
@objc public var isAnimationEnabled: Bool = false
@objc var isAnimationEnabled: Bool = false
Copy link
Contributor

Choose a reason for hiding this comment

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

@bsudekum Hmm 🤔 is there a reason why we deleted the public access level declaration here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vincethecoder it's not something the developer should be mutating, nor is it something they should be inspecting if they are using the delegate properly.

Copy link
Contributor

@vincethecoder vincethecoder left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bsudekum bsudekum merged commit bfe0161 into master Jun 12, 2018
@bsudekum bsudekum deleted the use-delegate-tunne branch June 12, 2018 22:53
@1ec5 1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018
@1ec5 1ec5 added this to the v0.18.1 milestone Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants