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 abbreviations #1169

Merged
merged 11 commits into from
Mar 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 6 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
## Changes to the Mapbox Navigation SDK for iOS

## v0.15.0 (tbd)
## Master

#### User Interface
* Added support for abbreviated top banner instructions. [#1169](https://github.com/mapbox/mapbox-navigation-ios/pull/1169)

## v0.15.0 (March 13, 2018)

#### Breaking changes
* `NavigationMapViewDelegate` and `RouteMapViewControllerDelegate`: `navigationMapView(_:didTap:)` is now `navigationMapView(_:didSelect:)` [#1063](https://github.com/mapbox/mapbox-navigation-ios/pull/1063)
Expand Down
2 changes: 1 addition & 1 deletion Cartfile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 3.7
github "mapbox/MapboxDirections.swift" ~> 0.17.0
github "mapbox/MapboxDirections.swift" ~> 0.18.0
github "mapbox/turf-swift" ~> 0.0.3
github "mapbox/mapbox-events-ios" ~> 0.3
github "ceeK/Solar" ~> 2.1.0
Expand Down
2 changes: 1 addition & 1 deletion Cartfile.resolved
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" "3.7.5"
github "ceeK/Solar" "2.1.0"
github "facebook/ios-snapshot-test-case" "2.1.4"
github "mapbox/MapboxDirections.swift" "v0.17.0"
github "mapbox/MapboxDirections.swift" "v0.18.0"
github "mapbox/mapbox-events-ios" "v0.3.1"
github "mapbox/mapbox-voice-swift" "v0.0.1"
github "mapbox/turf-swift" "v0.0.4"
Expand Down
2 changes: 1 addition & 1 deletion MapboxCoreNavigation.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ Pod::Spec.new do |s|
s.requires_arc = true
s.module_name = "MapboxCoreNavigation"

s.dependency "MapboxDirections.swift", "~> 0.17.0"
s.dependency "MapboxDirections.swift", "~> 0.18.0"
s.dependency "MapboxMobileEvents", "~> 0.3"
s.dependency "Turf", "~> 0.0.4"

Expand Down
2 changes: 1 addition & 1 deletion MapboxNavigation-Documentation.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Pod::Spec.new do |s|
s.requires_arc = true
s.module_name = "MapboxNavigation"

s.dependency "MapboxDirections.swift", "~> 0.17.0"
s.dependency "MapboxDirections.swift", "~> 0.18.0"
s.dependency "Mapbox-iOS-SDK", "~> 3.6"
s.dependency "MapboxMobileEvents", "~> 0.3"
s.dependency "Solar", "~> 2.1"
Expand Down
49 changes: 7 additions & 42 deletions MapboxNavigation.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

70 changes: 0 additions & 70 deletions MapboxNavigation/Abbreviations.swift

This file was deleted.

116 changes: 101 additions & 15 deletions MapboxNavigation/InstructionPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,26 +14,63 @@ class InstructionPresenter {
var onShieldDownload: ShieldDownloadCompletion?

private let imageRepository = ImageRepository.shared

func attributedText() -> NSAttributedString {
guard let label = self.label else {
return NSAttributedString()
}

let string = NSMutableAttributedString()

for component in instruction {
fittedAttributedComponents().forEach { string.append($0) }
return string
}

func fittedAttributedComponents() -> [NSAttributedString] {
guard let label = self.label else { return [] }
var attributedPairs = self.attributedPairs()
let availableBounds = label.availableBounds()
let totalWidth = attributedPairs.attributedStrings.map { $0.size() }.reduce(.zero, +).width
let stringFits = totalWidth <= availableBounds.width

guard !stringFits else { return attributedPairs.attributedStrings }

let indexedComponents = attributedPairs.components.enumerated().map { IndexedVisualInstructionComponent(component: $1, index: $0) }
let filtered = indexedComponents.filter { $0.component.abbreviation != nil }
let sorted = filtered.sorted { $0.component.abbreviationPriority < $1.component.abbreviationPriority }
for component in sorted {
let isFirst = component.index == 0
let joinChar = isFirst ? "" : " "
guard component.component.type == .text else { continue }
guard let abbreviation = component.component.abbreviation else { continue }

attributedPairs.attributedStrings[component.index] = NSAttributedString(string: joinChar + abbreviation, attributes: attributesForLabel(label))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to check we're within bounds here? I'm just slightly weary of bracket lookups like this and how fragile they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree about the fragility, but this could cover up an unwanted bug where we could abbreviate
"West Fremont Avenue" as "West W Ave" or crash due to out of bounds.

So instead, I added an assertion to make sure we would catch such a bug early in a8d8faf

let newWidth = attributedPairs.attributedStrings.map { $0.size() }.reduce(.zero, +).width

if newWidth <= availableBounds.width {
break
}
}

return attributedPairs.attributedStrings
}

typealias AttributedInstructionComponents = (components: [VisualInstructionComponent], attributedStrings: [NSAttributedString])

func attributedPairs() -> AttributedInstructionComponents {
guard let label = self.label else { return (components: [], attributedStrings: []) }
var strings = [NSAttributedString]()
var processedComponents = [VisualInstructionComponent]()
let components = instruction

for component in components {
let isFirst = component == instruction.first
let joinChar = !isFirst ? " " : ""
let joinChar = isFirst ? "" : " "

if let shieldKey = component.shieldKey() {
if let cachedImage = imageRepository.cachedImageForKey(shieldKey) {
string.append(NSAttributedString(string: joinChar))
string.append(attributedString(withFont: label.font, shieldImage: cachedImage))
processedComponents.append(component)
strings.append(attributedString(withFont: label.font, shieldImage: cachedImage))
} else {
// Display road code while shield is downloaded
if let text = component.text {
string.append(NSAttributedString(string: joinChar + text, attributes: attributesForLabel(label)))
processedComponents.append(component)
strings.append(NSAttributedString(string: joinChar + text, attributes: attributesForLabel(label)))
}
shieldImageForComponent(component, height: label.shieldHeight, completion: { [weak self] (image) in
guard image != nil else {
Expand All @@ -45,14 +82,27 @@ class InstructionPresenter {
})
}
} else if let text = component.text {
if component.type == .delimiter && instructionHasDownloadedAllShields() {
continue
// Hide delimiter if one of the adjacent components is a shield
if component.type == .delimiter {
let componentBefore = components.component(before: component)
let componentAfter = components.component(after: component)
if let shieldKey = componentBefore?.shieldKey(),
imageRepository.cachedImageForKey(shieldKey) != nil {
continue
}
if let shieldKey = componentAfter?.shieldKey(),
imageRepository.cachedImageForKey(shieldKey) != nil {
continue
}
}
string.append(NSAttributedString(string: (joinChar + text).abbreviated(toFit: label.availableBounds(), font: label.font), attributes: attributesForLabel(label)))
processedComponents.append(component)
strings.append(NSAttributedString(string: (joinChar + text), attributes: attributesForLabel(label)))
}
}

assert(processedComponents.count == strings.count, "The number of processed components must match the number of attributed strings")

return string
return (components: processedComponents, attributedStrings: strings)
}

private func shieldImageForComponent(_ component: VisualInstructionComponent, height: CGFloat, completion: @escaping (UIImage?) -> Void) {
Expand Down Expand Up @@ -103,3 +153,39 @@ class ShieldAttachment: NSTextAttachment {
return CGRect(x: 0, y: font.descender - image.size.height / 2 + mid + 2, width: image.size.width, height: image.size.height).integral
}
}

extension CGSize {
fileprivate static var greatestFiniteSize = CGSize(width: CGFloat.greatestFiniteMagnitude, height: CGFloat.greatestFiniteMagnitude)
Copy link
Contributor

Choose a reason for hiding this comment

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

@bsudekum should we consider an MB (or whatever) prefix to specific vars we add to extensions. This can prevent any future variable conflicts or overrides with Apple iOS specific classes, structs, enum etc. It would also help developers identify MB specific should we exposed certain variables in the future.

Copy link
Contributor

@frederoni frederoni Mar 9, 2018

Choose a reason for hiding this comment

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

Not necessary for private or fileprivate where we don't expose anything to ObjC or even outside this file to prevent conflicts and polluted namespaces.


fileprivate static func +(lhs: CGSize, rhs: CGSize) -> CGSize {
return CGSize(width: lhs.width + rhs.width, height: lhs.height + rhs.height)
}
}

fileprivate struct IndexedVisualInstructionComponent {
let component: Array<VisualInstructionComponent>.Element
let index: Array<VisualInstructionComponent>.Index
}

extension Array where Element == VisualInstructionComponent {

fileprivate func component(before component: VisualInstructionComponent) -> VisualInstructionComponent? {
guard let index = self.index(of: component) else {
return nil
}
if index > 0 {
return self[index-1]
}
return nil
}

fileprivate func component(after component: VisualInstructionComponent) -> VisualInstructionComponent? {
guard let index = self.index(of: component) else {
return nil
}
if index+1 < self.endIndex {
return self[index+1]
}
return nil
}
}
3 changes: 1 addition & 2 deletions MapboxNavigation/InstructionsBannerView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ open class BaseInstructionsBannerView: UIControl {
override open func prepareForInterfaceBuilder() {
super.prepareForInterfaceBuilder()
maneuverView.isStart = true

primaryLabel.instruction = [VisualInstructionComponent(type: .destination, text: "Primary text label", imageURL: nil, maneuverType: .none, maneuverDirection: .none)]
primaryLabel.instruction = [VisualInstructionComponent(type: .text, text: "Primary text label", imageURL: nil, maneuverType: .none, maneuverDirection: .none, abbreviation: nil, abbreviationPriority: NSNotFound)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another observation as well. Should we consider wrapping our strings in NSLocalizedString to aid future translation work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Abbreviations will be localized server-side starting from this PR.


distance = 100
}
Expand Down
2 changes: 1 addition & 1 deletion MapboxNavigation/NextBannerView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ open class NextBannerView: UIView {
override open func prepareForInterfaceBuilder() {
super.prepareForInterfaceBuilder()
maneuverView.isEnd = true
instructionLabel.instruction = [VisualInstructionComponent(type: .destination, text: "Next step", imageURL: nil, maneuverType: .none, maneuverDirection: .none)]
instructionLabel.instruction = [VisualInstructionComponent(type: .text, text: "Next step", imageURL: nil, maneuverType: .none, maneuverDirection: .none, abbreviation: nil, abbreviationPriority: NSNotFound)]
Copy link
Contributor

@vincethecoder vincethecoder Mar 9, 2018

Choose a reason for hiding this comment

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

NSLocalizedString on Next step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a placeholder.

Copy link
Contributor

Choose a reason for hiding this comment

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

This placeholder is never shown on the UI, I reckon.

(@frederoni mentioned that localization will be done at server-side... so cool 👍)

}

func setupLayout() {
Expand Down
81 changes: 0 additions & 81 deletions MapboxNavigation/Resources/Base.lproj/Abbreviations.csv.plist

This file was deleted.

Loading