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

Add then component #258

Merged
merged 46 commits into from
Jul 10, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
b60c9a3
And then
Apr 9, 2018
e4f8f3a
Add then
Apr 9, 2018
5b00f8c
fix
Apr 9, 2018
93a883d
Merge branch 'master' into then
vincethecoder May 16, 2018
bda1e28
Added a new visual instruction type, lane. Updated the visual compon…
vincethecoder May 17, 2018
dfc2842
Updates to retrieve the active value from JSON dsta.
vincethecoder May 17, 2018
71bd911
Updated how text components visual instruction is parsed. Refactored …
vincethecoder May 18, 2018
58e505c
Refactored directions paramater and variable names to indications.
vincethecoder May 18, 2018
6b716c5
Minor text cleanup.
vincethecoder May 18, 2018
ee3b7aa
Safely unwrapped the maneuver type and direction fields from the json…
vincethecoder May 19, 2018
630a4d9
Deleted the none enum type from the visual instruction component types.
vincethecoder May 21, 2018
d94febe
Refactored the isLaneActive property. Removed the new parameters adde…
vincethecoder May 21, 2018
7c549ae
Updated the parsing of the JSON structure in the visual instruction i…
vincethecoder May 21, 2018
dd78e95
Text update
vincethecoder May 21, 2018
b84d719
Renamed the subInstructions and the sub variable names.
vincethecoder May 21, 2018
af610d9
Added tests to validate the new tertiary instruction added to visual …
vincethecoder May 21, 2018
8d5d65c
Deleted the fields related to lane indications and active lane status.
vincethecoder May 22, 2018
f0682bc
Merge branch 'master' into then
vincethecoder May 22, 2018
2264160
Deleted the lane enum type within the visual instruction types.
vincethecoder May 22, 2018
489338e
Added the lane indications related components and tests.
vincethecoder May 22, 2018
d9865e4
Modified instructions json with to include a lane indication for tert…
vincethecoder May 22, 2018
ecd20ab
Updated test cases to check for an active lane and lane indications.
vincethecoder May 22, 2018
9b66591
Added an additional test for non lane text components and checks for …
vincethecoder May 22, 2018
49af516
Updates to reflect missing abbreviation and abbreviation priority.
vincethecoder May 22, 2018
facefa5
Refactor that creates a base component class. Created the lane indica…
vincethecoder May 23, 2018
ae6ac98
Re-added the component file, to make it discoverable on bitrise CI.
vincethecoder May 23, 2018
fe30487
Fixed missing targets on newly added classes/files.
vincethecoder May 23, 2018
93f0d2b
Revised the component change to a protocol. Updates to instructions t…
vincethecoder May 24, 2018
e54e52f
Renamed Component protocol. Deleted redundant properties in the LaneI…
vincethecoder May 30, 2018
f4d363d
Updated the instructions tests' instruction components.
vincethecoder May 30, 2018
916531f
Minor refactor.
vincethecoder May 30, 2018
9fd9360
Made updates to a few fields in lane indication and tertiary instruct…
vincethecoder Jun 11, 2018
f29382a
Added documentation to the visual instruction component class variables
vincethecoder Jun 11, 2018
8becd6c
Refactored MBComponentRepresentable
vincethecoder Jun 11, 2018
84d1e29
Updates to give more context of a visual instruction.
vincethecoder Jun 11, 2018
0578d7d
Updated the description for tertiary instructions.
vincethecoder Jun 11, 2018
4ab5aab
Updated comments for a few variables and field names. Minor refactor.
vincethecoder Jun 14, 2018
1d611ad
Added a json file dedicated to sub instructions with active and inact…
vincethecoder Jun 18, 2018
3e1dea8
Updated the instructions tests with a test dedicated to tertiary inst…
vincethecoder Jun 18, 2018
0392017
Merge branch 'master' into then
vincethecoder Jun 18, 2018
fbc058f
Reverted changes to existing instructions json file.
vincethecoder Jun 18, 2018
a6cf422
Eliminated the redundant route options configurations.
vincethecoder Jun 18, 2018
7a9f950
Renamed filename, subInstructionsWithLanes. Added a new file, subVisu…
vincethecoder Jun 19, 2018
7581816
Added a new test to cover tertiary instructions with visual text inst…
vincethecoder Jun 19, 2018
fa1c78f
Refactored mapped json data used by Visual Instruction Component init…
vincethecoder Jun 26, 2018
25d5650
Merge branch 'master' into then
vincethecoder Jun 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions MapboxDirections/MBVisualInstruction.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,20 @@ open class VisualInstruction: NSObject, NSSecureCoding {
let text = json["text"] as? String
let maneuverType = ManeuverType(description: json["type"] as! String) ?? .none
let maneuverDirection = ManeuverDirection(description: json["modifier"] as! String) ?? .none
let textComponents = (json["components"] as! [JSONDictionary]).map {
VisualInstructionComponent(json: $0)
let textComponents: [VisualInstructionComponent] = (json["components"] as! [JSONDictionary]).map { record in
let type = VisualInstructionComponentType(description: record["type"] as! String) ?? .none
var directions = LaneIndication()
if let laneDirections = record["directions"] as? [String],
let laneIndication = LaneIndication(descriptions: laneDirections) {
directions = laneIndication
}
return VisualInstructionComponent(type: type,
text: record["text"] as? String,
imageURL: URL(string: (record["imageBaseURL"] as? String) ?? ""),
abbreviation: record["abbr"] as? String,
abbreviationPriority: record["abbr_priority"] as? Int ?? NSNotFound,
directions: directions,
isLaneActive: record["active"] as? Bool ?? false)
}

let degrees = json["degrees"] as? CLLocationDegrees ?? 180
Expand Down Expand Up @@ -97,6 +109,6 @@ open class VisualInstruction: NSObject, NSSecureCoding {
coder.encode(maneuverType, forKey: "maneuverType")
coder.encode(maneuverDirection, forKey: "maneuverDirection")
coder.encode(finalHeading, forKey: "degrees")
coder.encode(textComponents, forKey: "textComponents")
}
}

18 changes: 16 additions & 2 deletions MapboxDirections/MBVisualInstructionBanner.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

*/
@objc public let subInstruction: VisualInstruction?
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@vincethecoder vincethecoder May 21, 2018

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

“Sub” isn’t a word.

Copy link
Contributor

@vincethecoder vincethecoder May 21, 2018

Choose a reason for hiding this comment

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

@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.


/**
Which side of a bidirectional road the driver should drive on, also known as the rule of the road.
*/
Expand All @@ -38,14 +43,20 @@ open class VisualInstructionBanner: NSObject, NSSecureCoding {

let primary = json["primary"] as! JSONDictionary
let secondary = json["secondary"] as? JSONDictionary
let sub = json["sub"] as? JSONDictionary

let primaryInstruction = VisualInstruction(json: primary)
var secondaryInstruction: VisualInstruction? = nil
if let secondary = secondary {
secondaryInstruction = VisualInstruction(json: secondary)
}

self.init(distanceAlongStep: distanceAlongStep, primaryInstruction: primaryInstruction, secondaryInstruction: secondaryInstruction, drivingSide: drivingSide)
var subInstruction: VisualInstruction? = nil
if let sub = sub {
subInstruction = VisualInstruction(json: sub)
}

self.init(distanceAlongStep: distanceAlongStep, primaryInstruction: primaryInstruction, secondaryInstruction: secondaryInstruction, subInstruction: subInstruction, drivingSide: drivingSide)
}

/**
Expand All @@ -56,10 +67,11 @@ open class VisualInstructionBanner: NSObject, NSSecureCoding {
- parameter secondaryInstruction: Less important details about the `RouteStep`.
- parameter drivingSide: Which side of a bidirectional road the driver should drive on.
*/
@objc public init(distanceAlongStep: CLLocationDistance, primaryInstruction: VisualInstruction, secondaryInstruction: VisualInstruction?, drivingSide: DrivingSide) {
@objc public init(distanceAlongStep: CLLocationDistance, primaryInstruction: VisualInstruction, secondaryInstruction: VisualInstruction?, subInstruction: VisualInstruction?, drivingSide: DrivingSide) {
self.distanceAlongStep = distanceAlongStep
self.primaryInstruction = primaryInstruction
self.secondaryInstruction = secondaryInstruction
self.subInstruction = subInstruction
self.drivingSide = drivingSide
}

Expand All @@ -77,6 +89,7 @@ open class VisualInstructionBanner: NSObject, NSSecureCoding {
}
self.primaryInstruction = primaryInstruction
self.secondaryInstruction = decoder.decodeObject(of: VisualInstruction.self, forKey: "secondary")
self.subInstruction = decoder.decodeObject(of: VisualInstruction.self, forKey: "sub")
}

open static var supportsSecureCoding = true
Expand All @@ -85,6 +98,7 @@ open class VisualInstructionBanner: NSObject, NSSecureCoding {
coder.encode(distanceAlongStep, forKey: "distanceAlongStep")
coder.encode(primaryInstruction, forKey: "primary")
coder.encode(secondaryInstruction, forKey: "secondary")
coder.encode(subInstruction, forKey: "sub")
coder.encode(drivingSide, forKey: "drivingSide")
}
}
43 changes: 41 additions & 2 deletions MapboxDirections/MBVisualInstructionComponent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@ open class VisualInstructionComponent: NSObject, NSSecureCoding {
*/
@objc public var abbreviationPriority: Int = NSNotFound

/**
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 straight or left from that lane. This is only set when the `component` is a lane.
*/
@objc public var directions: LaneIndication
Copy link
Author

@bsudekum bsudekum May 18, 2018

Choose a reason for hiding this comment

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

This should be an array of LaneIndication.

edit: Oh it is, internally.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


/**
The boolean that indicates whether the component is a lane and can be used to complete the upcoming maneuver.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/**
Initializes a new visual instruction component object based on the given JSON dictionary representation.

Expand All @@ -71,7 +85,18 @@ open class VisualInstructionComponent: NSObject, NSSecureCoding {
imageURL = URL(string: "\(baseURL)@\(Int(scale))x.png")
}

self.init(type: type, text: text, imageURL: imageURL, abbreviation: abbreviation, abbreviationPriority: abbreviationPriority)
var isLaneActive = false
if let active = json["active"] as? Bool {
isLaneActive = active
}

var directions = LaneIndication()
if let laneDirections = json["directions"] as? [String],
let laneIndication = LaneIndication(descriptions: laneDirections) {
directions = laneIndication
}

self.init(type: type, text: text, imageURL: imageURL, abbreviation: abbreviation, abbreviationPriority: abbreviationPriority, directions: directions, isLaneActive: isLaneActive)
}

/**
Expand All @@ -82,13 +107,17 @@ open class VisualInstructionComponent: NSObject, NSSecureCoding {
- parameter imageURL: The URL to an image representation of this component.
- parameter abbreviation: An abbreviated representation of `text`.
- parameter abbreviationPriority: The priority for which the component should be abbreviated.
- parameter directions: The possibile directions to go from a lane component.
- parameter isLaneActive: The flag to indicate that the upcoming maneuver can be completed with a lane component.
*/
@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, directions: LaneIndication = LaneIndication(), isLaneActive: Bool = false) {
self.text = text
self.imageURL = imageURL
self.type = type
self.abbreviation = abbreviation
self.abbreviationPriority = abbreviationPriority
self.directions = directions
self.isLaneActive = isLaneActive
}

