Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/mbgl/map/map.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ class Map : private util::noncopyable {
std::vector<Feature> querySourceFeatures(const std::string& sourceID, const SourceQueryOptions& options = {});

AnnotationIDs queryPointAnnotations(const ScreenBox&);
AnnotationIDs queryShapeAnnotations(const ScreenBox&, const RenderedQueryOptions& options = {});

// Memory
void setSourceTileCacheSize(size_t);
Expand All @@ -213,6 +214,7 @@ class Map : private util::noncopyable {
private:
class Impl;
const std::unique_ptr<Impl> impl;
AnnotationIDs queryAnnotations(const ScreenBox&, const RenderedQueryOptions& options = {});
};

} // namespace mbgl
1 change: 1 addition & 0 deletions platform/ios/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT
* Added support for iOS 11 location usage descriptions. ([#9869](https://github.com/mapbox/mapbox-gl-native/pull/9869))
* Fixed an issue where `MGLUserLocation.location` did not follow its documented initialization behavior. This property will now properly return `nil` until the user’s location has been determined. ([#9639](https://github.com/mapbox/mapbox-gl-native/pull/9639))
* `MGLMapView`’s `minimumZoomLevel` and `maximumZoomLevel` properties are now available in Interface Builder’s Attributes inspector. ([#9729](https://github.com/mapbox/mapbox-gl-native/pull/9729))
* Added support for selection of shape and polyline annotations. ([#9755](https://github.com/mapbox/mapbox-gl-native/pull/9755))

## 3.6.2 - August 18, 2017

Expand Down
10 changes: 10 additions & 0 deletions platform/ios/app/MBXViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -1809,4 +1809,14 @@ - (void)mapView:(MGLMapView *)mapView regionDidChangeAnimated:(BOOL)animated {
}
}

- (void)mapView:(MGLMapView *)mapView didSelectShapeAnnotation:(nonnull MGLShape *)shapeAnnotation
{
NSLog(@"Did Select: %f, %f", shapeAnnotation.coordinate.latitude, shapeAnnotation.coordinate.longitude);
}

- (void)mapView:(MGLMapView *)mapView didDeselectShapeAnnotation:(nonnull MGLShape *)shapeAnnotation
{
NSLog(@"Did deselect: %f, %f", shapeAnnotation.coordinate.latitude, shapeAnnotation.coordinate.longitude);
}

@end
122 changes: 119 additions & 3 deletions platform/ios/src/MGLMapView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ typedef NS_ENUM(NSUInteger, MGLUserTrackingState) {
/// Mapping from an annotation object to an annotation tag.
typedef std::map<id<MGLAnnotation>, MGLAnnotationTag> MGLAnnotationObjectTagMap;

/// Mapping from a shape annotation object to shape layer id.
typedef std::map<id<MGLAnnotation>, std::string> MGLShapeAnnotationObjectLayerIDMap;

static NSString *const MGLLayerIDShapeAnnotation = @"com.mapbox.annotations.shape.";

mbgl::util::UnitBezier MGLUnitBezierForMediaTimingFunction(CAMediaTimingFunction *function)
{
if ( ! function)
Expand Down Expand Up @@ -285,9 +290,11 @@ @implementation MGLMapView

MGLAnnotationTagContextMap _annotationContextsByAnnotationTag;
MGLAnnotationObjectTagMap _annotationTagsByAnnotation;

MGLShapeAnnotationObjectLayerIDMap _shapeAnnotationLayerIDsByAnnotation;

/// Tag of the selected annotation. If the user location annotation is selected, this ivar is set to `MGLAnnotationTagNotFound`.
MGLAnnotationTag _selectedAnnotationTag;
MGLShape *_selectedShapeAnnotation;

BOOL _userLocationAnnotationIsSelected;
/// Size of the rectangle formed by unioning the maximum slop area around every annotation image and annotation image view.
Expand Down Expand Up @@ -468,6 +475,7 @@ - (void)commonInit
_annotationImagesByIdentifier = [NSMutableDictionary dictionary];
_annotationContextsByAnnotationTag = {};
_annotationTagsByAnnotation = {};
_shapeAnnotationLayerIDsByAnnotation = {};
_annotationViewReuseQueueByIdentifier = [NSMutableDictionary dictionary];
_selectedAnnotationTag = MGLAnnotationTagNotFound;
_annotationsNearbyLastTap = {};
Expand Down Expand Up @@ -1483,12 +1491,20 @@ - (void)handleSingleTapGesture:(UITapGestureRecognizer *)singleTap
id<MGLAnnotation>annotation = [self annotationForGestureRecognizer:singleTap persistingResults:YES];
if(annotation)
{
[self deselectShapeAnnotation:_selectedShapeAnnotation];
[self selectAnnotation:annotation animated:YES];
}
else
{
[self deselectAnnotation:self.selectedAnnotation animated:YES];
MGLShape *shapeAnnotation = [self shapeAnnotationForGestureRecognizer:singleTap];
if (shapeAnnotation) {
[self selectShapeAnnotation:shapeAnnotation];
} else {
[self deselectShapeAnnotation:_selectedShapeAnnotation];
}
}

}

/**
Expand Down Expand Up @@ -1861,8 +1877,12 @@ - (BOOL)gestureRecognizerShouldBegin:(UIGestureRecognizer *)gestureRecognizer
if(!self.selectedAnnotation)
{
id<MGLAnnotation>annotation = [self annotationForGestureRecognizer:(UITapGestureRecognizer*)gestureRecognizer persistingResults:NO];
if(!annotation) {
return NO;
if(!annotation && !_selectedShapeAnnotation) {
MGLShape *shapeAnnotation = [self shapeAnnotationForGestureRecognizer:(UITapGestureRecognizer*)gestureRecognizer];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thank you for catching this bug.

if (!shapeAnnotation) {
return NO;
}

}
}
}
Expand Down Expand Up @@ -3226,6 +3246,8 @@ - (void)addAnnotations:(NS_ARRAY_OF(id <MGLAnnotation>) *)annotations
context.annotation = annotation;
_annotationContextsByAnnotationTag[annotationTag] = context;
_annotationTagsByAnnotation[annotation] = annotationTag;
NSString *layerID = [NSString stringWithFormat:@"%@%u", MGLLayerIDShapeAnnotation, annotationTag];
_shapeAnnotationLayerIDsByAnnotation[annotation] = layerID.UTF8String;

[(NSObject *)annotation addObserver:self forKeyPath:@"coordinates" options:0 context:(void *)(NSUInteger)annotationTag];
}
Expand Down Expand Up @@ -3524,6 +3546,7 @@ - (void)removeAnnotations:(NS_ARRAY_OF(id <MGLAnnotation>) *)annotations

_annotationContextsByAnnotationTag.erase(annotationTag);
_annotationTagsByAnnotation.erase(annotation);
_shapeAnnotationLayerIDsByAnnotation.erase(annotation);

if ([annotation isKindOfClass:[NSObject class]] && ![annotation isKindOfClass:[MGLMultiPoint class]])
{
Expand Down Expand Up @@ -4150,6 +4173,99 @@ - (void)applyIconIdentifier:(NSString *)iconIdentifier toAnnotationsWithImageReu
}
}

#pragma mark - Shape Annotation

- (void)selectShapeAnnotation:(MGLShape *)shapeAnnotation
{
if (!shapeAnnotation) return;

if (shapeAnnotation == _selectedShapeAnnotation) return;

[self deselectShapeAnnotation:_selectedShapeAnnotation];

_selectedShapeAnnotation = shapeAnnotation;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
annotation_api

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if ([self.delegate respondsToSelector:@selector(mapView:didSelectShapeAnnotation:)])
{
[self.delegate mapView:self didSelectShapeAnnotation:shapeAnnotation];
}
}

- (void)deselectShapeAnnotation:(MGLShape *)shapeAnnotation
{
if (!shapeAnnotation) return;

if (_selectedShapeAnnotation == shapeAnnotation)
{
if ([self.delegate respondsToSelector:@selector(mapView:didDeselectShapeAnnotation:)])
{
[self.delegate mapView:self didDeselectShapeAnnotation:shapeAnnotation];
}
_selectedShapeAnnotation = nil;
}

}

- (MGLShape*)shapeAnnotationForGestureRecognizer:(UITapGestureRecognizer*)singleTap
{
CGPoint tapPoint = [singleTap locationInView:self];

MGLAnnotationTag hitAnnotationTag = [self shapeAnnotationTagAtPoint:tapPoint];

if (hitAnnotationTag != MGLAnnotationTagNotFound) {
id <MGLAnnotation> annotation = [self annotationWithTag:hitAnnotationTag];
NSAssert(annotation, @"Cannot select nonexistent annotation with tag %u", hitAnnotationTag);
if ([annotation isKindOfClass:[MGLShape class]]) {
return (MGLShape *)annotation;
}
}

return nil;
}

- (MGLAnnotationTag)shapeAnnotationTagAtPoint:(CGPoint)point
{
CGRect queryRect = CGRectInset({ point, CGSizeZero },
-_unionedAnnotationRepresentationSize.width,
-_unionedAnnotationRepresentationSize.height);
queryRect = CGRectInset(queryRect, -MGLAnnotationImagePaddingForHitTest,
-MGLAnnotationImagePaddingForHitTest);

std::vector<MGLAnnotationTag> nearbyAnnotations = [self shapeAnnotationTagsInRect:queryRect];

MGLAnnotationTag hitAnnotationTag = MGLAnnotationTagNotFound;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
return hitAnnotationTag;
}

- (std::vector<MGLAnnotationTag>)shapeAnnotationTagsInRect:(CGRect)rect
{
mbgl::ScreenBox screenBox = {
{ CGRectGetMinX(rect), CGRectGetMinY(rect) },
{ CGRectGetMaxX(rect), CGRectGetMaxY(rect) },
};

std::vector<MGLAnnotationTag> nearbyAnnotations;

mbgl::optional<std::vector<std::string>> optionalLayerIDs;
if (_shapeAnnotationLayerIDsByAnnotation.size()) {
__block std::vector<std::string> layerIDs;
layerIDs.reserve(_shapeAnnotationLayerIDsByAnnotation.size());
for (const auto &annotation : _shapeAnnotationLayerIDsByAnnotation) {
layerIDs.push_back(annotation.second);
}
optionalLayerIDs = layerIDs;
nearbyAnnotations = _mbglMap->queryShapeAnnotations(screenBox, { optionalLayerIDs });
}

return nearbyAnnotations;
}

#pragma mark - User Location -

- (void)validateLocationServices
Expand Down
21 changes: 21 additions & 0 deletions platform/ios/src/MGLMapViewDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,27 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (void)mapView:(MGLMapView *)mapView didDeselectAnnotation:(id <MGLAnnotation>)annotation;

/**
Tells the delegate that one of its shape annotations was selected.

You can use this method to track changes in the selection state of annotations.

@param mapView The map view containing the annotation.
@param shapeAnnotation The shape annotation that was selected.
*/
- (void)mapView:(MGLMapView *)mapView didSelectShapeAnnotation:(MGLShape *)shapeAnnotation;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #9755 (comment) I mention an alternative

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MGLShape conforms to MGLAnnotation.


/**
Tells the delegate that one of its shape annotations was deselected.

You can use this method to track changes in the selection state of annotations.


@param mapView The map view containing the annotation.
@param shapeAnnotation The shape annotation that was deselected.
*/
- (void)mapView:(MGLMapView *)mapView didDeselectShapeAnnotation:(MGLShape *)shapeAnnotation;

/**
Tells the delegate that one of its annotation views was selected.

Expand Down
4 changes: 4 additions & 0 deletions platform/ios/test/MGLMapViewDelegateIntegrationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -78,5 +78,9 @@ extension MGLMapViewDelegateIntegrationTests: MGLMapViewDelegate {
func mapView(_ mapView: MGLMapView, annotation: MGLAnnotation, calloutAccessoryControlTapped control: UIControl) {}

func mapView(_ mapView: MGLMapView, shouldChangeFrom oldCamera: MGLMapCamera, to newCamera: MGLMapCamera) -> Bool { return false }

func mapView(_ mapView: MGLMapView, didSelectShapeAnnotation shapeAnnotation: MGLShape) {}

func mapView(_ mapView: MGLMapView, didDeselectShapeAnnotation shapeAnnotation: MGLShape) {}

}
6 changes: 6 additions & 0 deletions platform/macos/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog for Mapbox macOS SDK

## 0.5.2

This version of the Mapbox macOS SDK corresponds to version 3.6.3 of the Mapbox iOS SDK.

* Added support for selection of shape and polyline annotations. ([#9755](https://github.com/mapbox/mapbox-gl-native/pull/9755))

## 0.5.1

This version of the Mapbox macOS SDK corresponds to version 3.6.2 of the Mapbox iOS SDK.
Expand Down
Loading