Open michaelkirk opened 3 weeks ago
boldtrn
commented
3 weeks ago I am wondering, isn't it possible to change this on the client side using something like this:
for gestureRecognizer in mapView.gestureRecognizers ?? [] where gestureRecognizer is UITapGestureRecognizer{
gestureRecognizer.addTarget(self, action: #selector(didReceiveTap))
}I agree that the whole Gesture Recognizer logic is a bit odd, so improving this would be nice.
🚨- I haven't actually tested the Waypoint/Route selection from NavigationMapView because I'm not using these features yet. I'd appreciate if someone who is could test this. Otherwise, I expect to add these features in the coming weeks and this could remain open until then.
We should be sure that this actually works :).
One alternative would be to add some kind of mapView(_:unhandledTap:) to a delegate somewhere. But would it be added to the only the NavigationMapView which requires it or MLNMapViewDelegate for consistency? I landed on my approach as the most consistent/least surprising based on current behavior and documentation.
To be honest, I think adding this to the delegate, maybe even a way to either handle taps before and taps after the waypoint/route logic isn't a bad idea either. I am not 100% sure which option would be better though and which option would be more like other iOS libs?
This is kind of a brittle solution.
Yes. One thing that I am especially concerned about is the as casting. If we change the method signature this won't work anymore? I guess there is a 50% chance that we miss this in a refactor and then break the code in an unexpected way.
michaelkirk
commented
3 weeks ago I am wondering, isn't it possible to change this on the client side using something like this:
for gestureRecognizer in mapView.gestureRecognizers ?? [] where gestureRecognizer is UITapGestureRecognizer{ gestureRecognizer.addTarget(self, action: #selector(didReceiveTap)) }
Not a bad idea, but I think that approach might also be brittle. There are 4 pre-existing tap gestures, so we'd probably have to encode some kind of private knowledge of the implementation to get the tap gesture we actually care about.
let mapView = NavigationMapView(frame: .zero, styleURL: styleURL)
let tapGestures = mapView.gestureRecognizers!.filter { $0 is UITapGestureRecognizer }
print("\(tapGestures.count) tap gestures: \(tapGestures)")I suppose we could expose the tap gesture that we want people to extend publicly, and then document it, but I think the delegate approach would probably be more idiomatic/discoverable at that point.
One thing that I am especially concerned about is the as casting. If we change the method signature this won't work anymore?
It's brittle, but it's not that brittle. Note that it's infallible as casting, not as?, so really it's just a type checking hint. It's required because we have to disambiguate between an overloaded method that takes 2 different types.
If we were to change the method signature, it would fail to compile like this:
We should be sure that this actually works :).
I'll convert to a draft until I'm able to test the other code paths.
I'm happy to sit on it for a while to see if we can come up with something better. I do think it's likely that other implementers will want this feature, but I'm not married to this implementation.
Description
The current MLNMapView supports implementers adding their own TapGesture, and even documents how to do this:
For example I use this in maps.earth for selecting POIs rendered in the map layer.
https://github.com/maplibre/maplibre-navigation-ios/assets/217057/e6a54c95-c812-4cf7-9c53-20d2f6c31d84
Unfortunately this doesn't work with
NavigationMapView, which has a catch-all tap gesture that greedily consumes all taps.With this change, we will fail the gesture unless the NavigationMapView intends to actually handle it by selecting a
WayPointorRoute.Unfortunately this presents some redundant work, but I've rearranged things so that the redundant work only occurs in the case that the user has successfully selected a Route or WayPoint.
Open Tasks
none
Infos for Reviewer
This is kind of a brittle solution. There is logic mostly but not quite duplicated across two methods. I've tried to add asserts that should help keep them in sync if one is changed. I'm open to suggestions if someone has a better idea of how to handle this.
One alternative would be to add some kind of
mapView(_:unhandledTap:)to a delegate somewhere. But would it be added to the only theNavigationMapViewwhich requires it orMLNMapViewDelegatefor consistency? I landed on my approach as the most consistent/least surprising based on current behavior and documentation.🚨- I haven't actually tested the
Waypoint/Routeselection fromNavigationMapViewbecause I'm not using these features yet. I'd appreciate if someone who is could test this. Otherwise, I expect to add these features in the coming weeks and this could remain open until then.