-
Notifications
You must be signed in to change notification settings - Fork 315
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
Enable night style when inside tunnel #1127
Changes from 18 commits
d2581fc
0a67127
52564d6
2f67660
72fee83
1b0e577
14c5b8e
d764421
dc482f7
1b1152a
19ac74d
f81319b
aadadf4
181b428
5198ce4
b2b0186
05ef98f
bb89fff
613fdfc
8caa01a
c1d95d5
efc7527
256e154
7686d52
b8f8a9e
cf306ca
c7513bf
fb959c8
6915af5
e5058e6
f7463d4
232baa4
64a7897
4e5356a
be3cf6b
18160da
af050d4
e48ad83
3d1ef3f
da0ab08
82c8310
4518a12
49a5817
2b5cd86
f25448f
59b25b3
0b93231
f71f649
8f24774
0b44f03
3675974
2acde88
241bba6
4ad24d0
6ef815c
f0411b8
5ecb164
36a374a
cb8a46b
709231e
70c5e76
9c878c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,7 @@ public protocol RouteControllerDelegate: class { | |
optional func routeController(_ routeController: RouteController, willRerouteFrom location: CLLocation) | ||
|
||
/** | ||
Called when a location has been idenetified as unqualified to navigate on. | ||
Called when a location has been identified as unqualified to navigate on. | ||
|
||
See `CLLocation.isQualified` for more information about what qualifies a location. | ||
|
||
|
@@ -134,6 +134,16 @@ public protocol RouteControllerDelegate: class { | |
*/ | ||
@objc(routeController:didArriveAtWaypoint:) | ||
optional func routeController(_ routeController: RouteController, didArriveAt waypoint: Waypoint) -> Bool | ||
|
||
/** | ||
Called when a tunnel is detected on a route step. | ||
|
||
Implement this method to enable dark mode when a commuter enters a tunnel. | ||
|
||
- parameter darkModeEnabled: The darkModeEnabled flag determines whether a `DarkStyle` should apply. | ||
*/ | ||
@objc(didEnterTunnel:) | ||
optional func didEnterTunnel(_ darkModeEnabled: Bool) | ||
} | ||
|
||
/** | ||
|
@@ -634,6 +644,7 @@ extension RouteController: CLLocationManagerDelegate { | |
updateDistanceToIntersection(from: location) | ||
updateRouteStepProgress(for: location) | ||
updateRouteLegProgress(for: location) | ||
detectRouteStepInTunnel(for: location) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the simplification suggested in #1127 (comment), I think we should actually inline this calculation, much as you had originally. That way:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This would require something slightly different than what #1127 (comment) suggests: perhaps when advancing to a new step, we’d iterate over all the intersections, gathering their distances from the start of the step. That way we’d only need to perform a simple lookup on each location update. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @1ec5 hey I came up with a simplified approach in detecting multiple tunnel intersections on a route step. Please refer to the |
||
|
||
guard userIsOnRoute(location) || !(delegate?.routeController?(self, shouldRerouteFrom: location) ?? true) else { | ||
reroute(from: location) | ||
|
@@ -651,6 +662,27 @@ extension RouteController: CLLocationManagerDelegate { | |
checkForFasterRoute(from: location) | ||
} | ||
|
||
func detectRouteStepInTunnel(for location: CLLocation) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Up to you, but it might make sense to combine this with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention is to make |
||
guard routeProgress.currentLegProgress.currentStep.containsTunnel else { return } | ||
guard let tunnelDistance = routeProgress.currentLegProgress.currentStep.tunnelDistance, let tunnelSlice = routeProgress.currentLegProgress.currentStep.tunnelSlice else { return } | ||
guard let tunnelStartCoordinate = tunnelSlice.coordinates.first, let tunnelEndCoordinate = tunnelSlice.coordinates.last else { return } | ||
|
||
// Calculated distances from current location to tunnel entrance and exit | ||
let currentLocation = CLLocationCoordinate2D(latitude: location.coordinate.latitude, longitude: location.coordinate.longitude) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's necessary to create this variable. |
||
let distanceToEntrance = currentLocation.distance(to: tunnelStartCoordinate) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
let distanceToExit = currentLocation.distance(to: tunnelEndCoordinate) | ||
|
||
// Capute any location which lies within the tunnel route. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: no need for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// Pre-conditions: | ||
// - Current location to exit must be greater than zero meters. | ||
// - Distance to tunnel entrance must be less than tunnel distance. | ||
if (tunnelDistance - distanceToExit) > 0 && distanceToEntrance <= tunnelDistance { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would something like if distanceToEntrance <= tunnelDistance && distanceToExit <= tunnelDistance {
... I only suggest this since it's easier to understand what we're comparing on each side of the && operator. |
||
delegate?.didEnterTunnel?(true) | ||
} else { | ||
delegate?.didEnterTunnel?(false) | ||
} | ||
} | ||
|
||
func updateRouteLegProgress(for location: CLLocation) { | ||
let currentDestination = routeProgress.currentLeg.destination | ||
let legDurationRemaining = routeProgress.currentLegProgress.durationRemaining | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import MapboxDirections | ||
import Turf | ||
|
||
extension RouteStep { | ||
static func ==(left: RouteStep, right: RouteStep) -> Bool { | ||
|
@@ -38,4 +39,46 @@ extension RouteStep { | |
open var lastInstruction: SpokenInstruction? { | ||
return instructionsSpokenAlongStep?.last | ||
} | ||
|
||
/** | ||
Returns true if the current route step contains a tunnel. | ||
*/ | ||
var containsTunnel: Bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This property is unused. |
||
guard let intersections = intersections else { return false } | ||
for intersection in intersections { | ||
if intersection.outletRoadClasses?.contains(.tunnel) == true { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
/** | ||
Returns a tunnel slice for the current route step coordinates | ||
*/ | ||
var tunnelSlice: Polyline? { | ||
guard let coordinates = coordinates, let intersectionBounds = tunnelIntersectionBounds else { return nil } | ||
return Polyline(coordinates).sliced(from: intersectionBounds.entry.location, to: intersectionBounds.exit.location) | ||
} | ||
|
||
/** | ||
Returns the distance of a tunnel slice for the current route step | ||
*/ | ||
var tunnelDistance: CLLocationDistance? { | ||
guard let tunnelSlice = tunnelSlice else { return nil } | ||
return tunnelSlice.distance() | ||
} | ||
|
||
/** | ||
Returns the entry and exit intersection bounds of the tunnel on the current route step | ||
*/ | ||
var tunnelIntersectionBounds: (entry: Intersection, exit: Intersection)? { | ||
guard let intersections = intersections, containsTunnel else { return nil } | ||
for i in 0..<(intersections.count) where intersections.count > 1 { | ||
if intersections[i].outletRoadClasses == .tunnel { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a motorway, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call! Just realized after reviewing a sample JSON structure for a route step with multiple tunnels. |
||
return (entry: intersections[i], exit: intersections[i+1]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Double check that i + 1 is always within bounds before returning here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't line 77 only checking where the number of intersections is greater that 1? Not necessarily that the current index in the loop has a follow on index. |
||
} | ||
} | ||
return nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -561,6 +561,14 @@ extension NavigationViewController: RouteControllerDelegate { | |
} | ||
return advancesToNextLeg | ||
} | ||
|
||
@objc public func didEnterTunnel(_ darkModeEnabled: Bool) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if darkModeEnabled { | ||
self.styleManager.applyStyle(type:.nightStyle) | ||
} else { | ||
self.styleManager.resetTimeOfDayTimer() | ||
} | ||
} | ||
} | ||
|
||
extension NavigationViewController: StyleManagerDelegate { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,6 +96,21 @@ open class StyleManager: NSObject { | |
resetTimeOfDayTimer() | ||
} | ||
|
||
func applyStyle(type styleType: StyleType) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this func be combined with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I considered it. However, this func There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
guard currentStyleType != styleType else { return } | ||
|
||
NSObject.cancelPreviousPerformRequests(withTarget: self, selector: #selector(timeOfDayChanged), object: nil) | ||
|
||
for style in styles { | ||
if style.styleType == styleType { | ||
style.apply() | ||
currentStyleType = styleType | ||
delegate?.styleManager?(self, didApply: style) | ||
break | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's rather unlikely but still possible to split up the styles into something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @frederoni Hmmm... the break can be removed. Since we're only applying a specified There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good now 👍 |
||
} | ||
} | ||
} | ||
|
||
func applyStyle() { | ||
guard let location = delegate?.locationFor(styleManager: self) else { | ||
// We can't calculate sunset or sunrise w/o a location so just apply the first style | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the notion of dark mode from
RouteController
because MapboxCoreNavigation deals with non-UI related functionality.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Will do.