Make tap gesture recognizer fail if it doesn’t do anything #7246
Conversation
|
@JesseCrocker, thanks for your PR! By analyzing this pull request, we identified @1ec5, @friedbunny and @incanus to be potential reviewers. |
There was a problem hiding this comment.
This is a great solution to #2278. I have some questions about edge cases below that you probably understand better than I do.
Is any of this relevant to the macOS implementation of MGLMapView, which also has a built-in single tap gesture recognizer?
Would you do the honors and update the changelogs?
| @param singleTap An in progress tap gesture recognizer. | ||
| @param persist True to remember the cycleable set of annotations. @see annotationTagAtPoint:persistingResults | ||
| */ | ||
| - (id <MGLAnnotation> _Nullable)annotationForGestureRecognizer:(UITapGestureRecognizer*)singleTap persistingResults:(BOOL)persist |
There was a problem hiding this comment.
Nit: use the nullable keyword.
| else if (gestureRecognizer == _singleTapGestureRecognizer) | ||
| { | ||
| //Gesture will be recognized if it could deselect an annotation | ||
| if(!self.selectedAnnotation) |
There was a problem hiding this comment.
What if an annotation is selected and tapping would ordinarily deselect it? Wouldn't this prevent the single tap gesture recognizer from firing to deselect the annotation?
There was a problem hiding this comment.
If this is false, it returns YES from the last line of the method, so the recognizer runs
|
|
||
| Your application can use this to modify the behavior of your own gesture recognizers. | ||
| */ | ||
| @property (nonatomic, readonly) UITapGestureRecognizer *singleTapGestureRecognizer; |
There was a problem hiding this comment.
Is it enough for the application's gesture recognizers to avoid conflicts with this one gesture recognizer, or is it also necessary to avoid conflicts with, say, the double-tap gesture recognizer? Would it make sense for the application's gesture recognizer to require all the map view gesture recognizers (via the gestureRecognizers property) to fail? Or would those other gesture recognizers need to defer to the application's single tap gesture recognizer?
There was a problem hiding this comment.
In my case im not actually using this property, and instead im requiring all other gesture recognizers to fail. So maybe this property is not needed. Do you think i should remove it?
for(UIGestureRecognizer *r in self.glMapView.gestureRecognizers) {
[self.tapGestureRecognizer requireGestureRecognizerToFail:r];
}
There was a problem hiding this comment.
Yeah, let’s leave out the property for now. If someone really needs it, they can filter the array for UITapGestureRecognizers as a workaround. That leaves MGLMapView free to install additional gesture recognizers in the future for other purposes.
JesseCrocker
changed the title
1ec5
added
annotations
| @@ -57,6 +57,7 @@ | |||
| * Fixed an issue where the map view’s center would always be calculated as if the view occupied the entire window. ([#6102](https://github.com/mapbox/mapbox-gl-native/pull/6102)) | |||
| * Notification names and user info keys are now string enumeration values for ease of use in Swift. ([#6794](https://github.com/mapbox/mapbox-gl-native/pull/6794)) | |||
| * Fixed a typo in the documentation for the MGLCompassDirectionFormatter class. ([#5879](https://github.com/mapbox/mapbox-gl-native/pull/5879)) | |||
| * The NSClickGestureRecognizer on MGLMapView that is used for selecting annotations now fails if a tap does not select an annotation. ([#7246](https://github.com/mapbox/mapbox-gl-native/pull/7246)) | |||
There was a problem hiding this comment.
Nit: it’s a click, not a tap. 🖱
|
|
||
| Your application can use this to modify the behavior of your own gesture recognizers. | ||
| */ | ||
| @property (nonatomic, readonly) UITapGestureRecognizer *singleTapGestureRecognizer; |
There was a problem hiding this comment.
Yeah, let’s leave out the property for now. If someone really needs it, they can filter the array for UITapGestureRecognizers as a workaround. That leaves MGLMapView free to install additional gesture recognizers in the future for other purposes.
|
Thanks again for this PR – it’s looking solid. By the way, it’d be nice to get this feature into the v3.4.0 or v3.4.1 release. We’re developing the v3.4.x series on the release-ios-v3.4.0 branch. Would you mind retargeting this PR and rebasing this branch on release-ios-v3.4.0? (There’s an option to retarget the PR when editing the PR’s title.) We’ll periodically merge this branch to master as prereleases and releases go out. |
1ec5
changed the title
JesseCrocker
force-pushed
the
tap-gesture-fail
branch
from
b6436f3 to
afe6de9
Compare
|
Rebased, and re-targeted |
I couldn't tell from the discussion on #2278 and #7194 if implementation of this was already in progress or not, but i needed this functionality to be able to continue with a progress. If this is already is progress, please ignore this PR.