More robust distance formatting #1176
Conversation
| // Fall back on splitting the string on whitespace. | ||
| if unitRange == nil, let spaceRange = distanceString.rangeOfCharacter(from: .whitespaces) { | ||
| unitRange = spaceRange.upperBound..<distanceString.endIndex | ||
| distanceUnit = String(distanceString[unitRange!]) |
There was a problem hiding this comment.
if let instead of force unwrapping.
There was a problem hiding this comment.
unitRange is set to a non-nil value in the line right above this line. if let isn’t suitable because unitRange is used outside the if statement. Specifically, if distanceString doesn’t contain the unit string and doesn’t contain a space, we still need to bail unless we come up with a different way to deemphasize the unit string. (Or should we just treat the entire string as the number?)
|
A lane test addition here would be 💯 |
I think you’re thinking of #1175; this PR is unrelated to lanes. I’ll look into extracting this logic into something unit-testable. 👍 Meanwhile, a snapshot test would be more trouble than it’s worth, because the fix would require a specific locale (he-IL). It is possible to change the formatter’s locale, but unfortunately that doesn’t affect the return value of |
|
OK, I started over from scratch in ceb3c6e89ba545fe005c32532f3ff4113b85cabc: instead of looking for the unit within the distance string, look for the formatted number. This should be more reliable because we’re piggybacking on the distance formatter’s own number formatter. As a failsafe, if no quantity is found, the entire string is emphasized. In that worst-case scenario, it’s better to contort the layout a bit than to make the distance illegible. DistanceFormatter is now responsible for creating the attributed string in the first place. It applies a custom attribute that makes it easier for DistanceLabel to find the quantity and apply presentational attributes to it. I could’ve simplified things further by having DistanceLabel invoke the distance formatter instead of BaseInstructionsBannerView, but I wasn’t sure if it was appropriate for a lightweight class in Style.swift to depend on something in Core Navigation. The upshot is that |
1ec5
changed the title
| assertDistance(2_900, displayed: "२.९ कि॰मी॰", quantity: "२.९") | ||
| assertDistance(3_000, displayed: "३ कि॰मी॰", quantity: "३") | ||
| assertDistance(3_500, displayed: "४ कि॰मी॰", quantity: "४") | ||
| assertDistance(384_400_000, displayed: "३,८४,४०० कि॰मी॰", quantity: "३,८४,४००") |
There was a problem hiding this comment.
In case you’re wondering, this is the distance to the Moon, and the first comma separates the lakhs place from the ten-thousands place.
Worked around a Foundation bug by splitting the distance string on whitespace if LengthFormatter.unitString(fromValue:unit:) doesn’t return the expected value.
1ec5
force-pushed
the
1ec5-rtl-distance-1173
branch
from
bcdf179
to
84620fd
Compare
| let attributedString = NSMutableAttributedString(string: string, attributes: attrs) | ||
| let convertedDistance = distance.converted(to: threshold(for: distance).unit) | ||
| if let quantityString = numberFormatter.string(from: convertedDistance as NSNumber) { | ||
| // NSMutableAttributedString methods accept NSRanges, not NSRanges. |
Implemented DistanceFormatter.attributedString(for:), which applies a “quantity” attribute to the numeric quantity part of the returned attributed string. This method relies on finding the formatted number within the distance string, as opposed to finding the unit, which turned out to be unreliable in some locales. Simplified BaseInstructionsBannerView.distance’s setter to pass this attributed string directly to DistanceLabel instead of attempting to split the distance string on its quantity and unit. DistanceLabel now emphasizes the quantity relative to the unit based on the presence of the quantity attribute. Added tests of attributed distance formatting.
If a quantity isn’t found within the distance string, emphasize the whole thing, for legibility’s sake. For the normal case where that failsafe isn’t applied, test that the unit isn’t emphasized.
1ec5
force-pushed
the
1ec5-rtl-distance-1173
branch
from
84620fd
to
46ebd64
Compare
Implemented
DistanceFormatter.attributedString(for:), which applies aquantityattribute to the numeric quantity part of the returned attributed string. This method relies on finding the formatted number within the distance string, as opposed to finding the unit, which turned out to be unreliable in some locales.Simplified
BaseInstructionsBannerView.distance’s setter to pass this attributed string directly to DistanceLabel instead of attempting to split the distance string on its quantity and unit. DistanceLabel now emphasizes the quantity relative to the unit based on the presence of the quantity attribute.All this is to work around a Foundation bug in which
LengthFormatter.unitString(fromValue:unit:)is unreliable in metricated Hebrew locales, such as Hebrew (Israel).Left: The Hebrew (Israel) locale. Right: The English (United States) locale.
Original description:
Worked around a Foundation bug by splitting the distance string on whitespace ifLengthFormatter.unitString(fromValue:unit:)doesn’t return the expected value. Now the distance to the upcoming maneuver appears in the turn banner, as expected, when the system language is set to Hebrew and the system region is set to a metricated region, such as Israel.Fixes #1173.
/cc @JThramer @frederoni