Conversation
jill-cardamon
added
op-ex
|
|
||
| private weak var navigationService: NavigationService? | ||
| private var idleTimerCancellable: IdleTimerManager.Cancellable? | ||
| private let logger: OSLog = .init(subsystem: "com.mapbox.navigation", category: "CarPlayManager") |
There was a problem hiding this comment.
Should we add a factory for loggers? So that we don't duplicate subsystem value everywhere.
There was a problem hiding this comment.
Or we can go with a simpler way: creating an internal constant for the log subsystem.
There was a problem hiding this comment.
We could use something like OSLogManager but would probably want to consider using this manager for all of the SDK.
| try mapView.mapboxMap.style.addSource(streetsSource, id: sourceIdentifier) | ||
| } catch { | ||
| NSLog("Failed to add \(sourceIdentifier) with error: \(error.localizedDescription).") | ||
| os_log("Failed to add %s with error: %s.", log: logger, type: .error, sourceIdentifier as CVarArg, error.localizedDescription) |
There was a problem hiding this comment.
The %s will be redacted out in the logs. Should it be %{public}s?
There was a problem hiding this comment.
Ah yes. I will try this. Thank you
| try mapView.mapboxMap.style.addSource(streetsSource, id: sourceIdentifier) | ||
| } catch { | ||
| NSLog("Failed to add \(sourceIdentifier) with error: \(error.localizedDescription).") | ||
| os_log("Failed to add %s with error: %s.", log: logger, type: .error, sourceIdentifier as CVarArg, error.localizedDescription) |
There was a problem hiding this comment.
Why do we need casting to CVarArg here?
There was a problem hiding this comment.
Cast was needed since os_log uses StaticString
| import MapboxCoreNavigation | ||
| import os.log | ||
|
|
||
| private let logger: OSLog = .init(subsystem: "com.mapbox.navigation", category: "RoadNameLabeling") |
There was a problem hiding this comment.
Should we use private global constants in other classes too?
There was a problem hiding this comment.
We needed one here since you can't create stored properties in extensions. For consistency, we should either move constants to be global or use a manager for these OSLog constants
| } | ||
|
|
||
| @available(iOS 15.0, *) | ||
| func getLogEntries() throws -> [OSLogEntryLog] { |
There was a problem hiding this comment.
Is it for debugging? If so, don't forget to remove this
There was a problem hiding this comment.
This is for retrieving log entries if you want to see them printed to a file instead of looking at the log via the Console. This functionality is only available via iOS 15.
There was a problem hiding this comment.
I think there are several build errors because of OSLogEntryLog. For example getting: error: cannot find type 'OSLogEntryLog' in scope.
| interfaceController.setRootTemplate(mapTemplate, animated: false) | ||
|
|
||
| eventsManager.sendCarPlayConnectEvent() | ||
| os_log("CarInterfaceController did connect to window.", log: logger, type: .info) |
There was a problem hiding this comment.
I think that CPApplicationDelegate delegate methods are deprecated. We'll need similar logging for CPTemplateApplicationSceneDelegate as well. I think that it'd be also beneficial to log information regarding CPInterfaceController (e.g. its address, main template etc).
| import MapboxCoreNavigation | ||
| import MapboxDirections | ||
| import MapboxMaps | ||
| import os.log |
There was a problem hiding this comment.
We'll need to add logging for CPTemplate lifecycle callbacks in CarPlayManagerDelegate:
carPlayManager(_:templateWillAppear:animated:)carPlayManager(_:templateDidAppear:animated:)carPlayManager(_:templateWillDisappear:animated:)carPlayManager(_:templateDidDisappear:animated:)
| @_spi(Restricted) import MapboxMaps | ||
| import MapboxCoreNavigation | ||
| import MapboxDirections | ||
| import os.log |
There was a problem hiding this comment.
It'd be useful to have logging information when CarPlayActivity value changes.
|
|
||
| switch result { | ||
| case let .failure(error): | ||
| os_log("Failed to calculate routes.", log: logger, type: .debug) |
There was a problem hiding this comment.
This looks like an error log type.
| locationProvider.stopUpdatingHeading() | ||
| if let passiveLocationProvider = locationProvider as? PassiveLocationProvider { | ||
| passiveLocationProvider.locationManager.pauseTripSession() | ||
| os_log("Trip session paused", log: logger, type: .debug) |
There was a problem hiding this comment.
It'd be good to go through all methods in CarPlayManager and see what CarPlay related methods it uses. I think that we should log all callbacks we receive from it.
|
We should add logging for |
4 participants
This PR adds logging functionality to CarPlay to make it easier for us to debug locally.
Still needs testing on a real CarPlay head unit.
Additional ideas: