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

Make InstructionsCardViewController reusable independently of NavigationComponent #2296

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

avi-c
Copy link
Contributor

@avi-c avi-c commented Jan 6, 2020

Also re-expose the Route JSON string for use in debug metrics database.

…. Also reexpose the Route JSON for use in Apex's Navigation History functionality.
@avi-c avi-c requested a review from 1ec5 January 6, 2020 21:12
@avi-c avi-c self-assigned this Jan 6, 2020
MapboxCoreNavigation/Route.swift Outdated Show resolved Hide resolved
@@ -13,7 +13,7 @@ open class InstructionsCardViewController: UIViewController {
var instructionsCardLayout: InstructionsCardCollectionLayout!

public private(set) var isInPreview = false
private var currentStepIndex: Int?
public var currentStepIndex: Int?
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we expect the application to use this property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-prukop - Can you comment on what you use this property for in Apex? Is it possible we can avoid needing it in a client app?

Copy link
Contributor

Choose a reason for hiding this comment

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

@avi-c see my comments below. This and the exposing of reloadDataSource are related

@@ -130,7 +130,7 @@ open class InstructionsCardViewController: UIViewController {
NSLayoutConstraint.activate(instructionCollectionViewContraints)
}

func reloadDataSource() {
open func reloadDataSource() {
Copy link
Contributor

Choose a reason for hiding this comment

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

In what scenarios would the application need to force a reload because the view controller isn’t automatically reloading?

Copy link
Contributor

Choose a reason for hiding this comment

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

@1ec5 I tracked it down. The application actually doesn't need these things. This was part of an effort to prevent unnecessary passing of the navigation service to the guidance cards.

Currently the guidance cards have this code

extension InstructionsCardViewController: NavigationComponent {
    public func navigationService(_ service: NavigationService, didUpdate progress: RouteProgress, with location: CLLocation, rawLocation: CLLocation) {
        routeProgress = progress
        reloadDataSource()
    }
    
    public func navigationService(_ service: NavigationService, didPassVisualInstructionPoint instruction: VisualInstructionBanner, routeProgress: RouteProgress) {
        self.routeProgress = routeProgress
        reloadDataSource()
    }
    
    public func navigationService(_ service: NavigationService, didRerouteAlong route: Route, at location: CLLocation?, proactive: Bool) {
        self.currentStepIndex = nil
        self.routeProgress = service.routeProgress
        reloadDataSource()
    }
}

and I wanted to try it out with this code

extension InstructionsCardViewController {
    func routeProgressDidUpdate(_ progress: RouteProgress) {
        routeProgress = progress
        reloadDataSource()
    }
    
    func routeProgressDidPassVisualInstructionPoint(_ progress: RouteProgress) {
        routeProgress = progress
        reloadDataSource()
    }
    
    func routeProgressDidReroute(_ progress: RouteProgress) {
        currentStepIndex = nil
        routeProgress = progress
        reloadDataSource()
    }
}

This way we don't need to expose the entire nav service to the guidance cards or the UI side of the app.

I think we should move to make this change in InstructionsCardViewController

Copy link
Contributor

@1ec5 1ec5 Jan 6, 2020

Choose a reason for hiding this comment

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

Yes, let’s fold the extension into the main implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per chat, replacing the existing extension with the one proposed in #2296 (comment) would break NavigationComponent conformance and thus NavigationViewController compatibility.

By design, all navigation components need to be aware of the navigation service because that’s how they get access to the route progress, and delegate methods always need to refer to the delegator. (Alternatively, a component could observe notifications, but we moved away from that mechanism because components kept failing to filter for specific senders.) We didn’t consider that it might be desirable to encapsulate NavigationService from UI code. (cc @JThramer)

Making currentStepIndex and reloadDataSource() public allows an application to reuse InstructionsCardViewController in a non-NavigationViewController environment while wrapping these NavigationServiceDelegate methods in custom methods that don’t necessarily expose NavigationService. Looking ahead to when we make the guidance card feature public, we can document these members as being public for the purpose of subclassing. (In general, we could do a better job of adding subclassing notes to class documentation anyways.)

@1ec5 1ec5 added this to the v1.0.0 milestone Jan 7, 2020
@1ec5 1ec5 changed the title Re-expose a couple of private properties and functions that Mapbox Apex app uses Make InstructionsCardViewController reusable independently of NavigationComponent Jan 7, 2020
@avi-c avi-c merged commit 8ca6122 into master Jan 7, 2020
@avi-c avi-c deleted the ac-UpdatesForDemoAppCompatibility branch January 7, 2020 18:53
@1ec5 1ec5 restored the ac-UpdatesForDemoAppCompatibility branch January 7, 2020 19:17
@1ec5 1ec5 deleted the ac-UpdatesForDemoAppCompatibility branch January 7, 2020 19:17
@1ec5 1ec5 modified the milestones: v1.0.0, v0.x next Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants