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

Enable night style when inside tunnel #1127

Merged
merged 62 commits into from
Mar 13, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
d2581fc
Added ability to detect commuter is in a tunnel and automatically ena…
vincethecoder Feb 17, 2018
0a67127
Minor code refactor for tunnel intersection.
vincethecoder Feb 17, 2018
52564d6
Minor grammatical updates to comments on route step tunnel intersection.
vincethecoder Feb 17, 2018
2f67660
Minor text updates.
vincethecoder Feb 17, 2018
72fee83
Added comments to explain preconditions to capture all locations that…
vincethecoder Feb 17, 2018
1b0e577
Removed console logs for debug purposes.
vincethecoder Feb 17, 2018
14c5b8e
Removed resetTimeOfDayTimer method, as styles are automatically adjus…
vincethecoder Feb 17, 2018
d764421
Updated style manager to apply custom styles.
vincethecoder Feb 17, 2018
dc482f7
Set style to night style when in tunnel.
vincethecoder Feb 17, 2018
1b1152a
A few enhancement/refsctor for routestep and routecontroller discover…
vincethecoder Feb 18, 2018
19ac74d
Updated logic for setting a custom style.
vincethecoder Feb 18, 2018
f81319b
Modified delegate method to enable tunnel dark mode when commuter ent…
vincethecoder Feb 18, 2018
aadadf4
Added function to apply a specific style type.
vincethecoder Feb 18, 2018
181b428
Added a break to the for-loop for retrieving the desired style type.
vincethecoder Feb 18, 2018
5198ce4
corrected grammatical error
vincethecoder Feb 18, 2018
b2b0186
Removed extra spaces
vincethecoder Feb 18, 2018
05ef98f
Merge branch 'tunnel/dark_mode_style' of github.com:mapbox/mapbox-nav…
vincethecoder Feb 18, 2018
bb89fff
Refactored condition limit to 2
vincethecoder Feb 18, 2018
613fdfc
A few updates for when a tunnel is detected.
vincethecoder Feb 19, 2018
8caa01a
Refactored function - applyStyle()
vincethecoder Feb 19, 2018
c1d95d5
Minor text updates
vincethecoder Feb 19, 2018
efc7527
Removed break in style apply function.
vincethecoder Feb 19, 2018
256e154
Updated tunnelIntersectionBounds to check index bounds before accessi…
vincethecoder Feb 19, 2018
7686d52
Updated the check for intersection count.
vincethecoder Feb 19, 2018
b8f8a9e
Revised the absolute tunnel distance calculation to use Polyline meth…
vincethecoder Feb 19, 2018
cf306ca
Reversed variables around for distance to entrance.
vincethecoder Feb 19, 2018
c7513bf
Updated route controller delegate methods to detect when tunnel was e…
vincethecoder Feb 21, 2018
fb959c8
Updated changelog file with new delegate methods that detects when a …
vincethecoder Feb 21, 2018
6915af5
Revised logic to accommodate a single intersection on tunnel route.
vincethecoder Feb 21, 2018
e5058e6
Added logic to handle navigation through multiple tunnels on a route.
vincethecoder Feb 22, 2018
f7463d4
Removed extra lines of code to enhance readability
vincethecoder Feb 22, 2018
232baa4
Updated change log to reference link to PR
vincethecoder Feb 22, 2018
64a7897
Merge branch 'master' into tunnel/dark_mode_style
vincethecoder Feb 22, 2018
4e5356a
Minor refactor for array to use the defined typealias - IntersectionB…
vincethecoder Feb 22, 2018
be3cf6b
Updated the function to detect intersection bounds on a route.
vincethecoder Feb 26, 2018
18160da
Updates to cache and calculate the intersection bounds for detecting …
vincethecoder Feb 27, 2018
af050d4
Moved variable up to the top of the extension.
vincethecoder Feb 27, 2018
e48ad83
Made updates based on PR feedback.
vincethecoder Feb 28, 2018
3d1ef3f
Readded deleted function updateSpokenInstructionProgress(for:)
vincethecoder Feb 28, 2018
da0ab08
Updated the change log to reflect new changes.
vincethecoder Feb 28, 2018
82c8310
Updates to set intersectionDistances on RouteProgress class. Modifed …
vincethecoder Feb 28, 2018
4518a12
Updated the bounds check on currentIntersection and upcomingIntersection
vincethecoder Feb 28, 2018
49a5817
Restrict the intersection index to 0 base.
vincethecoder Mar 2, 2018
2b5cd86
Merge branch 'master' into tunnel/dark_mode_style
vincethecoder Mar 2, 2018
f25448f
Added a restriction for the minimum distance of a tunnel intersection…
vincethecoder Mar 2, 2018
59b25b3
Turf import is unnecessary.
vincethecoder Mar 2, 2018
0b93231
Removed valid adjective
vincethecoder Mar 3, 2018
f71f649
Updates to tunnel work change log file
vincethecoder Mar 5, 2018
8f24774
Added function to force refresh of appearance.
vincethecoder Mar 5, 2018
0b44f03
Added a constant to MBNavigation constants file. Removed irrelevant p…
vincethecoder Mar 6, 2018
3675974
Added a feature flag to detect when to switch styles for tunnels.
vincethecoder Mar 6, 2018
2acde88
Deleted extra white space.
vincethecoder Mar 6, 2018
241bba6
Minor refactor
vincethecoder Mar 7, 2018
4ad24d0
Updated the change logs with a note regarding new fields for current …
vincethecoder Mar 7, 2018
6ef815c
Updates to use a CountableClosedRange on the conditional check on int…
vincethecoder Mar 8, 2018
f0411b8
Updates to use a CountableClosedRange on the conditional check on int…
vincethecoder Mar 8, 2018
5ecb164
Reverted updates to use a CountableClosedRange on the conditional che…
vincethecoder Mar 8, 2018
36a374a
Updates to use a CountableClosedRange on the conditional check on int…
vincethecoder Mar 8, 2018
cb8a46b
Reverted updates to use a CountableClosedRange on the conditional che…
vincethecoder Mar 8, 2018
709231e
Added dead reckoning flag to restrict intersection calculations on th…
vincethecoder Mar 8, 2018
70c5e76
Refactored code into functions and renamed feature flag for switching…
vincethecoder Mar 8, 2018
9c878c3
Merge branch 'master' into tunnel/dark_mode_style
vincethecoder Mar 12, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* Exposes `PollyVoiceController.speak(_:)` which would allow custom subclass of PollyVoiceController to override this method and pass a modified SpokenInstruction to our superclass implementation.
* Added a `NavigationMapView.localizeLabels()` method that should be called within `MGLMapViewDelegate.mapView(_:didFinishLoading:)` for standalone `NavigationMapView`s to ensure that map labels are in the correct language. (#1111)
* Changed the heuristics needed for a the users location to unsnap from the route line. (#1110)
* Added delegate method `didEnterTunnel(_:)` which is useful for enabling dark mode when a commuter enters a tunnel.

## v0.13.1 (February 7, 2018)

Expand Down
34 changes: 33 additions & 1 deletion MapboxCoreNavigation/RouteController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Will do.

}

/**
Expand Down Expand Up @@ -634,6 +644,7 @@ extension RouteController: CLLocationManagerDelegate {
updateDistanceToIntersection(from: location)
updateRouteStepProgress(for: location)
updateRouteLegProgress(for: location)
detectRouteStepInTunnel(for: location)
Copy link
Contributor

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:

  • RouteStepProgress can keep track of the last intersection that the user has passed, either as an object or an index into the step’s intersections array.
  • RouteControllerDelegate can react to routeControllerProgressDidChange or any other notification by checking whether the current step progress’s lastIntersectionPassed has tunnel in its outletRoadClasses.
  • No new delegate methods would be required, and future features tied to freeway driving can piggyback on the same idea to check for the motorway class without requiring lots of new code.

Copy link
Contributor

Choose a reason for hiding this comment

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

RouteStepProgress can keep track of the last intersection that the user has passed, either as an object or an index into the step’s intersections array.

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.

Copy link
Contributor Author

@vincethecoder vincethecoder Feb 22, 2018

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 the hasEnteredTunnel(_: coordinates) function. It handles our edge case.


guard userIsOnRoute(location) || !(delegate?.routeController?(self, shouldRerouteFrom: location) ?? true) else {
reroute(from: location)
Expand All @@ -651,6 +662,27 @@ extension RouteController: CLLocationManagerDelegate {
checkForFasterRoute(from: location)
}

func detectRouteStepInTunnel(for location: CLLocation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you, but it might make sense to combine this with hasEnteredTunnel below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention is to make hasEnteredTunnel(_: coordinates) a testable function as opposed to embedding it in detectRouteStepInTunnel (for:) which seems to be a black box at this point.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

distance(to:) is the absolute distance. We need to calculate the distance along a the current step.

let distanceToExit = currentLocation.distance(to: tunnelEndCoordinate)

// Capute any location which lies within the tunnel route.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no need for route in tunnel route. Also, Capute -> Compute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

capture -- misspelt

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
43 changes: 43 additions & 0 deletions MapboxCoreNavigation/RouteStep.swift
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 {
Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

On a motorway, outletRoadClasses may be [.motorway, .tunnel], not just .tunnel.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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])
Copy link
Contributor

Choose a reason for hiding this comment

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

Double check that i + 1 is always within bounds before returning 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.

line #77 already does that check.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
}
}
8 changes: 8 additions & 0 deletions MapboxNavigation/NavigationViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,14 @@ extension NavigationViewController: RouteControllerDelegate {
}
return advancesToNextLeg
}

@objc public func didEnterTunnel(_ darkModeEnabled: Bool) {
Copy link
Contributor

@bsudekum bsudekum Feb 18, 2018

Choose a reason for hiding this comment

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

darkModeEnabled makes it seem like it was already enabled.

if darkModeEnabled {
self.styleManager.applyStyle(type:.nightStyle)
} else {
self.styleManager.resetTimeOfDayTimer()
}
}
}

extension NavigationViewController: StyleManagerDelegate {
Expand Down
15 changes: 15 additions & 0 deletions MapboxNavigation/StyleManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,21 @@ open class StyleManager: NSObject {
resetTimeOfDayTimer()
}

func applyStyle(type styleType: StyleType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this func be combined with the applyStyle below?

Copy link
Contributor Author

@vincethecoder vincethecoder Feb 18, 2018

Choose a reason for hiding this comment

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

Yes, I considered it. However, this func applyStyle(type:) is cleaner, since it requires the styleType. The other func applyStyle scope is getting a bit bigger and complex with more conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATES: I also refactored applyStyle so that we have a function applyStyle(type:) to contain styleType related logic.

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
Copy link
Contributor

@frederoni frederoni Feb 19, 2018

Choose a reason for hiding this comment

The 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 [TopBannerDayStyle(), BottomBannerDayStyle(), …], with that in mind, I don't think we should break 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.

@frederoni Hmmm... the break can be removed. Since we're only applying a specified StyleType, rather than Style -- which can be an array, I don't believe it's applicable in this context. Or what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down