-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add StyleManagerProtocol #317
Conversation
/// - update: Closure that mutates a layer passed to it | ||
/// - Returns: Result type with `.success` if update is successful, `LayerError` otherwise | ||
@discardableResult | ||
public func updateLayer<T: Layer>(id: String, type: T.Type, update: (inout T) -> Void) -> Result<Bool, LayerError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be updated as part of a future PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
case addLayerFailed(String) | ||
|
||
/// Remove the style layer from the map failed | ||
case removeStyleLayerFailed(String?) | ||
case removeLayerFailed(String) | ||
|
||
/// Setting layer property(s) failed | ||
case setLayerPropertyFailed(String) | ||
|
||
/// The layer properties for a layer are nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole file should go away in the future right? Are we planning on wrapping the errors from core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure yet.
func layerExists(withId id: String) -> Bool | ||
|
||
/// The ordered list of the current style layers' identifiers and types | ||
var layerIdentifiers: [LayerInfo] { get } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about renaming this to allLayerIdentifiers
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be good with that. Will tackle in next PR.
A few questions but no blockers.. Thanks for breaking up the work into multiple PRs! |
PRs must be submitted under the terms of our Contributor License Agreement CLA.
Pull request checklist:
Write tests for all new functionality. If tests were not written, please explain why.Will be addressed as tail-workmapbox-maps-ios
changelog:<changelog>Refactors existing Style API functions and adds ones that were hidden.</changelog>
.Summary of changes
This is part 1 of N PRs introducing a new
StyleManagerProtocol
and related public API changes.This PR:
adds a public protocol
StyleManagerProtocol
, which is the Swifty equivalent of Android'sStyleManagerDelegate
. As part of this effort, existing APIs that were returning aResult
type, nowthrow
instead. For example,_ = style.removeStyleLayer(forLayerId: "some-id")
is replaced by
try style.removeLayer(withId: "some-id")
This also exposes a number of APIs that were previously access via the "internal"
__map
property, for exampleaddCustomLayer
.Updates related tests and examples.
Updates ResourceOptions documentation
Metrics build trigger now tries a remote branch with the same name first.