Integrated new directions API fields with the tertiary instruction and lane indications #1514

vincethecoder · 2018-06-21T17:17:27Z

These updates make sure we rely on the new directions LaneIndicationComponents class for lane information. This information is usually retrieved from the tertiary instruction -- which can either be a LaneIndicationComponent or VisualInstructionComponent. The instructions banner and integrated tests were also modified to reflect the new changes.

Relies on mapbox/mapbox-directions-swift#258

…components.

… shields for banner instructions

vincethecoder · 2018-06-21T21:21:20Z

MapboxNavigation/LanesView.swift

-            }
+        let step = currentLegProgress.currentStep
+
+        guard durationRemaining < RouteControllerMediumAlertInterval,


@bsudekum do you think we should reduce the time threshold for tertiaryInstructions?

We should not actually be doing any check like this now. The server should tell us exactly when and what to show.

# Conflicts: # Cartfile # MapboxNavigation-Documentation.podspec

bsudekum · 2018-06-22T00:35:48Z

MapboxCoreNavigation.podspec

@@ -40,7 +40,7 @@ Pod::Spec.new do |s|
  s.requires_arc = true
  s.module_name = "MapboxCoreNavigation"

-  s.dependency "MapboxDirections.swift", "~> 0.21.0"
+  s.dependency "MapboxDirections.swift", :git => "https://github.com/mapbox/MapboxDirections.swift.git", :branch => "then"


Unfortunately, something like this does not work. Only published pods are able to be pulled in as deps here.

…hecks to access the lanes.

bsudekum · 2018-06-22T16:08:33Z

MapboxNavigation/LanesView.swift

-                stackView.addArrangedSubview(laneView)
-            }
+        let step = currentLegProgress.currentStep
+        guard let firstInstruction = step.instructionsDisplayedAlongStep?.first,


What if there is more than one visual instruction on the step?

I think we need something like what we have for the currentSpokenInstruction but for visual instruction:

mapbox-navigation-ios/MapboxCoreNavigation/RouteProgress.swift

Lines 458 to 462 in 03b93ad

@objc public var currentSpokenInstruction: SpokenInstruction? {

guard let instructionsSpokenAlongStep = step.instructionsSpokenAlongStep else { return nil }

guard spokenInstructionIndex < instructionsSpokenAlongStep.count else { return nil }

return instructionsSpokenAlongStep[spokenInstructionIndex]

}

We have all the pieces slightly above this computed variable to create currentVisualInstruction.

True. That was an oversight. I will make the necessary changes to account for multiple visual instructions.

bsudekum · 2018-06-22T16:09:27Z

MapboxNavigation/LanesView.swift

-        } else {
-            hide()
-        }
+        let subviews = lanes.map { LaneView(component: $0, direction: step.maneuverDirection) }


Why do we need to init new LaneView's? Shouldn't we already have these objects?

Yeah, we are given the LaneIndicationComponents from the API.

bsudekum · 2018-06-22T18:31:33Z

MapboxNavigation/LanesView.swift

+            return
+        }
+
+        let components: [ComponentRepresentable] = instructionsDisplayedAlongStep


@vincethecoder I'm still not seeing how we're finding the current visual instruction that should be displayed. If a step has 3 different tertiary instructions, how do we know which one to show?

@bsudekum smh - using the current spoken instruction index. Will update in a moment.

bsudekum · 2018-06-22T20:26:48Z

MapboxNavigation/LanesView.swift

