-
Notifications
You must be signed in to change notification settings - Fork 301
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
Add abbreviations #1169
Conversation
@@ -73,7 +73,7 @@ open class BaseInstructionsBannerView: UIControl { | |||
|
|||
func set(_ instruction: VisualInstruction?) { | |||
let secondaryInstruction = instruction?.secondaryTextComponents | |||
primaryLabel.numberOfLines = secondaryInstruction == nil ? 2 : 1 | |||
primaryLabel.numberOfLines = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sort of digging this change. This means we will abbreviate more, but will only use one line ever per primary/secondary instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would simplify the layout and snapshot tests. Note that this method
mapbox-navigation-ios/MapboxNavigation/InstructionsBannerViewLayout.swift
Lines 113 to 117 in 2890d2f
func baselineAlignInstructions() { | |
_separatorView.isHidden = true | |
centerYConstraints.forEach { $0.isActive = false } | |
baselineConstraints.forEach { $0.isActive = true } | |
} |
and accompanying arrays of constraints would be obsolete if we decide to make this change persistent.
if let text = component.text { | ||
string.append(NSAttributedString(string: joinChar + text, attributes: attributesForLabel(label))) | ||
if let text = joinCharPlusText { | ||
string.append(NSAttributedString(string: (text).abbreviated(toFit: label.availableBounds(), font: label.font, possibleAbbreviation: joinCharPlusAbbreviation), attributes: attributesForLabel(label))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about adding some sort of counter that tracks the current abbreviation priority level. We'd then pass it into this function and then it'd tell us if the priority level impacted whether a abbreviation was used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to first sort all the components, then try and fit them, then un-sort them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking you would need at most two nested loops, one to iterate over priority level i
and one to iterate over components j
. The inner loop would join strings together, using component[j].abbreviation
if component[j].abbreviationPriority <= i
, else using component[j].text
. Does that make sense?
Attempted to use priority level here: 1de9000 It's pretty unperformant, gist of what is happening:
All in all, there are quite a few loops here. Open to ideas. |
MapboxNavigation/Abbreviations.swift
Outdated
/// Returns the string abbreviated only as much as necessary to fit the given width and font. | ||
func abbreviated(toFit bounds: CGRect, font: UIFont) -> String { | ||
func abbreviated(toFit bounds: CGRect, font: UIFont, possibleAbbreviation: String?) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be clearer and more decoupled if this method took a [VisualInstructionsComponent]
with
abbreviations and priorities and we would leave the InstructionPresenter
class intact.
@bsudekum I think there’s too much space left here to truncate it onto one line: |
let totalWidth = attrComponents.map { $0.size() }.reduce(.zero, +).width | ||
let stringFits = totalWidth <= availableBounds.width | ||
|
||
if stringFits { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, a guard might be easier to read here.
Adding a test that abbreviates one but not all possible abbreviations would be another good test to add. |
@frederoni do you think there is a way we could try and abbreviate first before using two lines? In my opinion, a single line that reads |
@bsudekum would this type of abbreviation happen on the server or in the client? /cc @mcwhittemore @danpaz |
@willwhite on the client, given the abbreviations the server has given us. The switch statement that would occur:
|
@bsudekum that should be the case already. Added another snapshot test for iPhone X that renders like this with artificial abbreviations and priorities: Could it be that there are no provided abbreviations in your response through West Fremont Avenue? |
@bsudekum is this the route you took?
Doesn't seem to be any abbreviations for "West Fremont Avenue". 🤔 |
guard component.component.type == .text else { continue } | ||
guard let abbreviation = component.component.abbreviation else { continue } | ||
|
||
attributedComponents[component.index] = NSAttributedString(string: joinChar + abbreviation, attributes: attributesForLabel(label)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSAttributedString(string: "\(joinChar)\(abbreviation)", attributes: attributesForLabel(label))
} else { | ||
// Display road code while shield is downloaded | ||
if let text = component.text { | ||
string.append(NSAttributedString(string: joinChar + text, attributes: attributesForLabel(label))) | ||
strings.append(NSAttributedString(string: joinChar + text, attributes: attributesForLabel(label))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strings.append(NSAttributedString(string: "\(joinChar)\(text)", attributes: attributesForLabel(label)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason: As we continuously concatenate the strings in a for-loop, we creating multiple strings to get to the final string. I see we also map the results as well. Just an observation as I reviewed the scope of the function attributedComponents()
and its callee(s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see the distinction. It's just another syntax for concatenating using the string concat operator(+) or string interpolation but the reason indicates that it's more than just the code style?
We do one full iteration first to stringify all components including shields.
"[I-280] West Fremont Avenue"
then one more iteration until the instruction fits on the screen
"[I-280] W Fremont Avenue"
if West had the highest priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederoni difference is negligible with simple code. However, as we iterate, a larger data set it may affect performance. Interpolation does make good use of memory as stated in the abstract. Apple offers this as a given, so why not use it 😄 -- just like how we prefer to use functional programming 🤷♂️
cc @bsudekum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick performance test indicated that using the + operator for concatenating strings is 19 × faster than concatenating using string interpolation so I'm going to leave this intact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frederoni I believe this was more about good use of memory. However, I see how my last mention of performance could have stirred us towards performance measurement.
@@ -48,11 +80,11 @@ class InstructionPresenter { | |||
if component.type == .delimiter && instructionHasDownloadedAllShields() { | |||
continue | |||
} | |||
string.append(NSAttributedString(string: (joinChar + text).abbreviated(toFit: label.availableBounds(), font: label.font), attributes: attributesForLabel(label))) | |||
strings.append(NSAttributedString(string: (joinChar + text), attributes: attributesForLabel(label))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto ☝️ comment on line 68
@@ -103,3 +135,17 @@ 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -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)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -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)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NSLocalizedString
on Next step
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a placeholder.
There was a problem hiding this comment.
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 👍)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside minor suggested changes, LGTM 👍
@frederoni that looks about right to me. Unsure why, but most times I run this branch it crashes at the same spot each time. I investigated it for a second on one crash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
75a6047
to
a0dfee1
Compare
@akitchen would you mind 👀 the changes in a0dfee1? Before this change: After this change: |
@@ -101,12 +101,12 @@ class InstructionsBannerViewIntegrationTests: XCTestCase { | |||
let view = instructionsView() | |||
view.set(makeVisualInstruction(primaryInstruction: instructions, secondaryInstruction: nil)) | |||
|
|||
//Slash should be present until an adjacent shield is downloaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, with two shields next to each other, one exit and one destination shield, we should probably show the delimiter? However, exit types are yet to be added to MapboxDirections, IIRC? /cc @bsudekum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the case you mentioned in this comment makes sense to me.
It seems fine to me, but ultimately this should be a design decision. Is there a separate ticket which calls for this change? |
@1ec5 any thoughts about #1169 (comment)? |
@frederoni Visually, I think this change looks good. |
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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
ad66f8d
to
a8d8faf
Compare
Relies on: mapbox/mapbox-directions-swift#244
This adds abbreviations from the server.
todo:
abbr_priority
key. Right now, we're just abbreviating in order they are given depending on if the text can fit./cc @mapbox/navigation-ios @danpaz