Refactor for testability #282

frederoni · 2017-06-14T19:26:46Z

This PR adds a few location managers that can be set on the RouteController

NavigationLocationManager: This is the base location manager that handles permission and background mode. All other location managers inherits from this location manager.
DefaultLocationManager: This location manager resides in the UI part of the framework because it needs access to UIKit to determine battery state and set the desired accuracy accordingly.
ReplayLocationManager: Replays a list of locations.
SimulatedLocationManager: Replaces SimulatedRoute.

RouteController now also supports simple dead reckoning using any of the location managers. However, isDeadReckoningEnabled defaults to false.

NavigationMapView now receives location updates from the location manager associated with the RouteController through the new RouteControllerDelegate.routeController(_:didUpdateLocations:).

Implement ReplayLocationManager
Replace SimulatedRoute with SimulatedLocationManager
~~Refactor monitorStepProgress~~
~~Write tests using ReplayLocationManager~~

The gist of how to use these location managers:

SimulatedLocationManager - Simulates location updates along a route

let locationManager = SimulatedLocationManager(route: route)
let navigationViewController = NavigationViewController(for: route, locationManager: locationManager)

ReplayLocationManager - Replays a list of locations previously recorded to a file.

let locationManager = ReplayLocationManager()
locationManager.locations = Array<CLLocation>.locations(from: filePath)
let navigationViewController = NavigationViewController(for: route, locationManager: locationManager)

1ec5

Looking good so far.

1ec5 · 2017-06-20T00:08:42Z

Examples/Swift/ViewController.swift

@@ -52,6 +52,18 @@ class ViewController: UIViewController, MGLMapViewDelegate, NavigationViewContro
        getRoute()
    }