-        } else {
-            hide()
+        let currentInstruction = visualInstructions[spokenInstructionIndex]
+        guard let lanes: [LaneIndicationComponent] = currentInstruction.tertiaryInstruction?.components.compactMap({ component in


Want to pull this out into an optional variable? I think it would make this guard easier to read.

bsudekum · 2018-06-22T22:52:35Z

MapboxNavigation/LanesView.swift

-            show()
-        } else {
-            hide()
+        let currentInstruction = visualInstructions[spokenInstructionIndex]


It looks like we're still using spokenInstructionIndex instead of the visual index.

…or to lane indications to enhance readability.

vincethecoder · 2018-06-25T15:04:10Z

Waiting on dependent PR mapbox/mapbox-directions-swift#258 to be merged and published. Then this PR can pass the CI tests before it's merged into master.

bsudekum · 2018-06-25T17:06:59Z

Started to test a few test cases, here is one that I'm looking at, request, visual link.

However, it looks like something is stuck in an infinite loop:

…e cache key.

vincethecoder · 2018-06-26T14:29:02Z

@bsudekum Upon changes made here, here is the new memory report.

/cc @akitchen - decided to post the memory report, as the current function is not unit testable at the moment.

vincethecoder

@akitchen we definitely need to develop a level of abstraction to ensure that the cache key fo exit label works. Since the current implementation is not a viable candidate for a unit test, I've decided to share the improvement in memory performance as a result of my changes here.

1ec5

Would you mind capturing some current screenshots of the new UI in action? I’m particularly curious to see turn lanes with multiple usable lanes, “then” banners around arrival steps, and intermediate waypoint steps.

1ec5 · 2018-07-12T20:19:21Z

MapboxNavigation/ExitView.swift

@@ -141,4 +141,10 @@ class ExitView: StylableView {
        let labelTrailing = trailingAnchor.constraint(equalTo: exitNumberLabel.trailingAnchor, constant: 8)
        return [imageLeading, imageLabelSpacing, labelTrailing]
    }
+
+    static func criticalHash(side: ExitSide, dataSource: DataSource) -> String {


Why is this called “critical”? Is there something important about this method and similarly named methods that needs to be documented somewhere?

@JThramer could you chime in to elucidate the essence of this refactor for ExitView?

Still unclear on this.

Yes -- the reason why it's called criticalHash is because the side: and dataSource: arguments contain critical values that affect the appearance of the ExitView.

Considering that we're caching views by their rendered images, any property that affects how an ExitView might appear needs to be accounted for in the cache key logic.

1ec5 · 2018-07-12T21:16:57Z

MapboxNavigation/InstructionsBannerView.swift

@@ -93,7 +93,7 @@ open class BaseInstructionsBannerView: UIControl {
    /**
     Updates the instructions banner info with a given `VisualInstructionBanner`.
     */
-    public func updateInstruction(_ instruction: VisualInstructionBanner?) {
+    public func update(for instruction: VisualInstructionBanner?) {


Since this method is public, we should mention in the changelog that it has been renamed. Also, it appears to be inaccessible to Objective-C.

1ec5 · 2018-07-12T21:18:34Z

MapboxNavigation/LanesView.swift

-        if stackView.arrangedSubviews.count > 0 {
-            show()
-        } else {
+        let laneIndications: [LaneIndicationComponent]? = tertiaryInstruction.components.compactMap({ component in


Nit: we can use trailing closure syntax here, since it isn’t confusing.

1ec5 · 2018-07-12T21:19:39Z

MapboxNavigation/LanesView.swift

-            show()
-        } else {
+        let laneIndications: [LaneIndicationComponent]? = tertiaryInstruction.components.compactMap({ component in
+            guard let lane = component as? LaneIndicationComponent else { return nil }


This can be written more succinctly as { component as? LaneIndicationComponent }.

…ction method.

…ctor for mapping lane indication component in tertiary instruction.

….com:mapbox/mapbox-navigation-ios into navigation/tertiary_instructions_lane_update # Conflicts: # MapboxNavigation/NextBannerView.swift

bsudekum · 2018-07-16T16:53:52Z

@vincethecoder I think we're close, just need a few screenshots that @1ec5 is asking for and we can merge.

vincethecoder · 2018-07-16T17:49:30Z

@1ec5

/cc @bsudekum

1ec5 · 2018-07-16T17:23:48Z

CHANGELOG.md

@@ -11,8 +11,9 @@
 * Fixed an issue when selecting a step from the steps list, you could be brought to the wrong step. [#1524](https://github.com/mapbox/mapbox-navigation-ios/pull/1524/)
 * `StyleManager.locationFor(styleManager:)` now allows for an optional CLLocation to be returned. [#1523](https://github.com/mapbox/mapbox-navigation-ios/pull/1523)
 * NavigationViewController now uses the recommended way `.preferredStatusBarStyle` to set the style of the status bar. [#1535](https://github.com/mapbox/mapbox-navigation-ios/pull/1535)
+* Renamed `InstructionsBannerView.updateInstruction(_:)` to `InstructionsBannerView.update(for:)`.


Link to this PR from any new changeset entries, so that developers can get the full story from the PR description (which should be expanded to explain the API changes too).

1ec5 · 2018-07-16T17:24:56Z

CHANGELOG.md

 * Deleted `InstructionsBannerView.update(_:)`, which previously updated the banner instruction and the current distance.
-* This is replaced with `InstructionsBannerView.updateDistance(for:)` and `InstructionsBannerView.updateInstruction(_:)`. `InstructionsBannerView.updateDistance(for:)` should be called on every location update, while `InstructionsBannerView.updateInstruction(_:)` should be called when `NSNotification.Name. routeControllerDidPassVisualInstructionPoint` is emitted.
+* This is replaced with `InstructionsBannerView.updateDistance(for:)` and `InstructionsBannerView.update(for:)`. `InstructionsBannerView.updateDistance(for:)` should be called on every location update, while `InstructionsBannerView.update(for:)` should be called when `NSNotification.Name. routeControllerDidPassVisualInstructionPoint` is emitted.


These three points can potentially be combined into one with less repetition.

1ec5 · 2018-07-16T17:26:58Z

MapboxNavigation/InstructionsBannerView.swift

@@ -93,7 +93,7 @@ open class BaseInstructionsBannerView: UIControl {
    /**
     Updates the instructions banner info with a given `VisualInstructionBanner`.
     */
-    public func updateInstruction(_ instruction: VisualInstructionBanner?) {
+    @objc public func update(for instruction: VisualInstructionBanner?) {


The Objective-C name of this method is -updateWithFor:. It should be -updateForVisualInstructionBanner:. (×3)

1ec5 · 2018-07-16T17:59:48Z

MapboxNavigation/InstructionPresenter.swift

@@ -166,7 +167,10 @@ class InstructionPresenter {
    }

    private func instructionHasDownloadedAllShields() -> Bool {
-        for component in instruction.textComponents {
+        let textComponents = instruction.components.compactMap { $0 as? VisualInstructionComponent }
+        guard !textComponents.isEmpty else { return false }


This can be written more succinctly as:

guard let _ = instruction.components.contains(where: { $0 is VisualInstructionComponent }) else { return false }

guard instruction.components.contains(where: { $0 is VisualInstructionComponent }) else { return false } 😏

@1ec5 Indeed that is succinct. However, for the purpose of this function scope, we need the variable and also an early exit if it's empty. Essentially, we need the contents of textComponent to iterate at this line

https://github.com/mapbox/mapbox-navigation-ios/pull/1514/files/13f2d71321b7d5e320721e611c983e6411a2d904#diff-af48d70309ee195e34cc5e6cc6b27caaR173

…r the new instruction banner view revision.

1ec5 · 2018-07-16T20:27:43Z

MapboxNavigation/ExitView.swift

@@ -141,4 +141,10 @@ class ExitView: StylableView {
        let labelTrailing = trailingAnchor.constraint(equalTo: exitNumberLabel.trailingAnchor, constant: 8)
        return [imageLeading, imageLabelSpacing, labelTrailing]
    }
+
+    static func criticalHash(side: ExitSide, dataSource: DataSource) -> String {


Still unclear on this.

Misclicked

1ec5 · 2018-07-16T20:31:20Z

MapboxNavigation/InstructionsBannerView.swift

@@ -93,7 +93,8 @@ open class BaseInstructionsBannerView: UIControl {
    /**
     Updates the instructions banner info with a given `VisualInstructionBanner`.
     */
-    public func updateInstruction(_ instruction: VisualInstructionBanner?) {
+    @objc(updateForVisualInstruction:)


This should be -updateForVisualInstructionBanner:, given the parameter’s type.

1ec5

Missing documentation, plus that “critical hash” thing in #1514 (comment), but otherwise good to go.

1ec5 · 2018-07-16T20:40:21Z

MapboxNavigation/LanesView.swift

-        guard !currentLegProgress.userHasArrivedAtWaypoint else { return }
-        let durationRemaining = currentLegProgress.currentStepProgress.durationRemaining
-
+    @objc(updateForVisualInstructionBanner:)


Missing a documentation comment. (×3)

1ec5 · 2018-07-16T20:40:32Z

MapboxNavigation/ManeuverView.swift

+        }
+    }
+
+    @objc public var drivingSide: DrivingSide = .right {


These two properties are missing documentation comments.

…ther exit view and generic route shield.

…perties. Minor refactor.

JThramer · 2018-07-16T23:33:56Z

MapboxNavigation/ExitView.swift

@@ -141,4 +141,13 @@ class ExitView: StylableView {
        let labelTrailing = trailingAnchor.constraint(equalTo: exitNumberLabel.trailingAnchor, constant: 8)
        return [imageLeading, imageLabelSpacing, labelTrailing]
    }
+
+    /**
+     `criticalHash(side:dataSource:)` generates the cache key needed to hold the `ExitView`'s `imageRepresentation` in the `ImageCache` caching engine.


Nit: Possessive its' is with the apostrophe at the end.

Nit: Possessive its' is with the apostrophe at the end.

wait, what? where?

"its" is the possessive form (think "his")
"it's" is the contraction for "it is"

There is no such thing as its'

Perhaps you are thinking of the plural possessive? For example, "Our engineers' computers are too slow"

but I don't see how any of those apply here.

</grammar police>

Has been addressed.

vincethecoder added 3 commits June 21, 2018 12:19

Updated cartfile file with then branch off Directions

352f70b

Changes made to accommodate the new API changes made in instructions …

c73c2a3

…components.

Filtered instructions for visual instruction components to facilitate…

f7e88a1

… shields for banner instructions

vincethecoder self-assigned this Jun 21, 2018

Updates Lane tests upon changes made in both lane and lanes views.

61287aa

vincethecoder commented Jun 21, 2018

View reviewed changes

vincethecoder added 4 commits June 21, 2018 17:36

Added a new test for another maneuver.

18be61f

Merge branch 'master' into navigation/tertiary_instructions_lane_update

e3d839d

Updated podspec with directions then branch.

3a3fc0d

Merge branch 'master' into navigation/tertiary_instructions_lane_update

966ec8a

# Conflicts: # Cartfile # MapboxNavigation-Documentation.podspec

bsudekum reviewed Jun 22, 2018

View reviewed changes

Updates to make routecontroller a class property. Removed the extra c…

4f450cb

…hecks to access the lanes.

bsudekum reviewed Jun 22, 2018

View reviewed changes

Accommodates multiple lane banner instructions.

409e2fa

bsudekum reviewed Jun 22, 2018

View reviewed changes

[Update] Accommodates multiple lane banner instructions.

37018f8

bsudekum reviewed Jun 22, 2018

View reviewed changes

vincethecoder added 4 commits June 23, 2018 00:07

Updates to use visualInstructionIndex for visual instructions. Refact…

d8a0234

…or to lane indications to enhance readability.

Updated podspec to point to then branch.

7892bff

Updates to use a published MapboxDirections.swift dependency

fa9f08b

Merge branch 'master' into navigation/tertiary_instructions_lane_update

fa1b32c

vincethecoder added the ⚠️ DO NOT MERGE PR should not be merged! label Jun 25, 2018

Fixed issue where component was not being stored and looked up by sam…

bff3b2f

…e cache key.

Removed poorly composed unit test for cache key for exit label.

560c8ba

vincethecoder commented Jun 26, 2018

View reviewed changes

1ec5 reviewed Jul 12, 2018

View reviewed changes

Bobby Sudekum and others added 6 commits July 13, 2018 08:28

Dont hardcode maneuver view to primary instruction

b6fdc38

Merge branch 'master' into navigation/tertiary_instructions_lane_update

436889c

right

4ada71c

[GH-1514] - Updates to change log to reflect changes to update instru…

890f28f

…ction method.

[GH-1514] - Exposed update(for:) functions to Objective-C. Minor refa…

feaf4d9

…ctor for mapping lane indication component in tertiary instruction.

Merge branch 'navigation/tertiary_instructions_lane_update' of github…

13f2d71

….com:mapbox/mapbox-navigation-ios into navigation/tertiary_instructions_lane_update # Conflicts: # MapboxNavigation/NextBannerView.swift

1ec5 reviewed Jul 16, 2018

View reviewed changes

vincethecoder added 2 commits July 16, 2018 15:32

[GH-1514] - Updated function with objective-c friendly signature.

27f91a2

[GH-1514] - Updated the change log with a more concise description fo…

901e229

…r the new instruction banner view revision.

1ec5 previously approved these changes Jul 16, 2018

View reviewed changes

1ec5 reviewed Jul 16, 2018

View reviewed changes

[GH-1514] - Updated function with objective-c friendly signature.

11902ca

1ec5 approved these changes Jul 16, 2018

View reviewed changes

vincethecoder added 2 commits July 16, 2018 16:55

[GH-1514] - Added documentation for the critical hash function for bo…

6461783

…ther exit view and generic route shield.

[GH-1514] - Added the missing documentation in the declared class pro…

a117366

…perties. Minor refactor.

1ec5 mentioned this pull request Jul 16, 2018

Add missing MapboxDirections.swift classes to docs #1560

Merged

JThramer reviewed Jul 16, 2018

View reviewed changes

[GH-1514] - Minor refactor.

00e211a

vincethecoder merged commit dfe9df7 into master Jul 17, 2018

frederoni mentioned this pull request Aug 30, 2018

Flip roundabout if it's left-hand rule of the road #1639

Merged

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018

1ec5 added this to the v0.19.0 milestone Dec 18, 2018

JThramer mentioned this pull request Sep 30, 2019

[Regression] Next instruction banners show during entire step, not just at end #2245

Closed

1ec5 deleted the navigation/tertiary_instructions_lane_update branch December 5, 2019 18:22

1ec5 mentioned this pull request Feb 2, 2021

Dual-use lane icons are highlighted incorrectly #2796

Closed

	@objc public var currentSpokenInstruction: SpokenInstruction? {
	guard let instructionsSpokenAlongStep = step.instructionsSpokenAlongStep else { return nil }
	guard spokenInstructionIndex < instructionsSpokenAlongStep.count else { return nil }
	return instructionsSpokenAlongStep[spokenInstructionIndex]
	}

Integrated new directions API fields with the tertiary instruction and lane indications #1514

Integrated new directions API fields with the tertiary instruction and lane indications #1514

Conversation

vincethecoder commented Jun 21, 2018

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

vincethecoder commented Jun 25, 2018 • edited Loading

bsudekum commented Jun 25, 2018

vincethecoder commented Jun 26, 2018

vincethecoder left a comment • edited Loading

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

bsudekum commented Jul 16, 2018

vincethecoder commented Jul 16, 2018 • 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

vincethecoder Jul 16, 2018 • edited Loading

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 • 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

akitchen Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder commented Jun 25, 2018 •

edited

Loading

vincethecoder left a comment •

edited

Loading

vincethecoder commented Jul 16, 2018 •

edited

Loading

vincethecoder Jul 16, 2018 •

edited

Loading

1ec5 left a comment •

edited

Loading

akitchen Jul 17, 2018 •

edited

Loading