Fix building highlighting when changing style. #2550
Conversation
MaximAlien
added
bug
There was a problem hiding this comment.
In my opinion, the operations that take place inside the style observation in this PR should take place in whatever class uses NavigationMapView, be it RouteMapViewController or something custom. As it is, I don’t think it’s a huge burden on a custom view controller to call show(_:), showWaypoints(on:), and highlightBuildings(at:in3D:).
I would readily agree that it’s unfortunate that runtime styling customizations get blown away when changing the style, but that’s a broader map SDK limitation that would be better addressed by MGLMapView automatically reapplying runtime styling changes, as in mapbox/mapbox-gl-native#6180.
| @@ -266,6 +276,20 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate { | |||
| addGestureRecognizer(mapTapGesture) | |||
|
|
|||
| installUserCourseView() | |||
|
|
|||
| styleObservation = observe(\.style, options: .new) { [weak self] (mapView, change) in | |||
There was a problem hiding this comment.
Does observing a key path on self cause the map view to retain itself?
There was a problem hiding this comment.
I don't think so. As per docs here Neither the object receiving this message, nor observer, are retained.
| if let routes = self.routes, let currentRoute = routes.first { | ||
| self.show(routes) | ||
| self.showWaypoints(on: currentRoute) | ||
| } |
There was a problem hiding this comment.
It would be better to gradually reduce the usage of NavigationMapView.routes. We’ve successfully removed NavigationMapView’s listening for route progress change. In the same vein, we should aim to remove extra state that the map view keeps around that could easily get out of sync with RouteController.
There was a problem hiding this comment.
Not sure that in such case there is an easy way to prevent using routes variable. Since show(_:legIndex:) is public method and NavigationMapView is also independent class there should be a way to persist current routes (to allow routes selection on tap, etc).
I agree that current fix solves broader maps problem best fix for which would be in Maps SDK. On the other hand current behavior brings not really good UX, since those who're just getting familiar with the Navigation SDK might encounter it without any prior knowledge of why annotations are staying on map view whereas routes and waypoints do not. Since there was not that much movement in mapbox/mapbox-gl-native#6180 for the past few years It'd probably be better to keep current fix and add FIXME note which points to ticket in |
…NavigationMapView when style was changed.
MaximAlien
force-pushed
the
maxim/2545-fix-building-highlight-when-changing-style
branch
from
5d4b6bf to
8e624d7
Compare
On the one hand, I see how it’s unintuitive that changing the style clears out runtime styling changes, but on the other hand, we can only solve so much of that issue. The developer is free to apply their own runtime styling changes, so the navigation SDK can only reapply part of the runtime styling changes for them. We’d do so clumsily by enforcing a z-order that the developer might not want. I think I’d still prefer that we move the style observation to RouteMapViewController or NavigationViewController, where it’s already a normal thing to observe the style. Then we could add something to the documentation for /ref #2545 (comment) |
|
On the other other hand, changing the style currently seems to get |
IodaMikeMapbox
added
the
needs discussion
|
Current PR is no longer relevant. |
Closing #2545.