Add then component #258

bsudekum · 2018-04-09T19:32:29Z

~~This parses out the then text component for displaying information about the upcoming step.~~

UPDATES
This parses out the sub component for displaying information about the upcoming step. Also, the new lane component type here provides additional lane information to assist visual representation.

/cc @lyzidiamond @danpaz @mapbox/navigation-ios

bsudekum · 2018-04-10T17:59:41Z

MapboxDirections/MBVisualInstruction.swift

+        var thenText: String?
+        var thenTextComponents: [VisualInstructionComponent]?
+        if let thenTextComponent = json["then"] as? JSONDictionary {
+            thenText = thenTextComponent["text"] as? String


This key is being renamed to sub.

vincethecoder · 2018-05-16T18:20:08Z

Sample JSON structure - for future reference:

# Conflicts: # MapboxDirections/MBVisualInstruction.swift

…nt with directions and active properties. Modified the mapping of text components for the visual instructions initializer.

vincethecoder · 2018-05-17T20:01:33Z

MapboxDirections/MBVisualInstructionComponent.swift

+
+     If the value is `[.left", .straight]`, the driver can go straight or left from that lane. This is only set when the `component` is a lane.
+     */
+    @objc public var directions: [Any]?


Any was used instead of ManeuverDirection -- since this objc declared property is an array of enum.

How about renaming this and making it a lane indication: https://github.com/mapbox/MapboxDirections.swift/blob/master/MapboxDirections/MBLaneIndication.h

bsudekum · 2018-05-17T23:07:20Z

MapboxDirections/MBVisualInstructionComponent.swift

        self.text = text
        self.imageURL = imageURL
        self.type = type
        self.abbreviation = abbreviation
        self.abbreviationPriority = abbreviationPriority
+        if let directions = directions {
+            self.directions = directions.map { ManeuverDirection(description: $0) ?? .none }


I don't think directions is an array of ManeuverDirctions. Try using the LaneIndication initializer here.

@bsudekum yes (according to the recently published api-docs), when type == .lane we do have an array of directions.

@bsudekum checkout the new structure for lane and new directions field.

@vincethecoder I'm saying that internally in this library, these are LaneIndications and not ManeuverDirections.

bsudekum · 2018-05-17T23:09:01Z

MapboxDirections/MBVisualInstructionComponent.swift

+
+     If multiple lanes are active, then it used to complete the upcoming maneuver. This value is set to `false` by default.
+     */
+    @objc public var active: Bool = false


This property name does not really convey the link between it's name and whether or not the lane usable.

bsudekum · 2018-05-17T23:22:57Z

MapboxDirections/MBVisualInstructionType.swift

+    /**
+     The step does not have a particular visual instruction component type associated with it.
+
+     This visual instruction component type is used as a workaround for bridging to Objective-C which does not support nullable enumeration-typed values.


No need to state this is work around, we do this elsewhere with other enums. Fine to say there is no visual component in this case.

…variable name, active. Removed unnecessary comment in visual instruction type.

bsudekum · 2018-05-18T16:48:14Z

MapboxDirections/MBVisualInstructionComponent.swift

+
+     If the value is `[.left", .straight]`, the driver can go straight or left from that lane. This is only set when the `component` is a lane.
+     */
+    @objc public var directions: LaneIndication


This should be an array of LaneIndication.

edit: Oh it is, internally.

Let's also rename this indications to follow other patterns in this lib.

@bsudekum Yep, it's already an array... 😸

1ec5 · 2018-05-18T20:05:33Z

MapboxDirections/MBVisualInstructionBanner.swift

+    /**
+     The information about the next maneuver (the one after the upcoming maneuver). This detail gives the driver heads up when the current step is short.
+     */
+    @objc public let subInstruction: VisualInstruction?


Rename this property to subinstruction (and elsewhere in this PR). Precedents for this include NSCompoundPredicate.subpredicates.

@1ec5 I understand your preference for subinstruction based off, NSCompoundPredicate.subpredicates.
We should go for consistency here.

In this context, it feels out of place when subinstruction variable reads differently from, primaryInstruction and secondaryInstruction.

https://github.com/mapbox/MapboxDirections.swift/pull/258/files#diff-8aa869dddbf0e52feb01bd7652980dfbL42

“Sub” isn’t a word.

@1ec5 okay, if that's your notion, then we're better off with a real English word like tertiary --> tertiaryInstruction. Which makes this consistent.

PS: The initial lowercase of the entire variable is less favorable.

1ec5 · 2018-05-18T20:06:34Z

MapboxDirections/MBVisualInstructionBanner.swift

@@ -21,6 +21,11 @@ open class VisualInstructionBanner: NSObject, NSSecureCoding {
     */
    @objc public let secondaryInstruction: VisualInstruction?

+    /**
+     The information about the next maneuver (the one after the upcoming maneuver). This detail gives the driver heads up when the current step is short.


A visual instruction that is presented simultaneously to provide information about an additional maneuver that occurs in rapid succession.

1ec5 · 2018-05-18T20:08:23Z

MapboxDirections/MBVisualInstructionComponent.swift

+    /**
+     An array indicating which directions you can go from a lane (left, right, or straight).
+
+     If the value is `[.left", .straight]`, the driver can go left or straight from that lane. This is only set when the `component` is a `lane`.


There are a couple typos in the first sentence. How about:

If the value contains both LaneIndication.left and LaneIndication.straightAhead…

1ec5 · 2018-05-18T20:10:08Z

MapboxDirections/MBVisualInstructionComponent.swift

+    /**
+     An array indicating which directions you can go from a lane (left, right, or straight).
+
+     If the value is `[.left", .straight]`, the driver can go left or straight from that lane. This is only set when the `component` is a `lane`.


This property is set to nil unless the type property is set to VisualInstructionComponentType.lane.

@1ec5 FYI, LaneIndication is an array of lane indications. For obj-C compatibility, we shouldn't set this to an optional array.

LaneIndication is an OptionSet, not an array. It just happens that Swift uses the same syntax for option sets as it does for arrays. We should say the property is set to the empty set (or 0 in Objective-C) in this case, and we should describe the property as an option set, not an array.

Okay. Noted. In which case, we will indicate that this OptionSet is by default an empty set. Hence, we shall proceed to keep this as a type isActiveLane as a type LaneIndication.

1ec5 · 2018-05-18T20:12:33Z

MapboxDirections/MBVisualInstructionComponent.swift

+
+     If multiple lanes are active, then they can all be used to complete the upcoming maneuver. This value is set to `false` by default.
+     */
+    @objc public var isLaneActive: Bool = false


I think we should strive for more similarities with how lane indications are normally exposed. It’s poor API design to expose multiple mutually exclusive properties that are optionally set depending on each other. Consider making VisualInstructionComponent an umbrella class or protocol for subclasses such as LaneInstructionComponent – isActive can then be a property on that subclass.

Better yet, instead of a bunch of lane instruction components, build up a new Intersection object that can have a bunch of approachLanes and usableApproachLanes.

1ec5 · 2018-05-18T20:13:51Z

MapboxDirections/MBVisualInstructionComponent.swift

+
+     If multiple lanes are active, then they can all be used to complete the upcoming maneuver. This value is set to `false` by default.
+     */
+    @objc public var isLaneActive: Bool = false


Rename this property to isActiveLane, or isActive if you end up refactoring the class. Also refine the getter and setter names in Objective-C: -activeLane should be the getter, and -setActiveLane: should be the setter.

1ec5 · 2018-05-18T20:15:01Z

MapboxDirections/MBVisualInstructionComponent.swift

     */
-    @objc public init(type: VisualInstructionComponentType, text: String?, imageURL: URL?, abbreviation: String?, abbreviationPriority: Int) {
+    @objc public init(type: VisualInstructionComponentType, text: String?, imageURL: URL?, abbreviation: String?, abbreviationPriority: Int, indications: LaneIndication = LaneIndication(), isLaneActive: Bool = false) {


This is a lot of arguments for an initializer. Since these parameters aren’t always relevant, remove them and make the corresponding properties non-constant, so that the caller can set the properties after initializing the object.

1ec5 · 2018-05-18T20:15:22Z

MapboxDirections/MBVisualInstructionType.swift

@@ -6,6 +6,11 @@ import Foundation
 @objc(MBVisualInstructionComponentType)
 public enum VisualInstructionComponentType: Int, CustomStringConvertible {

+    /**
+     The step does not have a particular visual instruction component type associated with it.


What is the developer supposed to do with a none instruction? Hide it?

… data.

…d to the VisualInstructionComponent.

…nit function.

…instructions.

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

MapboxDirections/MBVisualInstructionComponent.swift

+    // MARK: Component/NSSecureCoding Protocols Variables
+    @objc public var text: String?
+
+    @objc public var type: VisualInstructionComponentType


These properties could use some documentation.

JThramer

Looks pretty good, but there are some things I thought I would point out.

JThramer · 2018-06-01T21:22:35Z

MapboxDirections/MBVisualInstruction.swift

+        let componentsDictionary = json["components"] as? [JSONDictionary]
+        var components = [MBComponentRepresentable]()
+
+        var maneuverType: ManeuverType = .none


This works, and it's not really a huge deal or anything, but IMHO argument requirements should be treated exactly the same when you initialize through coder as if you were to initialize for MBVisualInstruction.init(text:maneuverType:maneuverDirection:components:degrees).

Instead of assuming default values, I would consider making MBVisualInstruction.init(json:) a fail-able initializer that returns nil if we don't find a value in the encoder for the required arguments of the designated initializer.

@JThramer this pre-existing and basically refactored from https://github.com/mapbox/MapboxDirections.swift/pull/258/files/916531f27e054235e599a9f6fb9e9a8955533655#diff-361a2ac3a3f3ee2f7a0fe4bbdd15f1deL59

The deduction from the existing logic is, even if we provided a fail-able initializer, the nil value will also always return .none -- so it's a choice between being proactive vs. reactive.

JThramer · 2018-06-01T22:31:36Z

MapboxDirections/MBComponentRepresentable.swift

+The component representable protocol that comprises what the instruction banner should display.
+ */
+@objc(MBComponentRepresentable)
+public protocol MBComponentRepresentable: class, NSSecureCoding { }


Type prefixes in swift are somewhat of an anti-pattern, unless your team has a special reason (and convention) for using them. They were a convention in Objective-C because all namespaces in that language are global, but with swift's introduction of modules, this pattern is more or less deprecated. This is because even if you call your class UIViewController, MyApp.UIViewController() is different from UIKit.UIViewController().

Naming the OBJ-C bridge MBComponentRepresentable is a good call though and should be kept.

JThramer · 2018-06-01T22:51:17Z

MapboxDirections/MBVisualInstructionBanner.swift

+    /**
+     A visual instruction that is presented simultaneously to provide information about an additional maneuver that occurs in rapid succession.
+     */
+    @objc public let tertiaryInstruction: VisualInstruction?


This is good (abstracted because it's either a lane, or a turn component), but it may be nice to specifically call out how this property is used in our current implementation.

…ion components.

bsudekum · 2018-06-12T15:32:44Z

MapboxDirections/MBLaneIndicationComponent.swift

+    @objc public var indications: LaneIndication
+
+    /**
+     The boolean that indicates whether the component is a lane and can be used to complete the upcoming maneuver.


I think since this is a LaneIndicationComponent, we already know it's a lane. I think this is more about whether the lane can be used to complete the maneuver.

bsudekum · 2018-06-12T15:33:28Z

MapboxDirections/MBLaneIndicationComponent.swift

+    }
+
+    public func encode(with coder: NSCoder) {
+        coder.encode(indications, forKey: "directions")


Rename this to coder.encode(indications, forKey: "indications")

bsudekum · 2018-06-12T15:35:57Z

MapboxDirections/MBVisualInstruction.swift

-        let maneuverDirection = ManeuverDirection(description: json["modifier"] as! String)  ?? .none
-        let textComponents = (json["components"] as! [JSONDictionary]).map {
-            VisualInstructionComponent(json: $0)
+        let componentsDictionary = json["components"] as? [JSONDictionary]


Inline this line into the if let down below instead of pulling this out into a variable.

bsudekum · 2018-06-12T15:37:52Z

MapboxDirections/MBVisualInstructionBanner.swift

+    /**
+     A visual instruction that is presented simultaneously to provide information about an additional maneuver that occurs in rapid succession.
+
+     This instruction could either contain the visual layout information or the lane information about the next maneuver after the upcoming maneuver.


This instruction could either contain the visual layout information or the lane information about the upcoming maneuver.

bsudekum · 2018-06-12T15:41:21Z

MapboxDirectionsTests/IntructionsTests.swift

+        XCTAssertEqual(visualInstructions?.first?.tertiaryInstruction?.maneuverDirection, .right)
+        XCTAssertEqual(laneIndicationComponent.isUsable, true)
+        XCTAssertEqual(laneIndicationComponent.indications, [.left, .straightAhead])
+        XCTAssertEqual(visualInstructions?.first?.drivingSide, .right)


So does this step have both lane information and then information? I think the implementation on the server prevents this from ever occurring.

Will re-check the JSON response and relay my finding.

bsudekum

Looking good, a few minor comments.

…ive lanes.

…ructions with Lane indications.

bsudekum · 2018-06-18T17:04:08Z

MapboxDirectionsTests/IntructionsTests.swift

+        let step = route!.legs.first!.steps.first!
+        let visualInstructions = step.instructionsDisplayedAlongStep
+
+        let tertiaryInstruction = visualInstructions?.first?.tertiaryInstruction


@vincethecoder getting close! I think the final piece is to now add a test for a tertiaryInstruction where it's properties are non-nil. IE test for a then instruction instead of a lane instruction.

@bsudekum accounted for here: https://github.com/mapbox/MapboxDirections.swift/pull/258/files#diff-c645476c0ab60998ed8e475352ee8bdcR223

…alInstructions

…ructions. Refactored function name testTertiaryInstructions.

bsudekum

I can't approve my own PR, but I approve.

vincethecoder

LGTM /cc @bsudekum

…ializer.

Bobby Sudekum added 3 commits April 9, 2018 11:48

And then

b60c9a3

Add then

e4f8f3a

fix

5b00f8c

bsudekum commented Apr 10, 2018

View reviewed changes

vincethecoder added 3 commits May 16, 2018 17:47

Merge branch 'master' into then

93a883d

# Conflicts: # MapboxDirections/MBVisualInstruction.swift

Added a new visual instruction type, lane. Updated the visual compone…

bda1e28

…nt with directions and active properties. Modified the mapping of text components for the visual instructions initializer.

Updates to retrieve the active value from JSON dsta.

dfc2842

vincethecoder reviewed May 17, 2018

View reviewed changes

bsudekum commented May 17, 2018

View reviewed changes

Updated how text components visual instruction is parsed. Refactored …

71bd911

…variable name, active. Removed unnecessary comment in visual instruction type.

vincethecoder assigned vincethecoder and unassigned vincethecoder May 18, 2018

bsudekum commented May 18, 2018

View reviewed changes

vincethecoder added 2 commits May 18, 2018 13:20

Refactored directions paramater and variable names to indications.

58e505c

Minor text cleanup.

6b716c5

1ec5 reviewed May 18, 2018

View reviewed changes

Safely unwrapped the maneuver type and direction fields from the json…

ee3b7aa

… data.

vincethecoder mentioned this pull request May 21, 2018

Sub Instruction and lane component instructions integration tests mapbox/mapbox-navigation-ios#1445

Closed

vincethecoder added 4 commits May 21, 2018 12:21

Deleted the none enum type from the visual instruction component types.

630a4d9

Refactored the isLaneActive property. Removed the new parameters adde…

d94febe

…d to the VisualInstructionComponent.

Updated the parsing of the JSON structure in the visual instruction i…

7c549ae

…nit function.

Text update

dd78e95

vincethecoder self-assigned this May 21, 2018

vincethecoder added 4 commits May 21, 2018 16:15

Renamed the subInstructions and the sub variable names.

b84d719

Added tests to validate the new tertiary instruction added to visual …

af610d9

…instructions.

Deleted the fields related to lane indications and active lane status.

8d5d65c

Merge branch 'master' into then

f0682bc

bsudekum commented Jun 1, 2018

View reviewed changes

JThramer reviewed Jun 1, 2018

View reviewed changes

vincethecoder added 5 commits June 11, 2018 12:44

Made updates to a few fields in lane indication and tertiary instruct…

9fd9360

…ion components.

Added documentation to the visual instruction component class variables

f29382a

Refactored MBComponentRepresentable

8becd6c

Updates to give more context of a visual instruction.

84d1e29

Updated the description for tertiary instructions.

0578d7d

bsudekum commented Jun 12, 2018

View reviewed changes

vincethecoder added 6 commits June 14, 2018 09:03

Updated comments for a few variables and field names. Minor refactor.

4ab5aab

Added a json file dedicated to sub instructions with active and inact…

1d611ad

…ive lanes.

Updated the instructions tests with a test dedicated to tertiary inst…

3e1dea8

…ructions with Lane indications.

Merge branch 'master' into then

0392017

Reverted changes to existing instructions json file.

fbc058f

Eliminated the redundant route options configurations.

a6cf422

bsudekum commented Jun 18, 2018

View reviewed changes

vincethecoder added 2 commits June 19, 2018 10:35

Renamed filename, subInstructionsWithLanes. Added a new file, subVisu…

7a9f950

…alInstructions

Added a new test to cover tertiary instructions with visual text inst…

7581816

…ructions. Refactored function name testTertiaryInstructions.

bsudekum commented Jun 19, 2018

View reviewed changes

vincethecoder approved these changes Jun 19, 2018

View reviewed changes

vincethecoder mentioned this pull request Jun 21, 2018

Integrated new directions API fields with the tertiary instruction and lane indications mapbox/mapbox-navigation-ios#1514

Merged

vincethecoder added 2 commits June 26, 2018 16:07

Refactored mapped json data used by Visual Instruction Component init…

fa1c78f

…ializer.

Merge branch 'master' into then

25d5650

bsudekum merged commit 52fbfe5 into master Jul 10, 2018

bsudekum deleted the then branch July 10, 2018 21:02

1ec5 mentioned this pull request Jul 23, 2018

CI Warning: Inline Documentation not matching method signature #284

Closed

Add then component #258

Add then component #258

Conversation

bsudekum commented Apr 9, 2018 • edited by vincethecoder Loading

Choose a reason for hiding this comment

vincethecoder commented May 16, 2018

vincethecoder May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder May 18, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder May 18, 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

bsudekum May 18, 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

vincethecoder May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder May 21, 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

Choose a reason for hiding this comment

vincethecoder May 21, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer 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

bsudekum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

bsudekum left a comment

Choose a reason for hiding this comment

vincethecoder left a comment

Choose a reason for hiding this comment

bsudekum commented Apr 9, 2018 •

edited by vincethecoder

Loading

vincethecoder May 17, 2018 •

edited

Loading

vincethecoder May 18, 2018 •

edited

Loading

vincethecoder May 18, 2018 •

edited

Loading

bsudekum May 18, 2018 •

edited

Loading

vincethecoder May 21, 2018 •

edited

Loading

vincethecoder May 21, 2018 •

edited

Loading

vincethecoder May 21, 2018 •

edited

Loading

vincethecoder Jun 19, 2018 •

edited

Loading