-
Notifications
You must be signed in to change notification settings - Fork 310
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 11 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,17 @@ public protocol RouteControllerDelegate: class { | |
*/ | ||
@objc(routeController:didArriveAtWaypoint:) | ||
optional func routeController(_ routeController: RouteController, didArriveAt waypoint: Waypoint) -> Bool | ||
|
||
/** | ||
Called when a location is detected within a tunnel. | ||
|
||
Implement this method to enable dark mode when a commuter enters a tunnel. | ||
|
||
- parameter routeController: The route controller that detects the location within a tunnel route. | ||
- parameter location: The location that falls within the identified tunnel. | ||
*/ | ||
@objc(routeController:didEnterTunnelAt:) | ||
optional func routeController(_ routeController: RouteController, didEnterTunnelAt location: CLLocation?) | ||
} | ||
|
||
/** | ||
|
@@ -634,6 +645,7 @@ extension RouteController: CLLocationManagerDelegate { | |
updateDistanceToIntersection(from: location) | ||
updateRouteStepProgress(for: location) | ||
updateRouteLegProgress(for: location) | ||
detectRouteStepInTunnel(for: location) | ||
|
||
guard userIsOnRoute(location) || !(delegate?.routeController?(self, shouldRerouteFrom: location) ?? true) else { | ||
reroute(from: location) | ||
|
@@ -651,6 +663,28 @@ 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. | ||
// - Distance to tunnel exit must also be less than tunnel distance. | ||
if (tunnelDistance - distanceToExit) > 0 && distanceToEntrance <= tunnelDistance && distanceToExit <= 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. Do you think it's possible to have only 2 conditions 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. @bsudekum Yes indeed. Good looks 👍 |
||
delegate?.routeController?(self, didEnterTunnelAt: location) | ||
} else { | ||
delegate?.routeController?(self, didEnterTunnelAt: nil) | ||
} | ||
} | ||
|
||
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 tunnelIntersection = tunnelIntersectionBounds else { return nil } | ||
return Polyline(coordinates).sliced(from: tunnelIntersection.entry.location, to: tunnelIntersection.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 bounss 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 routeController(_ routeController: RouteController, didEnterTunnelAt location: CLLocation?) { | ||
if let _ = 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. The fact we're not using 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. Sure! |
||
self.styleManager.applyStyle(NightStyle()) | ||
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. Do you think we could first check if the style needs to be updated before continually updating the style on every 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. @bsudekum I do a safety check before I apply the style in |
||
} else { | ||
self.styleManager.applyStyle() | ||
} | ||
} | ||
} | ||
|
||
extension NavigationViewController: StyleManagerDelegate { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,7 +96,7 @@ open class StyleManager: NSObject { | |
resetTimeOfDayTimer() | ||
} | ||
|
||
func applyStyle() { | ||
func applyStyle(_ newStyle: Style? = nil) { | ||
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. We should be able to avoid the special case for custom style below if we’d take a 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 you're right 👍 |
||
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 | ||
if let style = styles.first { | ||
|
@@ -106,6 +106,15 @@ open class StyleManager: NSObject { | |
return | ||
} | ||
|
||
// Custom style usage | ||
if let customStyle = newStyle { | ||
if styles.count == 1 && !styles.contains(customStyle) || styles.count > 1 && styles.first != customStyle { | ||
customStyle.apply() | ||
delegate?.styleManager?(self, didApply: customStyle) | ||
} | ||
return | ||
} | ||
|
||
// Single style usage | ||
guard styles.count > 1 else { | ||
if let style = styles.first { | ||
|
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.
Based on the simplification suggested in #1127 (comment), I think we should actually inline this calculation, much as you had originally. That way:
intersections
array.routeControllerProgressDidChange
or any other notification by checking whether the current step progress’slastIntersectionPassed
hastunnel
in itsoutletRoadClasses
.motorway
class without requiring lots of new code.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.
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 comment
The 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
RouteStep
extension -tunnelIntersectionsBounds
which is consumed in thehasEnteredTunnel(_: coordinates)
function. It handles our edge case.