Feat/mapviewcamera ops update #102
Conversation
| /// - newPitch: The new pitch value. | ||
| /// - proxy: An optional map view proxy, this allows the camera to convert from .rect/.showcase to centered. | ||
| /// Allowing zoom from the user's current viewport. | ||
| mutating func pan(_ direction: CPMapTemplate.PanDirection, proxy: MapViewProxy? = nil) { |
There was a problem hiding this comment.
We could create our own direction enum here, but I figured CarPlay is always available to import. Thoughts?
|
|
||
| // Should convert to centered camera using defaults | ||
| if case let .centered(onCoordinate: coordinate, zoom: zoom, _, _, _) = camera.state { | ||
| #expect(coordinate.latitude.isApproximatelyEqual( |
There was a problem hiding this comment.
lol. Apparently this is the recommended way to do approximate float comparisons (using the numerics library) :P
| public var centerCoordinate: CLLocationCoordinate2D { | ||
| mapView.centerCoordinate | ||
| } | ||
| public let centerCoordinate: CLLocationCoordinate2D |
There was a problem hiding this comment.
So, a few discussion points here (that we want input from @hactar on). We changed this to improve testability (constructing this required a full MapLibre map object, which isn't really possible to configure for testing properly), and @Archdoog thinks that current usage patterns would not be holding on to the proxy long term. The proxy is recreated every time the region changes internally.
However, as written it's technically possible that some attribute of the map view change underneath, and the proxy is then out of sync. This is not great. But the usage pattern is that you keep getting proxies via the onMapViewProxyUpdate modifier.
So there seems to be an argument for making this an immutable struct and making it clear that this is not guaranteed to be valid after onMapViewProxyUpdate completes. It might also be worth changing the name if this is how we land (i.e. MapViewState or something... idk naming is hard).
As an aside, we can also make the initializers internal.
There was a problem hiding this comment.
Looking at https://developer.apple.com/documentation/swiftui/geometryproxy, we might consider actually building a custom private/internal struct that can be used instead of the mapView on the proxy. Its job would be to hold the data required to calculate the frame to lat-long and other similar functionality. I think would let us snapshot the advanced functionality without holding a reference to the whole MapView. Bonus, it'd also further enhance testability.
There was a problem hiding this comment.
So lets start with the easy part: I tested my bigger app against the branch and after some adjustments found no regressions, so all is good here.
But that said, I'm not sure how ideal this new solution is:
The proxy currently only covers a small part (the stuff I needed) of what might need to be exposed via it to get feature parity with MapLibre. The previous solution lazily "set" the properties - only the properties the dev actually uses were read out of MapLibre. Now, all properties have to be read out for every update of the proxy. Currently it is 7, but it could become a lot more if we go for MapLibre feature parity. Does this have a measurable performance impact, especially when ProxyUpdateMode is set to .realtime? If it doesn't yet, will it when the proxy has 20 values, or 40? I don't know - but the current lazy get solution would definitely not suffer here.
Secondly, now that the mapView is optional, all functions that use it have to have optional return values, making the proxy more difficult to use in code: everything needs to be unwrapped. In non test usage, you will always have an MLNMapView - making usage on call site worse just for testing isn't ideal.
If I see it correctly you don't want to test the proxy itself, but need to provide a proxy for other camera related tests, without having a map system in place. Wouldn't a better alternative be to create a MapViewProxyProtocol, then keep the MapViewProxy as it currently is, but also create a MockMapViewProxy that can be initialized with values as needed? The MockMapViewProxy wouldn't have to have an private let mapView: MLNMapView in it at all while it would still be a requirement for the real MapViewProxy?
There was a problem hiding this comment.
Looking at https://developer.apple.com/documentation/swiftui/geometryproxy, we might consider actually building a custom private/internal struct that can be used instead of the mapView on the proxy. Its job would be to hold the data required to calculate the frame to lat-long and other similar functionality. I think would let us snapshot the advanced functionality without holding a reference to the whole MapView. Bonus, it'd also further enhance testability.
I'm all for not having a reference to MLNMapView if possible - but how would you go about solving the second use case of MapViewProxy: calling into functions like convert that MLNMapView provides?
There was a problem hiding this comment.
This was a good discussion to do some more research on. Outcome was:
- Since the MLNMapView already exists as a ref type, there's little cost to attaching it to the MapViewProxy. This is basically what you said @hactar and given the potential to extend further map view proxy features makes sense.
- We already solved this problem for the camera. I've used this concept and renamed the camera's map view protocol to
MLNMapViewRepresentable. This is essentially the protocol that exposes any MLNMapView param or function we want to inject and test on an object.
This has been fixed and now we've got the original MapViewProxy but with my test suite.
|
It looks like I no longer have permissions to merge PRs on this repo after the 2FA mess 🤔 @birkskyum can you double check on the repo permissions and make sure that @Archdoog, @hactar and I all have maintainer-level access? |
|
@ianthetechie i double-checked , all three of you do presently have admin access on this repo |
|
Thanks @birkskyum; It looks like the issue was that I forgot to accept the invitation 😅 |
Issue/Motivation
setDirectionfor MapViewCamerapanMapViewCamera. For use with CarPlay directions.MapViewProxyto camera operations to allow changing camera from a 'stuck' rect and showcase camera.MapViewProxyto enable testing. @hactarTasklist
If there are any visual changes as a result, include before/after screenshots and/or videos