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.

Sign up for GitHub

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

Jump to bottom

Offline Directions errors-clarification #2374

Merged
merged 4 commits into from
Jul 16, 2020
Merged

Offline Directions errors-clarification #2374

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f32b0d
2273-errors-clarification: added new error case for indicating incorr…
Udumft May 27, 2020
0b4a128
2273-errors-clarification: removed sending error in case Directions i…
Udumft Jun 1, 2020
7a83226
2273-errors-clarification: removed unused error description
Udumft Jun 1, 2020
62d4ee3
2273-errors-clarification: comment formatting
Udumft Jun 11, 2020
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
13 changes: 1 addition & 12 deletions MapboxCoreNavigation/OfflineDirections.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@ public enum OfflineRoutingError: LocalizedError {
A Directions API error can occur whether directions are calculated online or offline.
*/
case standard(DirectionsError)

/**
The router returned an empty response.
*/
case noData

/**
The router returned a response that isn’t correctly formatted.
*/
Expand All @@ -34,8 +28,6 @@ public enum OfflineRoutingError: LocalizedError {
switch self {
case .standard(let error):
return error.localizedDescription
case .noData:
return NSLocalizedString("OFFLINE_NO_RESULT", bundle: .mapboxCoreNavigation, value: "Unable to calculate the requested route while offline.", comment: "Error description when an offline route request returns no result")
case .invalidResponse:
return NSLocalizedString("OFFLINE_CORRUPT_DATA", bundle: .mapboxCoreNavigation, value: "Found an invalid route while offline.", comment: "Error message when an offline route request returns a response that can’t be deserialized")
case .unknown(let underlying):
Expand Down Expand Up @@ -174,7 +166,7 @@ public class NavigationDirections: Directions {

- parameter options: A `RouteOptions` object specifying the requirements for the resulting routes.
- parameter offline: Determines whether to calculate the route offline or online.
- parameter completionHandler: The closure (block) to call with the resulting routes. This closure is executed on the application’s main thread.
- parameter completionHandler: The closure (block) to call with the resulting routes. This closure is executed on the application’s main thread. If called `NavigationDirections` instance is deallocated before route calculation is finished - completion won't be called.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not familiar with context of this problem, so just minor comment from me: probably better to limit length of lines to around 100-120, just for better readability.

*/
public func calculate(_ options: RouteOptions, offline: Bool = true, completionHandler: @escaping OfflineRouteCompletionHandler) {

Expand All @@ -196,9 +188,6 @@ public class NavigationDirections: Directions {

NavigationDirectionsConstants.offlineSerialQueue.async { [weak self] in
guard let result = self?.navigator.getRouteForDirectionsUri(url.absoluteString) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Per #2273 (comment), the only error case we should handle up here would be that self (NavigationDirections) has been deallocated.

Upon closer inspection, I’m wondering if it’s even appropriate to call the completion handler in this case. For mapbox/mapbox-gl-native-ios#210 mapbox/mapbox-gl-native-ios#200 (comment), we found that calling the completion handler after self has gone away forces the caller to consider whether self has gone away because the parent view controller has gone away or the application is terminating.

In other words, when an application calls this method from a MapViewController that holds a strong reference to NavigationDirections, does it expect the completion handler to magically be called even if that MapViewController has been deallocated (for example, because the user has exited that map screen to go to a different part of the application)? If the application has neglected to make self weak inside the completion handler, that could create other unexpected behavior in the application. The completion handler would have to explicitly handle the .cancelled case, so at least we would have to document that postcondition on the method, which is unfortunate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not calling a completion under some conditions may also lead to unexpected behavior. At least such use case must also be documented so that users are aware that completion call is not guaranteed.
Also, if we choose not to rise an explicit error message when NavigationDirections were deallocated, noData error becomes unused, so we can safely remove it

DispatchQueue.main.async {
completionHandler(session, .failure(.noData))
}
return
}

Expand Down
3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/Base.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Found an invalid route while offline.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Unable to calculate the requested route while offline.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Mapbox Navigation SDK for iOS version %@ is now available.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/de.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Es wurde eine ungültige Route berechnet, da keine Verbindung besteht.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Es ist nicht möglich, die gewünschte Route zu berechnen, da keine Verbindung besteht.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Mapbox Navigation SDK für iOS Version %@ ist jetzt verfügbar.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/es.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Ha encontrado una ruta no válida en modo offline.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "No se puede calcular la ruta peticionada en modo offline.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "La versión %@ de Mapbox Navigation SDK para iOS está disponible.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/fr.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Impossible de charger cet itinéraire hors ligne";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Impossible de calculer cet itinéraire hors ligne";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "La version %@ du SDK iOS Mapbox est maintenant disponible";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/ja.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "オフラインの為ルートを作成できません";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "オフラインの為、ルートを作成できません";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "iOS %@用 Mapbox Navigation SDKが使用できます";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/pt-PT.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Foi encontrada uma rota inválida desligado da internet.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Não é possível calcular a rota pedida desligado da internet.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "O SDK de Navegação Mapbox para iOS versão %@ já está disponível.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/ru.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Получен неверный маршрут без интернета.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Требуемый маршрут невозможно рассчитать без интернета.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Доступна версия Mapbox Navigation SDK %@ для iOS.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/sv.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Ogiltligt offline-rutt.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Kunde inte beräkna rutten i offline-läge.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Mapbox Navigation SDK for iOS version %@ är nu tillgängligt.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/vi.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Đã tìm thấy tuyến đường không hợp lệ trong khi ngoại tuyến.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Không tìm thấy tuyến đường trong khi ngoại tuyến.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Mapbox Navigation SDK cho iOS mới ra phiên bản %@.";

3 changes: 0 additions & 3 deletions MapboxCoreNavigation/Resources/yo.lproj/Localizable.strings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
/* Error message when an offline route request returns a response that can’t be deserialized */
"OFFLINE_CORRUPT_DATA" = "Found an invalid route while offline.";

/* Error description when an offline route request returns no result */
"OFFLINE_NO_RESULT" = "Unable to calculate the requested route while offline.";

/* Inform developer an update is available */
"UPDATE_AVAILABLE" = "Mapbox Navigation SDK for iOS version %@ is now available.";