Update annotation view touch handling (with offsets) #12234
Conversation
julianrex
added
iOS
| @@ -1658,23 +1658,6 @@ - (void)handleSingleTapGesture:(UITapGestureRecognizer *)singleTap | |||
| } | |||
| } | |||
|
|
|||
| // Handle the case of an offset annotation view by converting the tap point to be the geo location | |||
| // of the annotation itself that the view represents | |||
| for (MGLAnnotationView *view in self.annotationContainerView.annotationViews) | |||
There was a problem hiding this comment.
This loop has been removed and the offset logic moved deeper into the annotation query.
- tapPoint is mutated within this loop, potentially leading to drifting away from the original tap
- With an offset, the tap point can end up outside the annotation view (but still within the bounds of the source annotation) - this leads to annotations being selected when it looks like they shouldn't.
There was a problem hiding this comment.
I'd like to understand what the point of this loop was, in case I'm missing something obvious 🤔
There was a problem hiding this comment.
That loop was added to fix a similar bug #5931
| _largestAnnotationViewSize = CGSizeMake(MAX(_largestAnnotationViewSize.width, CGRectGetWidth(bounds)), | ||
| MAX(_largestAnnotationViewSize.height, CGRectGetHeight(bounds))); | ||
| // Take any offset into consideration | ||
| CGFloat adjustedAnnotationWidth = CGRectGetWidth(bounds) + fabs(annotationView.centerOffset.dx); |
There was a problem hiding this comment.
The annotation's apparent size now includes any offset, so that the initial annotation query returns the correct results. This means that _unionedAnnotationRepresentationSize can now be larger. This could be a performance issue with lots of annotations if the offsets are large.
platform/ios/src/MGLMapView.mm
Outdated
|
|
||
| // We need to take any offset into consideration. Note that a large offset will result in a | ||
| // large value for `_unionedAnnotationRepresentationSize` - which will affect performance for lots | ||
| // of annotations. Keep the offset as small as possible. |
There was a problem hiding this comment.
Would this note be worth adding to the API docs for centerOffset?
There was a problem hiding this comment.
Good question. Since the performance hasn't been measured and the extent of this (expected) effect is unknown perhaps we don't right now? In that case, maybe the above comment needs to be amended too?
| @@ -0,0 +1,68 @@ | |||
| // | |||
| // MGLAnnotationViewIntegrationTests.m | |||
There was a problem hiding this comment.
Oops this header snuck into the test file.
There was a problem hiding this comment.
This is WIP - but good catch :) I need to turn that off
julianrex
force-pushed
the
jrex-11829-touch-handling-with-offset
branch
from
36ad3fa to
ad8ca4c
Compare
|
Rebased. |
|
@julianrex Thanks — this’ll also need a changelog entry. |
friedbunny
added
the
needs changelog
julianrex
force-pushed
the
jrex-11829-touch-handling-with-offset
branch
from
04e4db8 to
4f6138a
Compare
|
Rebased. |
julianrex
removed
the
needs changelog
Addresses #11829
This PR adds the offset to the annotation rect checking in
-[MGLMapView annotationTagAtPoint:persistingResults:](and associated changes).