+    @IBAction func didTapReplay(_ sender: Any) {


An action is usually named something more active, like replay(_:).

1ec5 · 2017-06-20T00:10:18Z

MapboxCoreNavigation/LocationManagers/NavigationLocationManager.swift

+        }
+
+        if Bundle.main.backgroundModeLocationSupported {
+            activityType = .automotiveNavigation


Should the activity type always be automotiveNavigation, even when the user is getting walking directions?

Extended RouteOptions to convert from MBDirectionsProfileIdentifier to CLActivityType.

1ec5 · 2017-06-20T00:11:30Z

MapboxCoreNavigation/RouteController.swift

+     also derive from regular location updates from a `CLLocationManager`.
+
+     - parameter routeController: The route controller that received the new locations.
+     - parameter locations: The locations that were received from on of the location managers.


Typo: /on/one/

1ec5 · 2017-06-20T00:12:54Z

MapboxCoreNavigation/RouteController.swift

    public func locationManager(_ manager: CLLocationManager, didUpdateLocations locations: [CLLocation]) {
        guard let location = locations.last else {
            return
        }

+        delegate?.routeController!(self, didUpdateLocations: [location])


Should routeController(_:didUpdateLocations:) be optional if we require its existence here?

Should be optional

ericrwolfe

@frederoni this looks awesome, the ReplayLocationManager is going to be especially useful.

ericrwolfe · 2017-06-22T16:50:59Z

Examples/Swift/ViewController.swift

        navigationViewController.voiceController?.volume = 0.5
        navigationViewController.navigationDelegate = self

+        if let locationManager = locationManager {
+            navigationViewController.routeController.locationManager = locationManager


Would be nice if locationManager was a nullable property and you could set this to nil for the default location manager.

I wonder if Swift have something like Obj-C’s null_resettable.

Yes: CLLocationManager! with a custom getter or setter that handles nil.

ericrwolfe · 2017-06-22T16:51:39Z

Examples/Swift/ViewController.swift

@@ -143,11 +156,15 @@ class ViewController: UIViewController, MGLMapViewDelegate, NavigationViewContro
        // You can get a token here: http://docs.aws.amazon.com/mobile/sdkforios/developerguide/cognito-auth-aws-identity-for-ios.html
        //navigationViewController.voiceController?.identityPoolId = "<#Your AWS IdentityPoolId. Remove Argument if you do not want to use AWS Polly#>"

-        navigationViewController.simulatesLocationUpdates = simulatesLocationUpdates
        navigationViewController.routeController.snapsUserLocationAnnotationToRoute = true


We should make snapsUserLocationAnnotationToRoute=true by default

ericrwolfe · 2017-06-22T17:05:04Z

MapboxNavigation/NavigationViewController.swift

@@ -165,15 +166,6 @@ public class NavigationViewController: NavigationPulleyViewController, RouteMapV
    public var routeController: RouteController!

    /**
-     `simulate` provides simulated location updates along the given route.
-     */
-    public var simulatesLocationUpdates: Bool = false {


Should we keep this bool for simplicity in the Navigation UI and have the didSet function optionally set the SimulatedLocationManager?

We can't do the same for ReplayLocationManager, so for consistency, I'd prefer not to expose this shortcut.

ericrwolfe · 2017-06-22T17:06:27Z

MapboxNavigation/RouteMapViewController.swift

-        }
+
+        mapView.locationManager.stopUpdatingLocation()
+        mapView.locationManager.stopUpdatingHeading()


These should move to viewWillAppear to fix animation issues. More here: #303

frederoni · 2017-06-22T22:46:38Z

MapboxCoreNavigation/RouteController.swift

@@ -307,9 +368,6 @@ extension RouteController: CLLocationManagerDelegate {
                strongSelf.routeProgress = RouteProgress(route: route)
                strongSelf.routeProgress.currentLegProgress.stepIndex = 0
                strongSelf.delegate?.routeController?(strongSelf, didRerouteAlong: route)
-                NotificationCenter.default.post(name: RouteControllerDidReroute, object: self, userInfo: [
-                    MBRouteControllerNotificationRouteKey: location


Moved this to routeProgress didSet and changed RouteKey to LocationKey

frederoni · 2017-06-25T18:52:21Z

CLLocationManager crashes intermittently if it's deallocated in the same runloop it was initialized in, so I had to change the API a bit. RouteController and NavigationViewController is now initialized with an optional CLLocationManager.

frederoni · 2017-06-25T18:53:37Z

MapboxCoreNavigationTests/MapboxCoreNavigationTests.swift


        self.expectation(forNotification: RouteControllerAlertLevelDidChange.rawValue, object: navigation) { (notification) -> Bool in
            XCTAssertEqual(notification.userInfo?.count, 2)

            let routeProgress = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationRouteProgressKey] as? RouteProgress
            let userDistance = notification.userInfo![RouteControllerAlertLevelDidChangeNotificationDistanceToEndOfManeuverKey] as! CLLocationDistance

-            return routeProgress?.currentLegProgress.alertUserLevel == .low && routeProgress?.currentLegProgress.stepIndex == 2 && round(userDistance) == 1758
+            return routeProgress?.currentLegProgress.alertUserLevel == .low && routeProgress?.currentLegProgress.stepIndex == 2 && round(userDistance) == 1786


@1ec5 @bsudekum Any idea how userDistance could've changed?

Not sure. Do you think it could be a race condition?

I'm getting 1786 on master (2c6ec67) 😕

That test was disabled in the scheme. 😄

frederoni force-pushed the refactor-testability branch from 81b4c3d to 2f4df4d Compare June 15, 2017 23:15

frederoni requested review from bsudekum and 1ec5 June 16, 2017 00:05

frederoni force-pushed the refactor-testability branch from 89973de to d5e8261 Compare June 16, 2017 21:41

1ec5 mentioned this pull request Jun 19, 2017

Refactor monitorStepProgress #278

Closed

1ec5 reviewed Jun 20, 2017

View reviewed changes

frederoni force-pushed the refactor-testability branch 4 times, most recently from 4d6c2ae to 88b80ec Compare June 22, 2017 15:06

frederoni changed the title ~~[WIP] Refactor for testability~~ Refactor for testability Jun 22, 2017

ericrwolfe approved these changes Jun 22, 2017

View reviewed changes

frederoni force-pushed the refactor-testability branch 3 times, most recently from 8e8463b to 9eab5b7 Compare June 22, 2017 22:39

frederoni commented Jun 22, 2017

View reviewed changes

frederoni force-pushed the refactor-testability branch 6 times, most recently from f7fcaa8 to 860d3bf Compare June 25, 2017 18:42

frederoni commented Jun 25, 2017

View reviewed changes

frederoni force-pushed the refactor-testability branch from 860d3bf to a0a59c2 Compare June 25, 2017 21:04

Refactor and add convenient location managers

aaddd94

frederoni force-pushed the refactor-testability branch from a0a59c2 to aaddd94 Compare June 26, 2017 20:43

Enabled testLowAlert

9fca6f3

frederoni merged commit 1244ad7 into master Jun 26, 2017

frederoni deleted the refactor-testability branch March 19, 2018 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor for testability #282

Refactor for testability #282

frederoni commented Jun 14, 2017 •

edited

Loading

1ec5 left a comment

1ec5 Jun 20, 2017

1ec5 Jun 20, 2017

frederoni Jun 22, 2017 •

edited

Loading

1ec5 Jun 20, 2017

1ec5 Jun 20, 2017

frederoni Jun 22, 2017

ericrwolfe left a comment

ericrwolfe Jun 22, 2017

frederoni Jun 22, 2017

1ec5 Jun 22, 2017 •

edited

Loading

ericrwolfe Jun 22, 2017

ericrwolfe Jun 22, 2017

frederoni Jun 22, 2017 •

edited

Loading

ericrwolfe Jun 22, 2017

frederoni Jun 22, 2017

frederoni commented Jun 25, 2017 •

edited

Loading

frederoni Jun 25, 2017 •

edited

Loading

1ec5 Jun 26, 2017

frederoni Jun 26, 2017

frederoni Jun 26, 2017

Refactor for testability #282

Refactor for testability #282

Conversation

frederoni commented Jun 14, 2017 • edited Loading

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericrwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Jun 25, 2017 • edited Loading

frederoni Jun 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Jun 14, 2017 •

edited

Loading

frederoni Jun 22, 2017 •

edited

Loading

1ec5 Jun 22, 2017 •

edited

Loading

frederoni Jun 22, 2017 •

edited

Loading

frederoni commented Jun 25, 2017 •

edited

Loading

frederoni Jun 25, 2017 •

edited

Loading