Conversation
There was a problem hiding this comment.
Instead of flattening the list of steps, we should have multiple sections (instead of just one), each section listing the steps for a single leg.
|
Some notes,
/cc @1ec5 @frederoni |
| routeProgress.remainingWaypoints.count > 1 { | ||
| routeProgress.legIndex += 1 | ||
| routeProgress.currentLegProgress.stepIndex = 0 | ||
| routeProgress.currentLegProgress.alertUserLevel = .depart |
There was a problem hiding this comment.
Changing legIndex automatically resets currentLegProgress.
| on the destination of your route. The last coordinate of the route will be | ||
| used if no destination is given. | ||
| */ | ||
| @available(*, deprecated, message: "Destination is no longer support nor necessary. A destination annotation will automatically be added to map given the route.") |
There was a problem hiding this comment.
Note that this shouldn’t have much of an impact on most developers: it isn’t possible to customize this annotation (#213) other than changing its title, subtitle, and coordinates, and none of those properties are likely to be modified by the developer anyways.
There was a problem hiding this comment.
Ah, I missed that we’re exposing navigationMapView(_:imageFor:). In any case, that method will continue to work; it just won’t be possible to say something like if annotation == navigationViewController.destination.
| on the destination of your route. The last coordinate of the route will be | ||
| used if no destination is given. | ||
| */ | ||
| @available(*, deprecated, message: "Destination is no longer support nor necessary. A destination annotation will automatically be added to map given the route.") |
| annotation.coordinate = route.coordinates!.last! | ||
| destination = annotation | ||
| for leg in route.legs { | ||
| if let last = leg.steps.last { |
There was a problem hiding this comment.
This code only places an annotation at the last destination. Should we annotate the intermediate waypoints as well?
There was a problem hiding this comment.
@1ec5 hrm, no this annotates the last step of each leg. Or at least it's supposed to.
There was a problem hiding this comment.
You’re right, I misread this line.
| let annotation = MGLPointAnnotation() | ||
| annotation.coordinate = route.coordinates!.last! | ||
| destination = annotation | ||
| for leg in route.legs { |
There was a problem hiding this comment.
If we’re only looking at the last step, there’s no need to iterate over all the steps:
if let lastStep = route.legs.last?.steps.last {| navigationDelegate?.navigationViewController?(self, didArriveAt: destination) | ||
|
|
||
| if routeProgress.currentLegProgress.alertUserLevel == .arrive, | ||
| routeProgress.remainingWaypoints.count == 0 { |
There was a problem hiding this comment.
Should the developer be notified when the user reaches an intermediate waypoint? I could see use cases for only responding to the final arrival and for responding to intermediate arrivals as well. Perhaps the delegate method should have an additional argument, numberOfWaypointsRemaining, to allow the developer to distinguish between the two cases.
There was a problem hiding this comment.
@1ec5 I was thinking about this but wanted to keep the API the same. I think if we want to notify them when they reach a waypoint, we should either rename the method or add a new method for waypoints.
There was a problem hiding this comment.
The method’s current name implies that it’s called whenever the user reaches a waypoint once we added support for multiple waypoints. Why else would it take a second argument?
How about we add a new method, navigationViewControllerDidArrive(_:), that gets called after navigationViewController(_:didArriveAt:) when arriving at the final waypoint?
There was a problem hiding this comment.
Changed it to a single method that returns the waypoint the user is arriving at for each and every waypoint.
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, titleForHeaderInSection section: Int) -> String? { | ||
| return "Leg \(section + 1)" |
There was a problem hiding this comment.
The leg number is unimportant to the user. Instead, we should title the section with either of the following formats, preferably the first one:
- “Biloxi to Mobile” (see
leg.source,leg.destination) - “I-10” (see
leg.name)
| } | ||
|
|
||
| func tableView(_ tableView: UITableView, titleForHeaderInSection section: Int) -> String? { | ||
| return "Leg \(section + 1)" |
There was a problem hiding this comment.
If there is only one leg, there’s no need for a section header.
|
|
||
|
|
||
| cell.step = leg.steps[indexPath.row] | ||
| cell.step = legs[indexPath.section].steps[indexPath.row] |
| cell.step = legs[indexPath.section].steps[indexPath.row] | ||
|
|
||
| if routeController.routeProgress.currentLegProgress.stepIndex + 1 > indexPath.row { | ||
| if routeController.routeProgress.legIndex + 1 > indexPath.section && routeController.routeProgress.currentLegProgress.stepIndex + 1 > indexPath.row { |
There was a problem hiding this comment.
If the user is on step 5 of leg 1, only the first five steps of section 0 are dimmed out.
There was a problem hiding this comment.
@1ec5 I think that's how it should work though. Are you saying the current step should be dimmed?
There was a problem hiding this comment.
If leg 0 has 10 steps, leg 1 has 10 steps, and the user is on step 5 of leg 2 which has 10 steps, why would all three legs be halfway dimmed?
There was a problem hiding this comment.
Actually, this code is incorrect but in a different way than I first thought. This code is equivalent to: if currentLegIndex >= section && currentLegProgress.stepIndex >= row { dim the cell }. Now if the user is on step 5 of leg 2, the only dimmed cells are those that meet both the following criteria:
- the section number is at least 2; and
- the row number is at least 5
In other words:
- Sections 0 and 1 are completely lit, because they fail criteria (1). 🙅♂️
- Section 2 is dimmed up to row 4 and lit beginning with row 5. 👍
- Sections 3, 4, etc. are each dimmed up to row 4 but lit beginning with row 5, because rows 5 and beyond fail criteria (2). 🙅♂️
| return nil | ||
| } | ||
|
|
||
| let legToFrom = routeController.routeProgress.route.legs[section].description.components(separatedBy: ", ") |
There was a problem hiding this comment.
RouteLeg.description is equivalent to RouteLeg.name. name lists the main streets along the leg, not the leg’s origin and destination. For example, name might be “Market Street, I-10, Main Street”. If you want “Biloxi” and “Mobile”, you need to look at the name properties of RouteLeg’s source and destination properties, which won’t be very useful until mapbox/mapbox-directions-swift#135 is fixed.
|
This is ready for another round of reviews |
| Called when the user arrives at the destination. | ||
| */ | ||
| @objc optional func navigationViewController(_ navigationViewController : NavigationViewController, didArriveAt destination: MGLAnnotation) | ||
| @objc optional func navigationViewController(_ navigationViewController : NavigationViewController, didArriveAt destination: Waypoint) |
There was a problem hiding this comment.
If this gets called on every waypoint, then destination should be waypoint.
| Called when the user arrives at the destination. | ||
| */ | ||
| @objc optional func navigationViewController(_ navigationViewController : NavigationViewController, didArriveAt destination: MGLAnnotation) | ||
| @objc optional func navigationViewController(_ navigationViewController : NavigationViewController, didArriveAt destination: Waypoint) |
There was a problem hiding this comment.
Should we also add navigationViewControllerDidArrive(_:) (with only one argument of type NavigationViewController)?
There was a problem hiding this comment.
I think one method is good enough here.
| tableViewController?.notifyAlertLevelDidChange() | ||
|
|
||
| if routeProgress.currentLegProgress.alertUserLevel == .arrive { | ||
| navigationDelegate?.navigationViewController?(self, didArriveAt: routeProgress.route.routeOptions.waypoints[routeProgress.legIndex + 1]) |
There was a problem hiding this comment.
We should pass in the current route leg’s destination waypoint. Otherwise, if we pass in one of the waypoints used to request the route, its location won’t necessarily correspond to what the Directions API thinks the destination should be (after snapping or whatever).
| for leg in route.legs { | ||
| if let last = leg.steps.last { | ||
| let annotation = MGLPointAnnotation() | ||
| annotation.coordinate = last.coordinates!.last! |
| } | ||
|
|
||
| let sectionName = routeController.routeProgress.route.legs[section].name | ||
| let legToFrom = sectionName.components(separatedBy: ", ") |
There was a problem hiding this comment.
The suggestion in #270 (comment) was to use source and destination, not name.
There was a problem hiding this comment.
I added using source and destination but I also left this code in place for cases where we do not have source and destination.
| cell.step = legs[indexPath.section].steps[indexPath.row] | ||
|
|
||
| if routeController.routeProgress.currentLegProgress.stepIndex + 1 > indexPath.row { | ||
| if routeController.routeProgress.legIndex + 1 > indexPath.section && routeController.routeProgress.currentLegProgress.stepIndex + 1 > indexPath.row { |
There was a problem hiding this comment.
@1ec5 having a hard time getting something to work here. Have any ideas how to accomplish this?
There was a problem hiding this comment.
We need to dim a cell for either of two reasons:
- Either it represents a step that’s part of a leg that the user has already completed,
- Or it represents a completed step in the current leg.
So the correct logic would be:
if indexPath.section < routeController.routeProgress.legIndex
|| (indexPath.section == routeProgress.legIndex
&& indexPath.row <= routeProgress.currentLegProgress.stepIndex) {
// dim the cell
}|
|
||
| let legToFrom = leg.name.components(separatedBy: ", ") | ||
| if legToFrom.count == 2 { | ||
| return "\(legToFrom[0]) to \(legToFrom[1])" |
There was a problem hiding this comment.
Since these are two road names, “Main Street and Market Street” would be clearer than “Main Street to Market Street”.
|
Blocked by Project-OSRM/osrm-text-instructions.swift#24. |
|
|
||
| // Reset the navigation styling to the defaults | ||
| DefaultStyle().apply() |
Examples/Swift/ViewController.swift
Outdated
| let annotation = MGLPointAnnotation() | ||
| annotation.coordinate = coordinates | ||
| mapView.addAnnotation(annotation) | ||
|
|
There was a problem hiding this comment.
This class is inside the application target, so you can say simply Bundle.main. It’s only necessary to use Bundle(for:) inside a framework, since the framework isn’t the main bundle.
Examples/Swift/ViewController.swift
Outdated
|
|
||
| present(customViewController, animated: true, completion: nil) | ||
| } | ||
|
|
There was a problem hiding this comment.
The first part of this selector, confirmationControllerDidConfirm, already contains “confirmation controller”, so controller can be _.
This code path would be a lot cleaner with an unwind segue instead of the delegate pattern…
| Intializes a new `RouteProgress`. | ||
|
|
||
| - parameter route: The route to follow. | ||
| - parameter legIndex: Zero-based index indicating the current leg the user is on. |
There was a problem hiding this comment.
legIndex is the index of the leg to start on.
|
|
||
| let legToFrom = leg.name.components(separatedBy: ", ") | ||
| if legToFrom.count == 2 { | ||
| return "\(legToFrom[0]) to \(legToFrom[1])" |
There was a problem hiding this comment.
Not sure why GitHub thinks #270 (comment) is outdated, but it’s still current.
| let sourceName = leg.source.name | ||
| let destinationName = leg.destination.name | ||
|
|
||
| if let sourceName = sourceName?.nonEmptyString, let destinationName = destinationName?.nonEmptyString { |
There was a problem hiding this comment.
If the source is the user’s unnamed current location and the destination is a named location, we should say “Destination via Street Name and Street Name”.
If the source is a named location and the destination is unnamed for some reason, we should say “Street Name and Street Name”, same as below.
|
More reviews are welcome if anyone is up for it @1ec5 @frederoni. |
1ec5
left a comment
There was a problem hiding this comment.
Thanks for your patience. Some more feedback below.
| let arrowCasingSymbolLayerIdentifier = "arrowCasingSymbolLayer" | ||
| let arrowSymbolSourceIdentifier = "arrowSymbolSource" | ||
| let isOpaqueIdentifier = "isOpaqueIdentifier" | ||
| let isCurrentLeg = "isCurrentLeg" |
There was a problem hiding this comment.
The name isCurrentLeg implies that it’s a Boolean. Since this string is being used as an attribute name, call it currentLegAttribute but give it the value isCurrentLeg.
| func shape(for waypoints: [Waypoint]) -> MGLShape? { | ||
| var features = [MGLPointFeature]() | ||
| let letters = (97...122).map({Character(UnicodeScalar($0))}).map { String(describing:$0).uppercased() } | ||
| let letters = String.localizedStringWithFormat(NSLocalizedString("ALPHABET", bundle: .mapboxNavigation, value: "ABCDEFGHIJKLMNOPQRSTUVWXYZ", comment: "Format string for alphabet;")).components(separatedBy: "") |
There was a problem hiding this comment.
Use an actual separator like a space, because some locales may have multi-character letters in the alphabet, like “IJ” in Dutch.
| func shape(for waypoints: [Waypoint]) -> MGLShape? { | ||
| var features = [MGLPointFeature]() | ||
| let letters = (97...122).map({Character(UnicodeScalar($0))}).map { String(describing:$0).uppercased() } | ||
| let letters = String.localizedStringWithFormat(NSLocalizedString("ALPHABET", bundle: .mapboxNavigation, value: "ABCDEFGHIJKLMNOPQRSTUVWXYZ", comment: "Format string for alphabet;")).components(separatedBy: "") |
There was a problem hiding this comment.
Stray semicolon at the end of the comment. Make sure to point out that each letter should be separated by a space.
| for (waypointIndex, waypoint) in waypoints.enumerated() { | ||
| let feature = MGLPointFeature() | ||
| feature.coordinate = waypoint.coordinate | ||
| feature.attributes = [ "name": letters[waypointIndex] ] |
There was a problem hiding this comment.
A reminder to simplify this code by inlining the letters array, as requested in #270 (comment) and #270 (comment).
There was a problem hiding this comment.
This seems a bit unreadable IMO:
for (waypointIndex, waypoint) in waypoints.enumerated() {
let feature = MGLPointFeature()
feature.coordinate = waypoint.coordinate
feature.attributes = [ "name": String.localizedStringWithFormat(NSLocalizedString("ALPHABET", bundle: .mapboxNavigation, value: "ABCDEFGHIJKLMNOPQRSTUVWXYZ", comment: "Format string for alphabet;")).characters.map { String(describing: $0) }[waypointIndex] ]
features.append(feature)
}There was a problem hiding this comment.
Ah, the code I suggested in #270 (comment) was much more readable, but it no longer makes sense with the localization that we’re doing. How about this:
let letters = NSLocalizedString("ALPHABET", bundle: .mapboxNavigation, value: "ABCDEFGHIJKLMNOPQRSTUVWXYZ", comment: "Base letters in the alphabet for labeling waypoints, separated by spaces").components(separatedBy: " ")
// then in the for loop
feature.attributes = ["name": letters[waypointIndex]]Note that there doesn’t need to be a format string, just an plain localized string.
| for (waypointIndex, waypoint) in waypoints.enumerated() { | ||
| let feature = MGLPointFeature() | ||
| feature.coordinate = waypoint.coordinate | ||
| feature.attributes = [ "name": letters[waypointIndex] ] |
There was a problem hiding this comment.
If the waypoint has a name, how about using the first letter of the name by default? That would be in keeping with how iOS represents contacts who lack photos by their initials.
There was a problem hiding this comment.
What if one waypoint has a name and the other does not? So it'd look like B (for picking up bobby) and then a waypoint with no name B again? Some sort of order makes more sense to me here.
There was a problem hiding this comment.
If any of the waypoints has a name, I think it would be better to label the named ones with initials and leave the unnamed ones… unlabeled.
| let legToFrom = leg.name.components(separatedBy: ", ") | ||
|
|
||
| if let destinationName = destinationName?.nonEmptyString, legToFrom.count > 1 { | ||
| return String.localizedStringWithFormat(NSLocalizedString("WAYPOINT_DESTINATION_VIA_WAYPOINTS_FORMAT", bundle: .mapboxNavigation, value: "%@, via %@", comment: "Format for displaying destination and intermediate waypoints; 1 = source ; 2 = destinations"), destinationName, leg.name.replacingOccurrences(of: ", ", with: " \(String.localizedStringWithFormat(NSLocalizedString("AND", bundle: .mapboxNavigation, value: "and", comment: "Format string for and;"))) ")) |
There was a problem hiding this comment.
Stray semicolon at the end of the comment.
| let legToFrom = leg.name.components(separatedBy: ", ") | ||
|
|
||
| if let destinationName = destinationName?.nonEmptyString, legToFrom.count > 1 { | ||
| return String.localizedStringWithFormat(NSLocalizedString("WAYPOINT_DESTINATION_VIA_WAYPOINTS_FORMAT", bundle: .mapboxNavigation, value: "%@, via %@", comment: "Format for displaying destination and intermediate waypoints; 1 = source ; 2 = destinations"), destinationName, leg.name.replacingOccurrences(of: ", ", with: " \(String.localizedStringWithFormat(NSLocalizedString("AND", bundle: .mapboxNavigation, value: "and", comment: "Format string for and;"))) ")) |
There was a problem hiding this comment.
Given that Chinese and some other languages don’t delimit words with spaces, I think we should use a format string instead of interpolating the word “and” into a hard-coded string:
let majorWays = leg.name.components(separatedBy: ", ")
let summary = String.localizedStringWithFormat(NSLocalizedString("LEG_MAJOR_WAYS_FORMAT", bundle: .mapboxNavigation, value: "%@ and %@", comment: …), majorWays[0], majorWays[1])The downside is that this code would only be able to handle two major ways, instead of saying something like “A and B and C and D”, but I think the Directions API only gives us two ways anyways.
1ec5
left a comment
There was a problem hiding this comment.
Almost there! Just one request to rename a constant and a few documentation suggestions.
| /* | ||
| Returns an` MGLStyleLayer` that determines the appearance of the circles used to symbolize the remaining waypoints. | ||
|
|
||
| If this method is unimplemented, the navigation map view draws the route remaining waypoints using an `MGLSymbolStyleLayer` whose text is equal to the layer returned from `navigationMapView(_:shapeFor:)` and will be inserted in the style below the symbol layer in navigationMapView(:waypointSymbolStyleLayerWithIdentifier:). |
There was a problem hiding this comment.
If I’m not mistaken, this method is called only once, the result doesn’t have to be a circle, and the result is applied to all the waypoints (except the origin), right? In that case, how about:
Returns an
MGLStyleLayerthat marks the location of each destination along the route when there are multiple destinations. The returned layer is added to the map below the layer returned bynavigationMapView(_:waypointSymbolStyleLayerWithIdentifier:source:).If this method is unimplemented, the navigation map view marks each destination waypoint with a circle.
| @objc optional func navigationMapView(_ mapView: NavigationMapView, simplifiedShapeDescribing route: Route) -> MGLShape? | ||
|
|
||
| /* | ||
| Returns an` MGLStyleLayer` that determines the appearance of the circles used to symbolize the remaining waypoints. |
There was a problem hiding this comment.
There’s a syntax error here caused by a space being placed after a backtick instead of before.
| Returns an` MGLStyleLayer` that determines the appearance of the text used to symbolize the remaining waypoints. | ||
|
|
||
| If this method is unimplemented, the navigation map view draws the route remaining waypoints using an `MGLSymbolStyleLayer` whose text is equal to the layer returned from `navigationMapView(_:shapeFor:)`. | ||
| This text will start at the number `1` and continue on label each consecutive waypoint. |
There was a problem hiding this comment.
Returns an
MGLStyleLayerthat places an identifying symbol on each destination along the route when there are multiple destinations. The returned layer is added to the map above the layer returned bynavigationMapView(_:waypointStyleLayerWithIdentifier:source:).If this method is unimplemented, the navigation map view labels each destination waypoint with a number, starting with 1 at the first destination, 2 at the second destination, and so on.
| @objc optional func navigationMapView(_ mapView: NavigationMapView, waypointSymbolStyleLayerWithIdentifier identifier: String, source: MGLSource) -> MGLStyleLayer? | ||
|
|
||
| /** | ||
| Returns an `MGLShape` that represents the routes waypoints |
There was a problem hiding this comment.
Typo: “route’s”, and a missing period.
There was a problem hiding this comment.
Since the first (origin) waypoint is excluded:
Returns an
MGLShapethat represents the destination waypoints along the route (that is, excluding the origin).
| /** | ||
| Returns an `MGLShape` that represents the routes waypoints | ||
|
|
||
| If this method is unimplemented, the navigation map view represnts the route waypoints using `navigationMapView(_:shapeFor:)`. |
| let arrowLayerStrokeIdentifier = "arrowStrokeLayer" | ||
| let arrowCasingSymbolLayerIdentifier = "arrowCasingSymbolLayer" | ||
| let arrowSymbolSourceIdentifier = "arrowSymbolSource" | ||
| let isCurrentLeg = "isCurrentLeg" |
There was a problem hiding this comment.
#270 (comment) got collapsed for some reason: the name isCurrentLeg implies that it’s a Boolean. Since this string is being used as an attribute name, call it currentLegAttribute but give it the value isCurrentLeg.
|
🍾🥂 |
|
When will multiple waypoints be available? |
To do:
[x] Add UI elements for arriving and leaving waypoint/cc @1ec5 @frederoni @cammace @ericrwolfe