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.

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

Refine and expose TileRegionObserver API #737

Merged
merged 4 commits into from
Oct 5, 2021

Conversation

macdrevx
Copy link
Contributor

@macdrevx macdrevx commented Oct 5, 2021

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Update the changelog

Summary of changes

Breaking Changes

  • QueriedFeature.feature is no longer optional
  • Adds TypeConversionError.unsuccessfulConversion

Other Public API Changes

  • Adds TileStore.subscribe(_:) which can be used to observe a TileStore's activity. The API design deviates from Android's add/remove observer API so that we can adapt the developer-provided TileStoreObserver into a MapboxCommon_Private.TileStoreObserver without needing to use global state or something like Objective-C associated objects to keep track of which wrapper instance goes with with developer-provided observer.
  • Adds TileStoreObserver protocol

Other Internal Changes

  • Further DebugViewController cleanup
  • Feature.init(_:) now always succeeds
  • Core callback adapters have been refactored
  • Adds TileStoreObserverCancelable
  • Adds TileStoreObserverWrapper
  • Adds TileStoreProtocol and makes TileStore conform

@macdrevx macdrevx added the skip changelog Add this label if this item does not need to be included in the changelog label Oct 5, 2021
@macdrevx macdrevx force-pushed the ah/tile-store-observer-refinement branch from f89ed88 to 32af18f Compare October 5, 2021 17:39
@macdrevx macdrevx requested a review from jmkiley October 5, 2021 17:40
@macdrevx macdrevx force-pushed the ah/tile-store-observer-refinement branch from 71099da to b0614c6 Compare October 5, 2021 20:21
@macdrevx macdrevx enabled auto-merge (squash) October 5, 2021 20:22
@macdrevx macdrevx merged commit 69e9d10 into main Oct 5, 2021
@macdrevx macdrevx deleted the ah/tile-store-observer-refinement branch October 5, 2021 20:29
@macdrevx macdrevx mentioned this pull request Oct 5, 2021
XCTAssertEqual(observer.onRegionLoadFinishedStub.invocations.count, 1)
let parameters = try XCTUnwrap(observer.onRegionLoadFinishedStub.parameters.first)
XCTAssertEqual(parameters.id, id)
guard case .success(let tileRegion) = parameters.region else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m getting these errors locally in Xcode 12.4:

Swift Compiler Error Group
/path/to/mapbox-maps-ios/Tests/MapboxMapsTests/Offline/TileStoreObserverWrapperTests.swift:55:33: Definition conflicts with previous value
/path/to/mapbox-maps-ios/Tests/MapboxMapsTests/Offline/TileStoreObserverWrapperTests.swift:42:13: Previous definition of 'tileRegion' is here
Swift Compiler Warning Group
/path/to/mapbox-maps-ios/Tests/MapboxMapsTests/Offline/TileStoreObserverWrapperTests.swift:55:33: Immutable value 'tileRegion' was never used; consider replacing with '_' or removing it

XCTFail("Expected region parameter to be Result.success, but found \(parameters.region)")
return
}
XCTAssertTrue(tileRegion === tileRegion)
Copy link
Contributor

Choose a reason for hiding this comment

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

How would an NSObject ever be pointer-unequal to itself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Add this label if this item does not need to be included in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants