create a halo view for reduced accuracy in userCourseView #2664

ShanMa1991 · 2020-09-29T19:34:05Z

Description

Goal

This pr will create a halo view for the iOS 14 reduced Accuracy during the navigation, similar to mapbox/mapbox-gl-native-ios#381. To solve the issue to fix #2555 following #2693

fix Indicate reduced accuracy location mode #2555
add UserHaloCourseView to public API , update jazzy.yml to contain new symbol
update changelog

Implementation

This pr create a UserHaloCourseView similar to UserCourseView following the same protocol. Add a property in NavigationMapView to control the swift between the two views. Using the navigationServiceDidChangeAuthorization to set the value of the property.

Testing

The following screenshot shows the new halo view during navigation under reduced location accuracy.

/cc @mapbox/navigation-ios

1ec5

Thanks for your patience as we explore the best approach to implement this halo feature. Based on our discussion earlier, let’s consider an architecture that’s less closely tied to the existing puck implementation.

1ec5 · 2020-09-29T20:58:43Z

MapboxNavigation/UserCourseView.swift

@@ -56,8 +59,20 @@ public class UserPuckCourseView: UIView, CourseUpdatable {
        let duration: TimeInterval = animated ? 1 : 0
        UIView.animate(withDuration: duration, delay: 0, options: [.beginFromCurrentState, .curveLinear], animations: {
            let angle = tracksUserCourse ? 0 : CLLocationDegrees(direction - location.course)
+            let dataSource = PassiveLocationDataSource()
+            let locationManager = PassiveLocationManager(dataSource: dataSource)


It makes sense that the map would need to show a different-looking puck depending on the accuracy authorization. However, it’s problematic to have the puck view create a location manager, which under the hood brings in a lot of other machinery around location tracking. Instead of making the view responsible for tracking the user’s location and authorization, let’s move this logic up a few levels:

NavigationMapView is contained inside RouteMapViewController, and a developer can also choose to use NavigationMapView independently within their own view controller. The view controller is the appropriate level at which to conditionalize the display based on location accuracy authorization, because it already has access to the location manager.

NavigationMapView itself can have a property that indicates whether to display a puck or a halo, and it can maintain a separate halo view property that’s an instance of a new UserHaloView class you create. NavigationMapView can show and hide userCourseView and userHaloView, alternating between the two.

UserHaloView can be an instance of CourseUpdatable but otherwise doesn’t need to implement everything that UserPuckCourseView does.

DayStyle and NightStyle can customize the UserHaloView separately, and it only needs to customize the properties that make sense, like the halo color, not puck-specific aspects like the arrow color.

This approach piggy-backs on the existing CourseUpdatable protocol that gives developers the ability to customize the puck’s appearance by providing a different implementation. Conceptually, the approximate location halo is an altogether different user location indicator than the normal one, so hopefully developers will find it intuitive that approximate location causes the map to switch to a different user location indicator automatically.

Thanks for the suggestion! I'll make the following change based on the suggestion.

remove the change in UserCourseView

create a new UserHaloView similar to UserCourseView

create a new property of the location accuracy in NavigationMapView to switch UserHaloView similar to UserCourseView

add judgement in RouteMapViewController to change the property of NavigationMapView's location accuracy

add the customize style of the UserHaloView in DayStyle and NightStyle

When it comes to NavigationViewController as per original ticket, it'd also make sense to show Weak GPS Signal in top banner.

Yes. I'll talk with Jill about that, because the top banner also needs the judgement of the location accuracy. Thanks for the suggestion!

MaximAlien · 2020-09-29T19:42:26Z

MapboxNavigation/UserCourseView.swift

@@ -1,9 +1,9 @@
 import UIKit
 import Turf
 import Mapbox
+import MapboxCoreNavigation

 let PuckSize: CGFloat = 45


I don't see that PuckSize is used anywhere, can probably be safely removed.

Yes, I'll remove it.

MaximAlien · 2020-09-29T22:02:04Z

MapboxNavigation/UserCourseView.swift

@@ -56,8 +59,20 @@ public class UserPuckCourseView: UIView, CourseUpdatable {
        let duration: TimeInterval = animated ? 1 : 0
        UIView.animate(withDuration: duration, delay: 0, options: [.beginFromCurrentState, .curveLinear], animations: {
            let angle = tracksUserCourse ? 0 : CLLocationDegrees(direction - location.course)
+            let dataSource = PassiveLocationDataSource()
+            let locationManager = PassiveLocationManager(dataSource: dataSource)


When it comes to NavigationViewController as per original ticket, it'd also make sense to show Weak GPS Signal in top banner.

MaximAlien · 2020-09-29T22:09:52Z

MapboxNavigation/UserCourseView.swift

@@ -146,6 +176,51 @@ public class UserPuckCourseView: UIView, CourseUpdatable {
    }
 }

+class UserApproximateStyleKitView: UIView {


Please also make sure to run some tests with different use cases in active guidance: e.g. in Overview/Follow modes, different zoom levels, Day/Night styles, vanishing route line enabled/disabled.

Also when it comes to testing I also recommend moving to Xcode 12.0. Thanks to @1ec5 you can now easily build Carthage dependencies by running $ ./scripts/wcarthage.sh bootstrap --use-netrc --use-ssh --platform ios.

Thanks for the suggestion! I'll test the iOS14 with that.

1ec5

Consider rebasing this PR branch against the main branch, to eliminate the non-linear history involving the master branch.

1ec5 · 2020-10-16T14:26:59Z

MapboxNavigation/NavigationMapView.swift

+            self.userCourseView = reducedAccuracyActivatedMode ? UserHaloCourseView(frame: CGRect(origin: .zero, size: 75.0)) : UserPuckCourseView(frame: CGRect(origin: .zero, size: 75.0))
+            //TODO: add the weak gps connection banner below


#2693 will add the banner code to NavigationViewController at a spot where it’ll be convenient to set this property as well.

I see. I'll rebase this branch and follow the #2693 to add the property setting value here.

MaximAlien · 2020-11-09T18:28:12Z

MapboxNavigation/UserHaloCourseView.swift

+import Turf
+import Mapbox


I think neither Turf nor Mapbox are needed here.

Thanks for the suggestion! I'll remove the Turf and Mapbox

MaximAlien · 2020-11-09T18:32:45Z

MapboxNavigation/NavigationMapView.swift

@@ -119,6 +119,11 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
    @objc dynamic public var maneuverArrowStrokeColor: UIColor = .defaultManeuverArrowStroke
    @objc dynamic public var buildingDefaultColor: UIColor = .defaultBuildingColor
    @objc dynamic public var buildingHighlightColor: UIColor = .defaultBuildingHighlightColor
+    @objc dynamic public var reducedAccuracyActivatedMode: Bool = false {


Indentation can be improved here.

Yes. I'll improve it!

MaximAlien · 2020-11-09T18:38:46Z

MapboxNavigation/UserHaloCourseView.swift

+    /**
+    Transforms the location of the user puck.
+    */
+    public func update(location: CLLocation, pitch: CGFloat, direction: CLLocationDegrees, animated: Bool, tracksUserCourse: Bool) {


As I can see this public method is present in UserPuckCourseView as well, but not called. Do we really need it?

I think we need it. Because when the location accuracy changes, the user course view switches between UserPuckCourseView and UserHaloCourseView . Both of them follow the same protocol of CourseUpdatable, which has the update function to support the location and pitch change during the navigation.

MaximAlien · 2020-11-09T18:50:01Z

MapboxNavigation/UserHaloCourseView.swift

+        }
+    }
+
+    @objc public dynamic var haloRingColor: UIColor = #colorLiteral(red: 0.149, green: 0.239, blue: 0.341, alpha: 0.3) {


Please add more descriptive comments for public properties. Indentation can also be improved for already existing comments.

Thanks for the suggestion! I'll improve this part!

MaximAlien · 2020-11-09T18:55:09Z

MapboxNavigation/UserHaloCourseView.swift

+        haloView.backgroundColor = .clear
+        addSubview(haloView)
+
+        NotificationCenter.default.addObserver(self, selector: #selector(locationDidUpdate(_ :)), name: .routeControllerProgressDidChange, object: nil)


Is routeControllerProgressDidChange needed here? If not, can be probably removed.

Thanks for the suggestion, I'll remove the routeControllerProgressDidChange

MaximAlien · 2020-11-09T18:58:07Z

MapboxNavigation/UserHaloCourseView.swift

+        drawHaloView()
+    }
+
+    //var opacity: CGFloat = 0.25


Please remove unused commented code.

Thanks for the suggestion, I've removed the unused code and comments.

MaximAlien · 2020-11-09T19:10:07Z

MapboxNavigation.xcodeproj/project.pbxproj

@@ -309,6 +309,7 @@
 		AE87207E22CF97B900D7DAB7 /* InstructionsCardCollectionDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = AE87207D22CF97B900D7DAB7 /* InstructionsCardCollectionDelegate.swift */; };
 		AEC3AC9A2106703100A26F34 /* HighwayShield.swift in Sources */ = {isa = PBXBuildFile; fileRef = AEC3AC992106703100A26F34 /* HighwayShield.swift */; };
 		AED6285622CBE4CE00058A51 /* ViewController+GuidanceCards.swift in Sources */ = {isa = PBXBuildFile; fileRef = AED6285522CBE4CE00058A51 /* ViewController+GuidanceCards.swift */; };
+		B430D2FA25534FDC0088CC23 /* UserHaloCourseView.swift in Sources */ = {isa = PBXBuildFile; fileRef = B430D2F925534FDC0088CC23 /* UserHaloCourseView.swift */; };


As I can see in #2693 Enable precise location to navigate banner was added. Whenever we enable Precise Location banner disappears, but simulation banner is not present. @jill-cardamon, @1ec5 is it intentional? In my opinion it'd be a good idea to keep location simulation banner if Precise Location is switched on.

That was not intentional. To be clear, you mean the Simulating Navigation at 1x banner correct? There's some tail work to do for #2693 (linked here: #2723), and I can look into making sure the simulation banner stays as part of that work.

Yep, thanks for clarifying this. it'd be good if Simulating Navigation at 1x banner is available whenever Precise Location is On.

MaximAlien · 2020-11-09T19:11:12Z

MapboxNavigation/NavigationViewController.swift

@@ -696,6 +696,7 @@ extension NavigationViewController: NavigationServiceDelegate {
        if #available(iOS 14.0, *), accuracyAuthorization == .reducedAccuracy {
            let title = NSLocalizedString("ENABLE_PRECISE_LOCATION", bundle: .mapboxNavigation, value: "Enable precise location to navigate", comment: "Label indicating precise location is off and needs to be turned on to navigate")
            showStatus(title: title, spinner: false, duration: 20, animated: true, interactive: false)
+            mapView?.reducedAccuracyActivatedMode = true
        } else {
            //Fallback on earlier versions


Currently UserHaloCourseView doesn't change its look when Precise Location is switched on. I think we should update it whenever navigationServiceDidChangeAuthorization(_:didChangeAuthorizationFor:) is called.

you could add an observer to the notification that's posted whenever navigationServiceDidChangeAuthorization(_:didChangeAuthorizationFor:) is called:

mapbox-navigation-ios/MapboxCoreNavigation/NavigationService.swift

Lines 424 to 427 in e50fa90

let info: [NotificationUserInfoKey: Any] = [

.locationAuthorizationKey: manager.value(forKey: "accuracyAuthorization") ?? 0

]

NotificationCenter.default.post(name: .locationAuthorizationDidChange, object: manager, userInfo: info)

Thanks for the suggestion, I add the mapView?.reducedAccuracyActivatedMode = false when the Precise Location if switched on so right now it could switch between the two types of course view.

MaximAlien · 2020-11-09T19:12:44Z

MapboxNavigation/UserHaloCourseView.swift

+    }
+
+    //var opacity: CGFloat = 0.25
+    var boarderSize = 5.0


Suggested change

var boarderSize = 5.0

var borderWidth = 5.0

Thanks for the suggestion, I'll update this part.

MaximAlien · 2020-11-10T19:21:49Z

CHANGELOG.md

+* Added the `NavigationServiceDelegate.navigationService(_:didChangeAuthorizationFor:)` method and `Notification.Name.locationAuthorizationDidChange` to detect when the user changes the Location Services permissions for the current application, including for approximate location on iOS 14. ([#2693](https://github.com/mapbox/mapbox-navigation-ios/pull/2693))
+* When approximate location is enabled on iOS 14, a banner appears reminding the user to disable approximate location to continue navigating. ([#2693](https://github.com/mapbox/mapbox-navigation-ios/pull/2693))
+=======
+


I think those changes are no longer part of 1.2 since they were released in 1.1. Please also use rebasing instead of using merge commits when catching-up from main, as I can see conflict wasn't fixed cleanly in this case.

MaximAlien · 2020-11-10T19:26:20Z

MapboxNavigation/UserHaloCourseView.swift

+A view representing the user’s reduced accuracy location on screen.
+*/
+
+public class UserHaloCourseView: UIView, CourseUpdatable {


Since UserHaloCourseView is now part of public API please update jazzy.yml to contain new symbol.

MaximAlien · 2020-11-10T19:27:55Z

MapboxNavigation/UserHaloCourseView.swift

+
+

Nit: please use similar one-liner as a separator between methods.

MaximAlien · 2020-11-10T19:35:09Z

MapboxNavigation/UserHaloCourseView.swift

+        drawHaloView()
+    }
+
+    var borderSize = 5.0


Since borderSize is only used in a drawHaloView please move it there, it should also be let instead of var. Probably also better to rename it to something like borderWidth or width.

MaximAlien · 2020-11-12T18:05:24Z

MapboxNavigation/DayStyle.swift

@@ -199,6 +199,9 @@ open class DayStyle: Style {
        TopBannerView.appearance().backgroundColor = #colorLiteral(red: 1, green: 1, blue: 1, alpha: 1)
        UserPuckCourseView.appearance().puckColor = #colorLiteral(red: 0.149, green: 0.239, blue: 0.341, alpha: 1)
        UserPuckCourseView.appearance().stalePuckColor = #colorLiteral(red: 0.6000000238, green: 0.6000000238, blue: 0.6000000238, alpha: 1)
+        UserHaloCourseView.appearance().haloColor = #colorLiteral(red: 1, green: 1, blue: 1, alpha: 0.5)


Good idea, we can also style UserHaloCourseView a bit differently for NightStyle.

MaximAlien · 2020-11-12T18:08:18Z

CHANGELOG.md

 ## v1.1.0

 ### Packaging

 * MapboxNavigationNative dependency was updated to v22.0.5 and MapboxCommon to v7.1.2. ([#2648](https://github.com/mapbox/mapbox-navigation-ios/pull/2648))

 ### User location
-


Please try to be as clean as possible when doing rebase onto main (e.g. this line wasn't meant to be removed).

Thanks for the suggestion! I just cleaned the code to remove unnecessary change, and rebase the branch to main.

MaximAlien

Overall looks good to me. I think one last rebase onto main will be required. We should also do additional round of testing after merging to main.

ShanMa1991 linked an issue Sep 29, 2020 that may be closed by this pull request

Indicate reduced accuracy location mode #2555

Closed

ShanMa1991 self-assigned this Sep 29, 2020

ShanMa1991 requested review from 1ec5, MaximAlien and jill-cardamon September 29, 2020 20:09

ShanMa1991 added feature New feature request. topic: location labels Sep 29, 2020

ShanMa1991 added this to the v1.1.0 milestone Sep 29, 2020

1ec5 reviewed Sep 29, 2020

View reviewed changes

MaximAlien reviewed Sep 29, 2020

View reviewed changes

ShanMa1991 changed the base branch from master to main October 6, 2020 19:22

1ec5 reviewed Oct 16, 2020

View reviewed changes

ShanMa1991 modified the milestones: v1.1.0, v1.2.0 Oct 16, 2020

MaximAlien added iOS UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. labels Oct 30, 2020

ShanMa1991 force-pushed the shan-halo-reduced-accuracy-2555 branch from a1c546d to 79c7f11 Compare November 4, 2020 21:29

ShanMa1991 requested review from 1ec5 and MaximAlien November 4, 2020 21:47

ShanMa1991 marked this pull request as ready for review November 5, 2020 15:37

MaximAlien reviewed Nov 9, 2020

View reviewed changes

ShanMa1991 requested a review from MaximAlien November 9, 2020 21:10

ShanMa1991 force-pushed the shan-halo-reduced-accuracy-2555 branch 3 times, most recently from b910962 to 5871e35 Compare November 10, 2020 07:35

MaximAlien reviewed Nov 10, 2020

View reviewed changes

ShanMa1991 force-pushed the shan-halo-reduced-accuracy-2555 branch from 28d486b to e15a3b6 Compare November 11, 2020 05:15

MaximAlien reviewed Nov 12, 2020

View reviewed changes

MaximAlien approved these changes Nov 12, 2020

View reviewed changes

Shan Ma added 9 commits November 12, 2020 10:20

create a halo view for reduced accuracy in userCourseView

aaa518a

add controll to display halo view under reduced location accuracy

fe9fe1e

add the controller to turn off halo view when precise mode on

fbee916

update the changelog

f8800fa

update changelog for currentSpeed calculation

2004d03

update changelog

7118124

update changelog

0cfc9ba

update jazzy and add day style

834f8f4

clean the code

a826475

ShanMa1991 force-pushed the shan-halo-reduced-accuracy-2555 branch from e15a3b6 to a826475 Compare November 12, 2020 18:34

ShanMa1991 merged commit 1a589d9 into main Nov 12, 2020

ShanMa1991 deleted the shan-halo-reduced-accuracy-2555 branch November 12, 2020 19:09

		self.userCourseView = reducedAccuracyActivatedMode ? UserHaloCourseView(frame: CGRect(origin: .zero, size: 75.0)) : UserPuckCourseView(frame: CGRect(origin: .zero, size: 75.0))
		//TODO: add the weak gps connection banner below

	let info: [NotificationUserInfoKey: Any] = [
	.locationAuthorizationKey: manager.value(forKey: "accuracyAuthorization") ?? 0
	]
	NotificationCenter.default.post(name: .locationAuthorizationDidChange, object: manager, userInfo: info)

create a halo view for reduced accuracy in userCourseView #2664

create a halo view for reduced accuracy in userCourseView #2664

Conversation

ShanMa1991 commented Sep 29, 2020 • edited Loading

Description

Goal

Implementation

Testing

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaximAlien Nov 9, 2020 • 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

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

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

MaximAlien left a comment

Choose a reason for hiding this comment

ShanMa1991 commented Sep 29, 2020 •

edited

Loading

MaximAlien Nov 9, 2020 •

edited

Loading