[ios, macos] Add shapes annotations select/deselect delegates. by fabian-guerra · Pull Request #9755 · mapbox/mapbox-gl-native

fabian-guerra · 2017-08-11T12:59:14Z

Fixes #2082

The new delegates return a MGLShape annotation. The consideration is for polyllines you get the centroid (coordinate) for the whole line. The initial implementation I made returned a feature and the feature's centroid. I'm thinking add a delegate only for polylines. Thoughts?

1ec5 · 2017-08-30T18:00:15Z

+
+ @param mapView The map view containing the annotation.
+ @param shapeAnnotation The shape annotation that was selected.
+ */


Given that the existing methods are called -mapView:didSelectAnnotation: and -mapView:didDeselectAnnotation:, without referring to point annotations specifically, it’s interesting that this PR adds separate shape-specific methods.

On the one hand, I understand that suddenly passing shape annotations into a method that has long received only point annotations could be a surprise to some developers. On the other hand, we’ve never documented that prior limitation – it’s essentially a bug. There is precedent for opening up delegate methods to new data, namely when we started passing MGLUserLocation into -mapView:viewForAnnotation: in #5882.

In #9755 (comment) I mention an alternative

The reason behind naming this as didSelectShapeAnnotation is based on been clear to developers that this is not going to be called by selecting any *MGLPointAnnotation thus there is no callout view.

boundsj · 2017-08-30T18:34:45Z

+/// Mapping from a shape annotation object to shape layer id.
+typedef std::map<id<MGLAnnotation>, std::string> MGLShapeAnnotationObjectLayerIDMap;
+
+const NSString *MGLLayerIDShapeAnnotation = @"com.mapbox.annotations.shape.";


The @"" string literal NSString is already immutable. So this should be a constant pointer. As it stands, you can repoint MGLLayerIDShapeAnnotation and change its value. Also, this is only used in the implementation so it can be designated static. (i.e. static NSString *const MGLFoo = @"foo")

boundsj · 2017-08-30T18:36:41Z

    MGLAnnotationTagContextMap _annotationContextsByAnnotationTag;
    MGLAnnotationObjectTagMap _annotationTagsByAnnotation;
-
+    MGLShapeAnnotationObjectLayerIDMap _shapeAnnotationLayerIDs;


I think it'd help readability later on to follow the convention (see _annotationTagsByAnnotation just above) and call this _shapeAnnotationLayerIDsByAnnotation

boundsj · 2017-08-30T18:49:44Z