@objc public required init?(coder decoder: NSCoder) {
Expand All @@ -111,6 +140,14 @@ open class VisualInstructionComponent: NSObject, NSSecureCoding {
self.abbreviation = abbreviation

abbreviationPriority = decoder.decodeInteger(forKey: "abbreviationPriority")

guard let directions = decoder.decodeObject(of: [NSArray.self, NSString.self], forKey: "directions") as? [String] else {
return nil
}

self.directions = LaneIndication(descriptions: directions) ?? LaneIndication()

self.isLaneActive = decoder.decodeObject(forKey: "active") as? Bool ?? false
}

open static var supportsSecureCoding = true
Expand All @@ -120,6 +157,8 @@ open class VisualInstructionComponent: NSObject, NSSecureCoding {
coder.encode(type, forKey: "type")
coder.encode(abbreviation, forKey: "abbreviation")
coder.encode(abbreviationPriority, forKey: "abbreviationPriority")
coder.encode(directions, forKey: "directions")
coder.encode(isLaneActive, forKey: "active")
}
}

18 changes: 17 additions & 1 deletion MapboxDirections/MBVisualInstructionType.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

*/
case none

/**
The component separates two other destination components.

Expand All @@ -30,6 +35,11 @@ public enum VisualInstructionComponentType: Int, CustomStringConvertible {
*/
case exit

/**
The component contains information about which lanes can be used to complete the maneuver.
*/
case lane
Copy link
Contributor

Choose a reason for hiding this comment

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

This case becomes unnecessary because LaneIndicationComponent no longer has a type property.

Choose a reason for hiding this comment

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

Key


/**
A component contains an exit number.
*/
Expand All @@ -48,14 +58,18 @@ public enum VisualInstructionComponentType: Int, CustomStringConvertible {
type = .exit
case "exit-number":
type = .exitCode
case "lane":
type = .lane
default:
return nil
type = .none
}
self.init(rawValue: type.rawValue)
}

public var description: String {
switch self {
case .none:
return "none"
case .delimiter:
return "delimiter"
case .image:
Expand All @@ -66,6 +80,8 @@ public enum VisualInstructionComponentType: Int, CustomStringConvertible {
return "exit"
case .exitCode:
return "exit-number"
case .lane:
return "lane"
}
}
}