TileStore config for Navigator and MapView

[Udumft]

Description

Resolves #2926
Adds setting to control Navigator and MapView tile store location.

Implementation

Navigator TileStore can be configured only before it is initialised. There are 2 entry points which instantiate a Navigator instance and it is never deallocated (for now): FreeDrive (aka PassiveLocationDataSource and Active Guidance (aka RouteController). Navigator always has to have a tile storage somewhere. MapView is handled by NavigationMapViewwhich can be used separately or as a part ofNavigationViewController`. MapView can have no on-device tile storage at all.

[Udumft]

I am concerned about this thing. From one perspective, since Map and Navigator have slightly different use-cases (Map may have no TileStore) using URL? as a setting does not cover all possible cases. In this case nil for Nav == default, while nil for map == no tile store.
Introducing this enum construct allows fine control and convenience usage like:

options.tileStoreLocation = .custom(url) // to pass user-provided location, or
options.tileStoreLocation = .isolated(.default, noStorage) // to disable Map Tile storage while keeping Nav at default

On the other hand - this setting is not reusable while manipulating bare NavigationView or NavigationService. It is also very sensitive to initialisation order. If User wants to create his custom NavigationService to pass it in NavigationOptions - he has to also take care of configuring a tile storage parameter.

[Udumft]

And yes, naming is an open question

[1ec5]

An enumeration with associated values seems reasonable to me.

this setting is not reusable while manipulating bare NavigationView or NavigationService

Not sure I follow – we could modify those classes to accept a TileStoreLocation too, right?

[Udumft]

Not sure I follow – we could modify those classes to accept a TileStoreLocation too, right?

In this case we should frame it outside NavigationOptions to avoid uglyNavigationService.init definition, and it makes TileStoreLocation a standalone type which I'd like to avoid. Also, in terms of bare NavigationMapView plain TileStore? is a more robust way for picking storage location.

[Udumft]

Had to store it here to be able to reach it during loadView and configure NavigationView. Tile storage is configured at initialisation time for MapView which forces piping this as init parameter through NavigationView and NavigationMapView. Pretty much the same happens for Navigator.
Apart of ravaging the init API, it also makes tile store setup very sensitive to init order. If any component is created before NavigationViewController.init - it will ignore NavigationOptions setup.

[1ec5]

If any component is created before NavigationViewController.init - it will ignore NavigationOptions setup.

This suggests that maybe those components should also accept a NavigationOptions in their initializers, or that the tile store should be independent of NavigationOptions.

[Udumft]

that the tile store should be independent of NavigationOptions

Not sure to follow this part. Did you mean to extract TileStore configuration from NavigationOptions? But that ruins concept of NavigationOptions as single customization source.
Having components accept NavigationOptions seems and overkill since it just needs a TileStore bit. But it brings us back to having TileStoreLocation as a standalone type suitable for configuring components. I guess I'll try this case

[1ec5]

An enumeration with associated values seems reasonable to me.

this setting is not reusable while manipulating bare NavigationView or NavigationService

Not sure I follow – we could modify those classes to accept a TileStoreLocation too, right?

[1ec5]

This case would be clearer with labels for each of the associated values.

[1ec5]

There are a lot of arguments with default values. If the default is sensible in most cases, we could omit this argument from the initializer and expect developers to set the tileStoreLocation property on a separate line.

This goes for the rest of the arguments too. I’m not sure why we really need a convenience initializer, since there’s no inherent order to the arguments other than the order in which we implemented each feature. v2.0 would be a good opportunity to remove this initializer, though maybe in a separate PR.

[1ec5]

If any component is created before NavigationViewController.init - it will ignore NavigationOptions setup.

This suggests that maybe those components should also accept a NavigationOptions in their initializers, or that the tile store should be independent of NavigationOptions.

[Udumft]

I've updated the code to utilise TileStoreLocation in all components where TileStore setting is required. Hopefully it will be clearer for users what kind of setting is required and help avoid initialisation order issues.

if such solution seems reasonable, I'll go on and finalize it.

[azarovalex]

small typo here

[azarovalex]

Those enums seem too sophisticated IMHO, especially the fact that Strict.tileStoreURL can be nil.
I thought that Strict enum is used to explicitly show that the url is always present.
I wonder if there is a way to simplify code here.

[bamx23]

I have an idea for this class:

public struct TileStoreConfiguration {
    enum Location: RawRepresentable {
        case `default`
        case custom(URL)

        init?(rawValue: URL?) {
            self = rawValue.map { .custom($0) } ?? .default
        }

        var rawValue: URL? {
            switch self {
            case .default:
                return nil
            case .custom(let url):
                return url
            }
        }

        var tileStore: TileStore {
            switch self {
            case .default:
                return TileStore.getInstance()
            case .custom(let url):
                return TileStore.getInstanceForPath(url.path)
            }
        }
    }

    var navigatorLocation: Location
    var mapLocation: Location?

    static var `default`: Self {
        .init(navigatorLocation: .default, mapLocation: .default)
    }
    static func custom(_ url: URL) -> Self {
        .init(navigatorLocation: .custom(url), mapLocation: .custom(url))
    }
    static func isolated(navigationLocation: Location, mapLocation: Location?) -> Self {
        .init(navigatorLocation: navigationLocation, mapLocation: mapLocation)
    }
}

[Udumft]

I like this! I think I'll try this in next iteration

[bamx23]

I'm not sure if Location enum needs RawRepresentable, but it looks cooler :) But also this makes some ambiguity for let x = Location(rawValue: nil) vs let x: Location? = nil.

[bamx23]

Is it necessary to have it optional? The only place where it is used has a fallback to .default in case it's nil. Maybe it's better to leave it non-optional and if the user doesn't want to provide this they just use .default.

[Udumft]

Yes, it should have an option to be nil. Map may have no local tile store. So to distinguish default and no tilestore we need it optional.
But you are also right about the only place it is used and is defaulted - that's a bug :)

[bamx23]

PredictiveCacheManager can also initialize a Navigator instance.

And also I see some work with tile store in NavigationMapView.enablePredictiveCaching. Oh, nevermind, it's taken into consideration in the NavigationMapView constructor.

[Udumft]

Good catch! Even though NavigationMapView.enablePredictiveCaching is covered by constructor, PredictiveCacheManager is publicly available and thus can potentially be called before any other setup is done.

[bamx23]

It worth thinking about how to avoid such potential problems in the future. Maybe it can be solved by extracting Navigator pre-initialization config into a separate struct to make a single entry point that at least easier to validate in Xcode by inspecting "show all callers" or something like that.

[Udumft]

I thought about that, but I see there are few issues: either we force this struct to be configured before any interaction w/ SDK (technically we introduce an SDK setup step) and trust user to do so at proper moment, or we encapsulate this setup anywhere where it may be triggered which makes the process transparent for users, but requires us to maintain all such places. What we are doing currently looks more like a latter case.

[bamx23]

I understand why you've done it this way. What I'm talking about is to make future internal changes safer, to reduce the chance of forgetting to put property initiation into some of the places where a Navigator instance can be created.

[Udumft]

I think we should better frame it outside current PR, because this seems to me like a complex solution, covering more than tile store configuration.

[bamx23]

And, by the way, we have ...Options semantic here. Maybe it's better to keep this in this PR and rename TileStoreConfiguration into TileStoreOptions?

[Udumft]

Unfortunately it will cause naming conflict with Common's MBXTileStoreOptions. I would like to avoid namespace usage.

[1ec5]

I agree that “options” would be a more consistent naming choice. Internal type conflicts ideally shouldn’t influence what we expose publicly to developers, but this would be an opportunity to prefer the more grammatical “TileStorage” terminology over “TileStore” (which incidentally sounds like a kind of shop).

[Udumft]

I think I've covered all entry points to Navigator initialization now. Indeed, that might become complicated to add more settings or maintain them. We might consider introducing some kind of a setup mechanism for such things to ease the support and reduce potential risk of human error.

[1ec5]

I think there remain some opportunities for making the API more ergonomic for developers, but they can be handled as tail work. The overall functionality is in good shape.

[1ec5]

Suggested change

* Introduced `TileStoreConfiguration` struct for setting up tile storage locations for Navigation and Map tiles. You can customise it via `NavigationOptions.tileStoreConfiguration`, `PassiveLocationDatasource.init(directions: Directions, systemLocationManager:, tileStoreLocation:)`, `NavigationMapView.init(frame:, navigationCameraType:, tileStoreLocation:)`, `PredictiveCacheManager.init(predictiveCacheOptions:, tileStoreMapOptions:)` or others, accepting `TileStoreConfiguration`. ([#2956](https://github.com/mapbox/mapbox-navigation-ios/pull/2956))

* Added the `NavigationOptions.tileStoreConfiguration` property and arguments to `PassiveLocationDataSource(directions:systemLocationManager:tileStoreLocation:)`, `NavigationMapView(frame:navigationCameraType:tileStoreLocation:)`, and `PredictiveCacheManager(predictiveCacheOptions:tileStoreMapOptions:)` for customizing the locations where navigation and map tiles are stored. ([#2956](https://github.com/mapbox/mapbox-navigation-ios/pull/2956))

[1ec5]

Is TileStore a class that the developer would work with directly? If not, use a more generic term for it:

Options for configuring how map and navigation tiles are stored on the device.

[Udumft]

It is. This class is exposed to user and is a main point for Offline Regions feature.

[1ec5]

Similar to mapbox/mapbox-maps-ios#252, I think it would be more ergonomic for Location to be a raw-representable struct with a single default static constant, or we could even reuse the URL type, treating nil as the default value and extending the type for properties such as tileStoreURL.

[Udumft]

Using URL type is problematic because we cannot exactly say that nil == default value. Map and Navigator treat nil differently: Navigator will switch to a default location while Map may have no tile storage at all.
IMHO in this case an enum with associated value is more descriptive than a struct with static constant.

[1ec5]

I agree that “options” would be a more consistent naming choice. Internal type conflicts ideally shouldn’t influence what we expose publicly to developers, but this would be an opportunity to prefer the more grammatical “TileStorage” terminology over “TileStore” (which incidentally sounds like a kind of shop).