-            if(!annotation) {
-                return NO;
+            if(!annotation && !_selectedShapeAnnotation) {
+                MGLShape *shapeAnnotation = [self shapeAnnotationForGestureRecognizer:(UITapGestureRecognizer*)gestureRecognizer];


Tapping a map with no annotations now crashes in -[MGLMapView shapeAnnotationForGestureRecognizer:] because the hitAnnotationTag is 0 (instead of MGLAnnotationTagNotFound) and that gets a nil result from [self annotationWithTag:hitAnnotationTag] which triggers an assert. I think the root cause is that -[MGLMapView shapeAnnotationTagsInRect:] has no _shapeAnnotationLayerIDs values to put in options so the _mbglMap->queryShapeAnnotations() actually returns some values instead of an empty list.

Fixed. Thank you for catching this bug.

boundsj · 2017-08-30T19:25:41Z

+
+ @param mapView The map view containing the annotation.
+ @param shapeAnnotation The shape annotation that was selected.
+ */


These new APIs don't provide a shape "annotation". They do provide an MGLShape. Since they are intended for MGLPolyline(gon)(Feature) instances, should we remove the word "annotation" from the API and use a more specific type?

MGLShape conforms to MGLAnnotation.

boundsj · 2017-08-30T19:26:44Z

+- (MGLShape*)shapeAnnotationForGestureRecognizer:(UITapGestureRecognizer*)singleTap
+{
+    CGPoint tapPoint = [singleTap locationInView:self];
+//    CGRect queryRect = CGRectInset({ tapPoint, CGSizeZero },


These commented lines were pulled into methods. Don't forget to delete them here.

boundsj · 2017-08-30T19:31:59Z

+    // Choose the first nearby annotation.
+    if (nearbyAnnotations.size())
+    {
+        hitAnnotationTag = nearbyAnnotations.front();


The older -[MGLMapView annotationTagAtPoint:persistingResults:] method has logic to cycle through points that re overlapping / nearby each other. Do we need to worry about that for shapes? What is the expected behavior if a user touches in an area where several polygons overlap or several polylines intersect? cc @1ec5

It’s currently possible to disable touch interaction for a given point annotation using either MGLAnnotationImage.enabled. That’s a weird API, but do we need something like it for shape annotations?

boundsj · 2017-08-30T19:41:56Z

@fabian-guerra wrt comments #9755 (comment) and #9755 (comment), it might be worth another look at the approach taken in #5502. That PR reuses the existing annotation delegate path (and -[MGLMapView annotationTagAtPoint:persistingResults:]) method.

cc @1ec5

1ec5 · 2017-08-31T17:31:20Z

+    [self deselectShapeAnnotation:_selectedShapeAnnotation];
+
+    _selectedShapeAnnotation = shapeAnnotation;
+


The intent of #2082 was that we’d display a callout over the selected shape. Per #2082 (comment), we’d need to constrain the coordinate to the current viewport before calculating the centroid. So coordinate wouldn’t give us quite the right behavior, but we could fashion a method that takes an MGLCoordinateBounds and clips the polygon to polyline to that bounds before passing the geometry off to Polylabel.

The intent of #2082 was that we’d display a callout over the selected shape.

Our Android SDK gained essentially the exact same functionality that is proposed here in #9443. I don't think there is any mechanism in that SDK to assist with callout placement, yet. I think it would be ok to use the same approach and keep #2082 open and handle callouts separately.

I guess that would be an argument for adding a separate set of delegate methods, because it would be weird for -mapView:didSelectAnnotation: to work with -mapView:annotationCanShowCallout: in some cases but not others.

It doesn’t feel great to potentially have a point annotation and shape annotation both be selected at the same time. It’ll lock us into a design that’s reminiscent of MapKit but quite different, even after we implement the callout views.

My principal deterrent to avoid using [MGLMapView annotationTagAtPoint:persistingResults:] was the fact that *MGLShape "annotations" does not have a callout view. So selecting an annotation that handles a callout only in specific cases would be confusing for developers and interpreted as a bug.

But selecting a MGLShape annotation deselects MGLPointAnnotation and viceversa.

I think the broader question is whether shapes should have callouts. The original request in #2082 and much of the subsequent comments were about callouts, so I guess I’d consider that PR unfinished without them. If we land this feature without callouts and with an API that’s separate from normal annotation selection, does that make it more difficult for us to implement shape callouts later? Or are we counting on being able to clean up this API in v4.0.0?

Right, the original request in #2082 was callouts. I should have referred to #3720 which is what the Android implementation #9443 fixes.

Maybe we could wait and refactor our Annotation API, then support callouts for shape annotations, this is how it looks right know so far.

How much of a lift would it be to support callouts for shape annotations? At a glance, it looks like most of the pieces are already in place (especially polylabel).

boundsj · 2017-09-18T17:38:47Z

Closing in favor of #9984

fabian-guerra requested review from 1ec5 and boundsj August 11, 2017 12:59

kkaefer added the iOS Mapbox Maps SDK for iOS label Aug 16, 2017

fabian-guerra self-assigned this Aug 21, 2017

fabian-guerra requested a review from friedbunny August 24, 2017 21:04

fabian-guerra added this to the ios-v3.6.3 milestone Aug 24, 2017

1ec5 reviewed Aug 30, 2017

View reviewed changes

boundsj reviewed Aug 30, 2017

View reviewed changes

1ec5 reviewed Aug 31, 2017

View reviewed changes

fabian-guerra added 2 commits September 1, 2017 20:01

[ios, macos] Add shapes annotations select/deselect delegates.

5164788

[ios, macos] Update changelogs.

4a13933

fabian-guerra force-pushed the fabian-shape-polyline-v2-2082 branch from 4a9edb4 to 4a13933 Compare September 2, 2017 00:09

fabian-guerra mentioned this pull request Sep 13, 2017

[ios, macos] Add selection support to MGLMultiPoint annotations. #9984

Merged

5 tasks

boundsj closed this Sep 18, 2017

lilykaiser removed this from the ios-v3.6.3 milestone Sep 29, 2017

jfirebaugh deleted the fabian-shape-polyline-v2-2082 branch October 24, 2017 20:40

		[self deselectShapeAnnotation:_selectedShapeAnnotation];

		_selectedShapeAnnotation = shapeAnnotation;

Conversation

fabian-guerra commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj commented Aug 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boundsj commented Sep 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fabian-guerra commented Aug 11, 2017 •

edited

Loading