Introduce ResourceOptionsManager #396

julianrex · 2021-05-26T20:11:45Z

PRs must be submitted under the terms of our Contributor License Agreement CLA.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

Pull request checklist:

Briefly describe the changes in this PR.
~~Write tests for all new functionality. If tests were not written, please explain why.~~ Updated
Document any changes to public APIs.
Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
~~Add an entry inside this element for inclusion in the mapbox-maps-ios changelog: <changelog></changelog>.~~ Manual edit.
Update the migration guide, API Docs, Markdown files - Readme, Developing, etc

Summary of changes

This PR replaces CredentialsManager with ResourceOptionsManager creating a struct based ResourceOptions that shadows the type from MapboxCoreMaps. The bulk of the other changes are related to this except for the following:

Update the cache path to live in .mapbox. This aligns with other frameworks.
CI creates the MapboxAccessToken file - since it appears that the build pre-action script is not being run
Updated min/max latitude projection test
Re-enabled testCacheManagerIsReleasedAndHandlerRetainingManagerIsCalled
Fix MapView retain issue with integration tests
Fix ResourceEvent and associated test

julianrex · 2021-05-27T13:25:52Z

Sources/MapboxMaps/Foundation/Extensions/Core/ResourceOptions.swift

+
+    /// The access token to access the resource. This should be a valid, non-empty,
+    /// Mapbox access token
+    public var accessToken: String


I'd like us to consider making access token a custom type, for example an enum. Not unlike an optional, but instead of a .none case, we could have a .search or .default case that would look for the token in the Info.plist etc. This way it's more explicit and we could deprecate it later if we ever choose to.

I like this idea.. may be we could even make it expressibleByString to get the best of both worlds?

cc @Chaoba
on Android we can not overwrite the type in the ResourceOptions..

julianrex · 2021-05-27T13:26:45Z

Sources/MapboxMaps/Foundation/Extensions/Core/ResourceOptions.swift

