Navigator Tiles management#2823
Conversation
Udumft
added
op-ex
| tilesConfig: TilesConfig()) | ||
|
|
||
| self.systemLocationManager = systemLocationManager ?? NavigationLocationManager() | ||
| self.navigatorWithHistory = NavigatorProvider.sharedWeakNavigator() |
There was a problem hiding this comment.
For clarity, can we rename NavigatorProvider to Navigator, rename NavigatorProvider.sharedWeakNavigator() to a computed variable called Navigator.shared that returns an instance of Navigator, and fold NavigatorWithHistory’s members into Navigator? Basically, Navigator would be a public SDK wrapper for the Navigator (and HistoryRecorder) provided by MapboxNavigationNative. I don’t think we’d need a separate factory class to vend the singleton.
(By the way, this Navigator wrapper idea was why we originally created the Router protocol. But that protocol would need some cleanup to be relevant here.)
There was a problem hiding this comment.
@1ec5, Navigator shared instance logic is now part of Navigator.swift extension. Navigator.shared, Navigator.tilesVersion, Navigator.tilesURL, Navigator.enableHistoryRecorder(), Navigator.disableHistoryRecorder(), Navigator.history() is exposed to outer world. Please let me know what you think.
| /** | ||
| Creates a cache for tiles of the given version and configures the navigator to use this cache. | ||
| */ | ||
| func configureNavigator(withTilesVersion tilesVersion: String) throws { |
There was a problem hiding this comment.
The original idea was to defer the navigator’s creation and configuration until the map SDK starts consuming location updates from PassiveLocationManager. Now that configuration happens earlier when attaching the PassiveLocationManager to the MapView. The nice thing about waiting until startUpdatingLocation() is that we could make it completely asynchronous without having to synchronize within a factory method.
|
@1ec5, can you please give it another pass. After adding proposed changes passive/active navigation seems to be working fine, but it'd be good to add some unit-tests (I think this is currently blocked). |
| Provides a new or an existing one `Navigator` instance along with related `HistoryRecorderHandle`, | ||
| satisfying provided configuration (`tilesVersion` and `tilesURL`). | ||
| */ | ||
| private static let navigatorWithHistoryRecorder: (Navigator, HistoryRecorderHandle) = { |
There was a problem hiding this comment.
I hope that tuple usage is fine in this case. Not sure it's worth creating another entity for this similar to NavigatorWithHistory.
There was a problem hiding this comment.
Tuple is no longer relevant after adding CoreNavigationNavigator.swift (btw feel free to propose better naming if you have any ideas). Shall I remove NavigatorResources in scope of this PR? As I can see it's not used anywhere at the moment.
| return (navigator, historyRecorder) | ||
| }() | ||
|
|
||
| func status(at timestamp: Date) -> NavigationStatus { |
There was a problem hiding this comment.
I've removed navigatorIsActive() as it wasn't used anywhere.
…ller, PassiveLocationDataSource to use it
MaximAlien
force-pushed
the
vk/430-tile-manager
branch
from
95e1936 to
a7e26cf
Compare
| /** | ||
| Shared instance on `Navigator`. | ||
| */ | ||
| static let shared: Navigator = { | ||
| return navigatorWithHistoryRecorder.0 | ||
| }() |
There was a problem hiding this comment.
I should’ve been clearer in #2823 (comment): the suggestion was to use composition/encapsulation rather than an extension. MapboxCoreNavigation.Navigator would be a singleton containing a MapboxNavigationNative.Navigator. This way the raw methods defined by MapboxNavigationNative won’t be exposed to the developer. It’ll also be possible to encapsulate other resources at the same time, such as HistoryRecorderHandle – obviating both the NavigationResources struct from 5ac8c9a and the (Navigator, HistoryRecorderHandle) tuple defined in this PR.
There was a problem hiding this comment.
Got it. Please check latest update, CoreNavigationNavigator.swift now encapsulates both HistoryRecorderHandle and MapboxNavigationNative.Navigator. Access point is possible via Navigator.shared static computed property.
…Handle and Navigator.
5 participants
This PR solves
Navigatorinstance management by introducing single entry point and sharing existing instances. It also ensures that all Navigators are properly configured when initialized.