[ios] keep callout view open when panning #6676

frederoni · 2016-10-12T12:28:31Z

Fixes #3154
An open callout view now stays open while panning around.
Tapping the map view will dismiss the callout view.

mention-bot · 2016-10-12T12:28:37Z

@frederoni, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @incanus and @boundsj to be potential reviewers.

frederoni · 2016-10-12T12:30:29Z

platform/ios/src/MGLMapView.mm

-                || self.userTrackingMode == MGLUserTrackingModeNone
-                || self.userTrackingState != MGLUserTrackingStateChanged)
-            {
-                [self deselectAnnotation:self.selectedAnnotation animated:NO];


Was dismissing the callout the single purpose of deselecting this annotation?

I believe so. That's what it was for in the raster SDK...

The annotation still needs to be deselected if it lies outside the viewport. See -selectAnnotation:animated:.

frederoni · 2016-10-12T12:31:58Z

platform/ios/src/MGLMapView.mm

+            UIView <MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;
+            if (calloutView && calloutView.representedObject == annotationContext.annotation) {
+                CGPoint point = annotationView.center;
+                point.y -= annotationView.bounds.size.height/2.0f;


Is center-top alignment appropriate for every case?

If we want it to look like MapKit did pre-iOS 10 (when it got rid of callouts of this style), then yes. But we don't have now / this PR still doesn't provide a facility for placement which moves the map before presentation the way SMCalloutView does (see here).

Eventually we’ll support shifting the viewport to fit the callout (#1089) and it might be nice to support alternative popup directions (bottom, side, etc), but let’s not worry about those until #4392.

Eventually we’ll support shifting the viewport to fit the callout (#1089)

More specifically, #3249.

incanus · 2016-10-12T16:55:41Z

platform/ios/src/MGLMapView.mm

-                || self.userTrackingMode == MGLUserTrackingModeNone
-                || self.userTrackingState != MGLUserTrackingStateChanged)
-            {
-                [self deselectAnnotation:self.selectedAnnotation animated:NO];


I believe so. That's what it was for in the raster SDK...

incanus · 2016-10-12T16:57:23Z

platform/ios/src/MGLMapView.mm

+            UIView <MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;
+            if (calloutView && calloutView.representedObject == annotationContext.annotation) {
+                CGPoint point = annotationView.center;
+                point.y -= annotationView.bounds.size.height/2.0f;


If we want it to look like MapKit did pre-iOS 10 (when it got rid of callouts of this style), then yes. But we don't have now / this PR still doesn't provide a facility for placement which moves the map before presentation the way SMCalloutView does (see here).

incanus · 2016-10-12T21:59:38Z

👍

1ec5

Please update the iOS changelog.

friedbunny · 2016-10-17T19:04:11Z

Bumpin’ — I think this is good to go, once there’s a changelog entry?

1ec5 · 2016-10-17T20:28:50Z

Come to think of it, we should probably still deselect the annotation it goes off-screen. We do that for the user location annotation.

friedbunny · 2016-10-17T20:40:56Z

I’m not seeing the callout move with the annotation:

frederoni · 2016-10-20T14:58:48Z

Good catch @friedbunny. It only works for annotation views so far.

frederoni · 2016-10-20T16:39:04Z

232817a fixes #6676 (comment) but the implementation is broken. The callout view shouldn't be positioned in -[MGLMapView updateAnnotationViews], partially because it's not related but also because it gets interrupted if the delegate doesn't respond to -[MGLMapViewDelegate mapView:viewForAnnotation:]

1ec5 · 2016-11-29T03:09:27Z

platform/ios/src/MGLMapView.mm

+            UIView <MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;
+            if (calloutView && calloutView.representedObject == annotationContext.annotation) {
+                CGPoint point = annotationView.center;
+                point.y -= annotationView.bounds.size.height/2.0f;


This doesn’t account for any centerOffset on the annotation view.

1ec5 · 2016-11-29T03:10:12Z

platform/ios/src/MGLMapView.mm

+                    if (image)
+                    {
+                        CGPoint point = [self convertCoordinate:annotation.coordinate toPointToView:self];
+                        point.y -= image.image.size.height/2.0f;


This doesn’t account for the annotation image’s alignment rect insets.

1ec5 · 2016-11-30T16:12:33Z

platform/ios/src/MGLMapView.mm

+                CGPoint point = annotationView.center;
+                point.x += annotationView.centerOffset.dx;
+                point.y += annotationView.centerOffset.dy;
+                point.y -= annotationView.bounds.size.height/2.0f;


It would be simpler to calculate the point based on the annotation view's frame, i.e., CGGetMinY(annotationView.frame).

MinY from the frame is not what I'm trying to achieve here but MidY from the bounds would work.

My point was that all four of these lines could become something like:

CGPoint point = CGPointMake(CGGetMidX(annotationView.frame), CGGetMinY(annotationView.frame));

Also, looking back at this, I’m not sure if center offset is already accounted for in the center and frame. I think it may already be.

1ec5 · 2016-11-30T16:15:29Z

platform/ios/src/MGLMapView.mm

+            UIView <MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;
+            if (calloutView && calloutView.representedObject == annotationContext.annotation) {
+                NSObject<MGLAnnotation> *annotation = annotationContext.annotation;
+                BOOL implementsImageForAnnotation = [self.delegate respondsToSelector:@selector(mapView:imageForAnnotation:)];


Move this outside the loop.

1ec5 · 2016-11-30T16:17:30Z

platform/ios/src/MGLMapView.mm

+                NSObject<MGLAnnotation> *annotation = annotationContext.annotation;
+                BOOL implementsImageForAnnotation = [self.delegate respondsToSelector:@selector(mapView:imageForAnnotation:)];
+                if (implementsImageForAnnotation) {
+                    MGLAnnotationImage *image = [self.delegate mapView:self imageForAnnotation:annotation];


Use -imageOfAnnotationWithTag: to avoid asking the delegate for information the map view already has.

1ec5 · 2016-11-30T16:28:37Z

platform/ios/src/MGLMapView.mm

+            }
+        } else {
+            // Pin the callout view to the gl annotation
+            UIView <MGLCalloutView> *calloutView = self.calloutViewForSelectedAnnotation;


Like -selectAnnotation:animated:, this method should use -positioningRectForCalloutForAnnotationWithTag:, which accounts for the padding around the annotation image and avoids half-pixel issues. To convert from the positioning rect to an anchor point, use CGRectGetMidX() and CGRectGetMinY.

1ec5 · 2016-11-30T16:29:45Z

platform/ios/src/MGLMapView.mm

-                || self.userTrackingMode == MGLUserTrackingModeNone
-                || self.userTrackingState != MGLUserTrackingStateChanged)
-            {
-                [self deselectAnnotation:self.selectedAnnotation animated:NO];


The annotation still needs to be deselected if it lies outside the viewport. See -selectAnnotation:animated:.

1ec5 · 2016-12-01T17:23:49Z

platform/ios/src/MGLMapView.mm

+            if (image)
+            {
+                CGRect rect = [self positioningRectForCalloutForAnnotationWithTag:tag];
+                CGRect insetRect = UIEdgeInsetsInsetRect(rect, image.image.alignmentRectInsets);


-positioningRectForCalloutForAnnotationWithTag: already calls -frameOfImage:centeredAtCoordinate:, so it already accounts for the alignment rect insets.

1ec5 · 2016-12-01T17:25:32Z

platform/ios/src/MGLMapView.mm

+                image = [self dequeueReusableAnnotationImageWithIdentifier:MGLDefaultStyleMarkerSymbolName];
+            }
+
+            if (image)


Is it necessary to guard for the presence of an image here? -positioningRectForCalloutForAnnotationWithTag: already looks up the annotation’s image.

1ec5 · 2016-12-02T16:33:33Z

platform/ios/src/MGLMapView.mm

+        {
+            MGLAnnotationImage *image = [self imageOfAnnotationWithTag:tag];
+
+            if (!image && [self.delegate respondsToSelector:@selector(mapView:imageForAnnotation:)]) {


I think -imageOfAnnotationWithTag: should be good enough. We shouldn't be overusing this delegate method.

Actually, image is unused now that we're getting the positioning rect below.

1ec5 · 2016-12-02T16:37:28Z

platform/ios/src/MGLMapView.mm

+            CGPoint point = annotationView.center;
+            point.x += annotationView.centerOffset.dx;
+            point.y += annotationView.centerOffset.dy;
+            point.y -= CGRectGetMidY(annotationView.bounds);


At this point, I think you can pretty much collapse the view and non-view cases together: for view-backed annotations, rect is the view's frame; for GL annotations, rect comes from -positioningRectForCalloutForAnnotationWithTag:.

The center (and thus the bounds and frame) already account for the center offset. Sorry for being unclear about that previously. This does mean you can combine the view and non-view cases, just like you do in the MapChangeRegionWillChange handler above.

boundsj · 2016-12-02T23:28:08Z

platform/ios/src/MGLMapView.mm

@@ -4513,7 +4513,26 @@ - (void)notifyMapChange:(mbgl::MapChange)change
                || self.userTrackingMode == MGLUserTrackingModeNone
                || self.userTrackingState != MGLUserTrackingStateChanged)
            {
-                [self deselectAnnotation:self.selectedAnnotation animated:NO];
+                // Deselect annotation if it lies outside the viewport


This is solving for something similar to -[MGLMapView updateAnnotationViews]. I wonder if checking if the -array -[MGLMapView visibleAnnotations] contains the selected annotation would be fast enough. If it is, it would simplify this implementation.

We already call -visibleAnnotations soon after this in order to reposition annotation views. We could refactor this code to call `-visibleAnnotations upfront and reuse the results for placement of annotation views, the callout view, and (on macOS) tooltip and cursor rects.

I think -visibleAnnotations is fine but -[NSArray containsObject:] is O(n2) and takes 2ms with 100 annotations. Not sure if it's in the beginning or the end but it would decrease the performance while panning around.

I agree. Ideally visibleAnnotations would be a set, but unfortunately the way we compute it (using feature querying) means it’s always going to be O(n) regardless. (Not O(n²), because we’re only checking whether one particular annotation is visible.)

1ec5

A couple remaining things to take care of, but I’ll remove my senatorial hold now. 😉

1ec5 · 2016-12-03T19:19:18Z

platform/ios/src/MGLMapView.mm

@@ -4513,7 +4513,26 @@ - (void)notifyMapChange:(mbgl::MapChange)change
                || self.userTrackingMode == MGLUserTrackingModeNone
                || self.userTrackingState != MGLUserTrackingStateChanged)
            {
-                [self deselectAnnotation:self.selectedAnnotation animated:NO];
+                // Deselect annotation if it lies outside the viewport


I agree. Ideally visibleAnnotations would be a set, but unfortunately the way we compute it (using feature querying) means it’s always going to be O(n) regardless. (Not O(n²), because we’re only checking whether one particular annotation is visible.)

1ec5 · 2016-12-03T19:31:53Z

platform/ios/src/MGLMapView.mm

@@ -4631,6 +4646,9 @@ - (void)updateAnnotationViews

    if (!delegateImplementsViewForAnnotation)
    {
+        [CATransaction begin];
+        [self updateCalloutView];


Have you considered moving the calls to -updateCalloutView to directly within the MapChangeDidFinishRenderingFrame handler, after the call to -updateAnnotationViews? That would avoid the double call here, and I think it’d be more intuitive, since we need to reposition the callout view even if there are no view-backed annotations.

For reference, the macOS implementation calls -updateAnnotationCallouts within the MapChangeRegionIsChanging and MapChangeRegionDidChange handlers, which may fire less frequently. It makes sense that we’d want to keep the callout view in sync with any annotation views we update in MapChangeDidFinishRenderingFrame, though.

1ec5 · 2016-12-03T19:36:32Z

platform/ios/src/MGLMapView.mm

+            CGPoint point = annotationView.center;
+            point.x += annotationView.centerOffset.dx;
+            point.y += annotationView.centerOffset.dy;
+            point.y -= CGRectGetMidY(annotationView.bounds);


The center (and thus the bounds and frame) already account for the center offset. Sorry for being unclear about that previously. This does mean you can combine the view and non-view cases, just like you do in the MapChangeRegionWillChange handler above.

frederoni · 2016-12-04T10:05:13Z

Thanks for iterating with me on this one. I hope one final glance is sufficient.

frederoni added iOS Mapbox Maps SDK for iOS needs discussion labels Oct 12, 2016

frederoni self-assigned this Oct 12, 2016

frederoni commented Oct 12, 2016

View reviewed changes

frederoni force-pushed the 3154-keep-callout-open branch 2 times, most recently from 974fb16 to 7a6733c Compare October 12, 2016 13:52

incanus approved these changes Oct 12, 2016

View reviewed changes

1ec5 suggested changes Oct 13, 2016

View reviewed changes

friedbunny added feature annotations Annotations on iOS and macOS or markers on Android and removed needs discussion labels Oct 17, 2016

friedbunny added this to the ios-v3.4.0 milestone Oct 17, 2016

frederoni added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold needs discussion labels Oct 20, 2016

frederoni changed the base branch from master to release-ios-v3.4.0 November 16, 2016 11:45

frederoni force-pushed the 3154-keep-callout-open branch 2 times, most recently from 3511e71 to 46afa93 Compare November 22, 2016 16:07

frederoni added ✓ ready for review and removed needs discussion ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 22, 2016

1ec5 suggested changes Nov 29, 2016

View reviewed changes

1ec5 suggested changes Nov 30, 2016

View reviewed changes

frederoni force-pushed the 3154-keep-callout-open branch from 16df3aa to 5bffd38 Compare December 1, 2016 15:38

1ec5 suggested changes Dec 1, 2016

View reviewed changes

1ec5 suggested changes Dec 2, 2016

View reviewed changes

boundsj approved these changes Dec 2, 2016

View reviewed changes

1ec5 approved these changes Dec 3, 2016

View reviewed changes

1ec5 removed the ✓ ready for review label Dec 4, 2016

frederoni force-pushed the 3154-keep-callout-open branch from 89f29f4 to 47a57f6 Compare December 4, 2016 09:46

[ios] keep callout view open when panning

307b4d5

frederoni force-pushed the 3154-keep-callout-open branch from 47a57f6 to 307b4d5 Compare December 4, 2016 09:53

1ec5 approved these changes Dec 5, 2016

View reviewed changes

frederoni merged commit 440e06c into release-ios-v3.4.0 Dec 5, 2016

frederoni deleted the 3154-keep-callout-open branch December 5, 2016 10:42

1ec5 mentioned this pull request Dec 5, 2016

Annotation callout closes automatically when changing the viewport #3154

Closed

This was referenced Dec 22, 2016

MGLMapView moves custom MGLCalloutView, ignoring custom frame #7483

Closed

Callout dismisses when annotation coordinate changes slightly #6284

Closed

friedbunny mentioned this pull request Jan 11, 2017

Optional positioning and stickiness for callout view #7646

Merged

rmnblm mentioned this pull request Feb 8, 2017

Callout sticks to edge of screen when panning quickly #7982

Closed

1ec5 mentioned this pull request Aug 8, 2017

selectAnnotation changes the tracking mode to None #9455

Closed

This was referenced Sep 6, 2017

-[MGLMapView selectAnnotation:animated:] does not pan map to fit callout view #3249

Closed

Selecting an off-screen annotation doesn't work #9790

Closed

pushian mentioned this pull request Jan 12, 2018

Customised annotation callout closes automatically when changing the viewport #10919

Closed

[ios] keep callout view open when panning #6676

[ios] keep callout view open when panning #6676

Conversation

frederoni commented Oct 12, 2016 • edited Loading

mention-bot commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

incanus commented Oct 12, 2016 • edited by friedbunny Loading

1ec5 left a comment

Choose a reason for hiding this comment

friedbunny commented Oct 17, 2016

1ec5 commented Oct 17, 2016

friedbunny commented Oct 17, 2016

frederoni commented Oct 20, 2016

frederoni commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Dec 4, 2016

frederoni commented Oct 12, 2016 •

edited

Loading

incanus Oct 12, 2016 •

edited

Loading

incanus Oct 12, 2016 •

edited

Loading

incanus commented Oct 12, 2016 •

edited by friedbunny

Loading

1ec5 Dec 1, 2016 •

edited

Loading