+}
+
+extension ResourceOptions: CustomStringConvertible, CustomDebugStringConvertible {
+    private func redactedAccessToken() -> String {


If we do add a custom AccessToken type, then this description logic would move there too.

julianrex · 2021-05-27T13:28:12Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+    internal static func destroyDefault() {
+        defaultInstance = nil
+    }


This is in preparation of being able to clean up TileStores completely - useful for tests.

we are considering to expose the same cc @Chaoba

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

julianrex · 2021-05-27T13:29:13Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+    ///     ```
+    ///
+    /// - Parameter accessToken: Valid access token or `nil`
+    public convenience init(accessToken: String? = nil) {


Also added a convenience that just takes an access token.

we are aligned here.

julianrex · 2021-05-27T13:30:03Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+    ///
+    /// - Parameter accessToken: Valid access token or `nil`
+    internal func reset(resourceOptions: ResourceOptions? = nil) {
+        self.resourceOptions = resourceOptions ?? ResourceOptions(accessToken: defaultAccessToken())


The default access token logic should probably move to where the core ResourceOptions object is initialized from the struct.

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

julianrex · 2021-05-27T13:32:05Z

Tests/MapboxMapsTests/Foundation/Projection/ProjectionTests.swift

+        // Check clamping
+        let northMax = CLLocationCoordinate2D(latitude: Projection.latitudeMax, longitude: 0)
+        let northMaxMercator = projectorType.project(northMax, zoomScale: zoomScale)
+        XCTAssertEqual(northMercator.y, northMaxMercator.y, accuracy: 0.000001)
+
+        let southMin = CLLocationCoordinate2D(latitude: Projection.latitudeMin, longitude: 0)
+        let southMinMercator = projectorType.project(southMin, zoomScale: zoomScale)
+        XCTAssertEqual(southMercator.y, southMinMercator.y, accuracy: 0.000001)


Unrelated test - will likely leave in this PR for simplicity

julianrex · 2021-05-27T13:32:43Z

Tests/MapboxMapsTests/MapView/Integration Tests/Map/DidIdleFailureIntegrationTest.swift

@@ -33,7 +34,7 @@ internal struct ResourceEventResponse: Decodable {
    var noContent: Bool
    var notModified: Bool
    var mustRevalidate: Bool
-    var offlineData: Bool
+    var source: String


Unrelated test fix.

Sources/MapboxMaps/Foundation/Extensions/Core/CacheManager.swift

pengdev · 2021-05-27T15:22:48Z

Sources/MapboxMaps/Foundation/Extensions/Core/MapSnapshotOptions.swift

-                            glyphsRasterizationOptions: GlyphsRasterizationOptions? = GlyphsRasterizationOptions(fontFamilies: []),
-                            resourceOptions: ResourceOptions = ResourceOptions()) {
+                            glyphsRasterizationOptions: GlyphsRasterizationOptions? = GlyphsRasterizationOptions(),
+                            resourceOptions: ResourceOptions = ResourceOptionsManager.default.resourceOptions) {


MapSnapshotOptions is a gl-native generated class in Android, and we couldn't override the default value here...

Sources/MapboxMaps/Foundation/Extensions/Core/ResourceOptions.swift

Sources/MapboxMaps/Foundation/MapView.swift

pengdev · 2021-05-27T15:36:50Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+/// Convenience class that manages a global `ResourceOptions`
+///
+/// It's possible to create `ResourceOptionsManager` instances as you need them,
+/// however it's convenient to use the default object (`default`).
+///
+/// For example, we recommend that the Mapbox access token be set in
+/// `application(_:didFinishLaunchingWithOptions:)` rather than relying on the
+/// value in your application's Info.plist:
+///
+///     ```
+///     func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
+///         // Override point for customization after application launch.
+///         ResourceOptionsManager.default.resourceOptions.accessToken = "overridden-access-token"
+///         return true
+///     }
+///     ```


@Chaoba let's also align on the documentation

pengdev · 2021-05-27T15:37:44Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+    /// A valid access token must be provided or found.
+    public static var `default`: ResourceOptionsManager {
+        if defaultInstance == nil {
+            defaultInstance = ResourceOptionsManager(accessToken: nil)


on iOS, do you throw exceptions when the accessToken is null?

Not at the moment, does Android? Also, nil in this context means "search for the token".

pengdev · 2021-05-27T15:43:51Z

Sources/MapboxMaps/Foundation/ResourceOptionsManager.swift

+        if token.isEmpty {
+            Log.warning(forMessage: "Empty access token.", category: "ResourceOptions")
+        }


So you don't throw exceptions when the token is not found, and instead just show a warning message? What does it look like to users? will the MapView will show a black screen?

fyi, if the access token is not found anywhere, we will throw a runtime exception that crashes the app..

cc @Chaoba

We could do the same - though there's some trickiness around testing.

[run device tests]

Co-authored-by: Peng Liu <peng.liu@mapbox.com>

julianrex requested review from nishant-karajgikar, neelmistry94 and pengdev May 27, 2021 13:15

julianrex added the breaking change ⚠️ If your pull request introduces a breaking change and updates are required when version is published label May 27, 2021

julianrex changed the title ~~ResourceOptionsManager~~ Introduce ResourceOptionsManager May 27, 2021

julianrex commented May 27, 2021

View reviewed changes

neelmistry94 reviewed May 27, 2021

View reviewed changes

Sources/MapboxMaps/Foundation/Extensions/Core/CacheManager.swift Outdated Show resolved Hide resolved

nishant-karajgikar approved these changes May 27, 2021

View reviewed changes

pengdev reviewed May 27, 2021

View reviewed changes

Sources/MapboxMaps/Foundation/Extensions/Core/ResourceOptions.swift Show resolved Hide resolved

pengdev reviewed May 27, 2021

View reviewed changes

Sources/MapboxMaps/Foundation/MapView.swift Outdated Show resolved Hide resolved

pengdev reviewed May 27, 2021

View reviewed changes

pengdev requested a review from Chaoba May 27, 2021 15:56

julianrex and others added 13 commits May 27, 2021 19:15

Replace CredentialsManager with ResourceOptionsManager

f90f283

[run device tests] [run app device tests]

3597a6c

Switch to access token parameter

a841cb6

Remove update function

ab21c96

[run device tests]

Update comments. Adds . to mapbox directory

3e7d0fa

Use MapboxMaps.xcodeproj

1ca646a

Remove previous tweak. Fix tear down for offline manager test.

0e1c102

[run device tests]

Temporary test of pre-action script

5de71ed

Update pre-action script. CI creates MapboxAccessToken file.

c879d45

Add guard for metal device.

69a6dff

Update Sources/MapboxMaps/Foundation/MapView.swift

347b222

Co-authored-by: Peng Liu <peng.liu@mapbox.com>

Add metal device guards to other tests.

10bb065

Updated change log

4d54035

julianrex force-pushed the jrex/resource-manager branch from ce2d94f to 4d54035 Compare May 27, 2021 23:16

Compile fix post rebase

8cbfc51

[run device tests] [run app device tests]

de86218

julianrex marked this pull request as ready for review May 28, 2021 00:02

julianrex merged commit 31241bf into main May 28, 2021

julianrex deleted the jrex/resource-manager branch May 28, 2021 00:02

julianrex mentioned this pull request May 28, 2021

Fix aggressive teardowns #404

Merged

3 tasks

evil159 mentioned this pull request May 24, 2022

Feature/add puck mercator scale #1347

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce ResourceOptionsManager #396

Introduce ResourceOptionsManager #396

julianrex commented May 26, 2021 •

edited

Loading

julianrex May 27, 2021

nishant-karajgikar May 27, 2021 •

edited

Loading

pengdev May 27, 2021

julianrex May 27, 2021

julianrex May 27, 2021

pengdev May 27, 2021

julianrex May 27, 2021

pengdev May 27, 2021

julianrex May 27, 2021

julianrex May 27, 2021

julianrex May 27, 2021

pengdev May 27, 2021

pengdev May 27, 2021

pengdev May 27, 2021

julianrex May 27, 2021

pengdev May 27, 2021

pengdev May 27, 2021

julianrex May 27, 2021

Introduce ResourceOptionsManager #396

Introduce ResourceOptionsManager #396

Conversation

julianrex commented May 26, 2021 • edited Loading

Pull request checklist:

Summary of changes

Choose a reason for hiding this comment

nishant-karajgikar May 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex commented May 26, 2021 •

edited

Loading

nishant-karajgikar May 27, 2021 •

edited

Loading