From b460efdf00092797446570389a6c9afa230da162 Mon Sep 17 00:00:00 2001 From: Julian Rex Date: Tue, 22 Jan 2019 17:45:06 -0500 Subject: [ios] Partially offscreen annotations (without callouts) are no longer moved on-screen (#13727) --- platform/ios/CHANGELOG.md | 1 + .../MGLAnnotationViewIntegrationTests.m | 409 ++++++++++++++++++++- .../Integration Tests/MGLMapViewIntegrationTest.h | 9 + .../Integration Tests/MGLMapViewIntegrationTest.m | 14 + platform/ios/ios.xcodeproj/project.pbxproj | 4 + platform/ios/src/MGLMapView.mm | 101 +++-- platform/ios/vendor/SMCalloutView/SMCalloutView.m | 12 +- platform/macos/CHANGELOG.md | 2 +- platform/macos/src/MGLMapView.mm | 69 ++-- platform/macos/test/MGLAnnotationTests.m | 2 +- 10 files changed, 562 insertions(+), 61 deletions(-) diff --git a/platform/ios/CHANGELOG.md b/platform/ios/CHANGELOG.md index 58413b3184..9212b97c9a 100644 --- a/platform/ios/CHANGELOG.md +++ b/platform/ios/CHANGELOG.md @@ -10,6 +10,7 @@ Mapbox welcomes participation and contributions from everyone. Please read [CONT * Reinstates version 11 as the default Mapbox Streets style (as introduced in 4.7.0). ([#13690](https://github.com/mapbox/mapbox-gl-native/pull/13690)) * Added the `-[MGLShapeSource leavesOfCluster:offset:limit:]`, `-[MGLShapeSource childrenOfCluster:]`, `-[MGLShapeSource zoomLevelForExpandingCluster:]` methods for inspecting a cluster in an `MGLShapeSource`s created with the `MGLShapeSourceOptionClustered` option. Feature querying now returns clusters represented by `MGLPointFeatureCluster` objects (that conform to the `MGLCluster` protocol). ([#12952](https://github.com/mapbox/mapbox-gl-native/pull/12952) * `MGLMapView` no longer freezes on external displays connected through AirPlay or CarPlay when the main device’s screen goes to sleep or the user manually locks the screen. ([#13701](https://github.com/mapbox/mapbox-gl-native/pull/13701)) +* Fixed a bug where selecting partially on-screen annotations (without a callout) would move the map. ([#13727](https://github.com/mapbox/mapbox-gl-native/pull/13727)) ## 4.7.1 - December 21, 2018 diff --git a/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m b/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m index fefb938773..7ec45de072 100644 --- a/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m +++ b/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m @@ -2,9 +2,26 @@ #import "MGLTestUtility.h" #import "MGLMapAccessibilityElement.h" #import "MGLTestLocationManager.h" +#import "MGLCompactCalloutView.h" + +@interface MGLTestCalloutView : MGLCompactCalloutView +@property (nonatomic) BOOL implementsMarginHints; +@end + +@implementation MGLTestCalloutView +- (BOOL)respondsToSelector:(SEL)aSelector { + if (!self.implementsMarginHints && + (aSelector == @selector(marginInsetsHintForPresentationFromRect:))) { + return NO; + } + return [super respondsToSelector:aSelector]; +} +@end + @interface MGLMapView (Tests) - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)persist; +@property (nonatomic) UIView *calloutViewForSelectedAnnotation; @end @interface MGLAnnotationViewIntegrationTests : MGLMapViewIntegrationTest @@ -12,6 +29,368 @@ @implementation MGLAnnotationViewIntegrationTests +#pragma mark - Offscreen/panning selection tests + +typedef struct PanTestData { + CGPoint relativeCoord; + BOOL showsCallout; + BOOL implementsMargins; + BOOL moveIntoView; + BOOL expectMapToHavePanned; + BOOL calloutOnScreen; +} PanTestData; + +#define PAN_TEST_TERMINATOR {{FLT_MAX, FLT_MAX}, NO, NO, NO, NO, NO} +static const CGFloat kAnnotationScale = 0.125f; + +- (void)internalTestOffscreenSelectionTitle:(NSString*)title withTestData:(PanTestData)test animateSelection:(BOOL)animateSelection { + + CGPoint relativeCoordinate = test.relativeCoord; + BOOL showsCallout = test.showsCallout; + BOOL calloutImplementsMarginHints = test.implementsMargins; + BOOL moveIntoView = test.moveIntoView; + BOOL expectMapToHavePanned = test.expectMapToHavePanned; + BOOL expectCalloutToBeFullyOnscreen = test.calloutOnScreen; + + // Reset the map to a consistent state - want the map to be zoomed in, so that + // it's free to be panned without hitting boundaries. + [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(0, 0) zoomLevel:14 animated:NO]; + [self waitForMapViewToBeRenderedWithTimeout:1.0]; + + XCTAssert(self.mapView.annotations.count == 0); + + NSString * const MGLTestAnnotationReuseIdentifer = @"MGLTestAnnotationReuseIdentifer"; + CGSize size = self.mapView.bounds.size; + CGSize annotationSize = CGSizeMake(floor(size.width*kAnnotationScale), floor(size.height*kAnnotationScale)); + + self.viewForAnnotation = ^MGLAnnotationView*(MGLMapView *view, id annotation) { + + if (![annotation isKindOfClass:[MGLPointAnnotation class]]) { + return nil; + } + + // No dequeue + MGLAnnotationView *annotationView = [[MGLAnnotationView alloc] initWithAnnotation:annotation reuseIdentifier:MGLTestAnnotationReuseIdentifer]; + annotationView.bounds = (CGRect){ .origin = CGPointZero, .size = annotationSize }; + annotationView.backgroundColor = UIColor.redColor; + annotationView.enabled = YES; + + return annotationView; + }; + + // Coordinate for annotation screen coordinate + CGPoint annotationPoint = CGPointMake(relativeCoordinate.x * size.width, relativeCoordinate.y * size.height); + CLLocationCoordinate2D coordinate = [self.mapView convertPoint:annotationPoint toCoordinateFromView:self.mapView]; + + MGLPointAnnotation *point = [[MGLPointAnnotation alloc] init]; + point.title = title; + point.coordinate = coordinate; + + self.mapViewAnnotationCanShowCalloutForAnnotation = ^BOOL(MGLMapView *mapView, id annotation) { + return showsCallout; + }; + + self.mapViewCalloutViewForAnnotation = ^id(MGLMapView *mapView, id annotation) { + if (!showsCallout) + return nil; + + MGLTestCalloutView *calloutView = [[MGLTestCalloutView alloc] init]; + calloutView.representedObject = annotation; + calloutView.implementsMarginHints = calloutImplementsMarginHints; + return calloutView; + }; + + [self.mapView addAnnotation:point]; + + // Check assumptions before selection + UIView *annotationViewBeforeSelection = [self.mapView viewForAnnotation:point]; + XCTAssertNotNil(annotationViewBeforeSelection); + + CLLocationCoordinate2D mapCenterBeforeSelection = self.mapView.centerCoordinate; + XCTAssert(CLLocationCoordinate2DIsValid(mapCenterBeforeSelection)); + + // Also note, that at this point, the internal mechanism that determines if + // an annotation view is offscreen and should be put back in the reuse queue + // will have run, and `viewForAnnotation` may return nil + + [self.mapView selectAnnotation:point moveIntoView:moveIntoView animateSelection:animateSelection]; + + // Animated selection takes MGLAnimationDuration (0.3 seconds), so wait a little + // longer. We don't need to wait as long if we're not animated (but we do + // want the runloop to tick over) + [self waitFor:animateSelection ? 0.4: 0.05]; + + UIView *annotationViewAfterSelection = [self.mapView viewForAnnotation:point]; + CLLocationCoordinate2D mapCenterAfterSelection = self.mapView.centerCoordinate; + XCTAssert(CLLocationCoordinate2DIsValid(mapCenterAfterSelection)); + + // If the annotation is still "offscreen" at this point, then the above annotation view + // may be nil, which is expected. + BOOL (^CGRectContainsRectWithAccuracy)(CGRect, CGRect, CGFloat) = ^(CGRect rect1, CGRect rect2, CGFloat accuracy) { + CGRect expandedRect1 = CGRectInset(rect1, -accuracy, -accuracy); + return CGRectContainsRect(expandedRect1, rect2); + }; + + CGFloat epsilon = 0.00001; + if (expectMapToHavePanned) { + CLLocationDegrees latitudeDelta = fabs(mapCenterAfterSelection.latitude - mapCenterBeforeSelection.latitude); + CLLocationDegrees longitudeDelta = fabs(mapCenterAfterSelection.longitude - mapCenterBeforeSelection.longitude); + + XCTAssert( (latitudeDelta > epsilon) || (longitudeDelta > epsilon), @"Deltas: lat=%f, long=%f", latitudeDelta, longitudeDelta); // One of them should have moved + + // If the map panned - the intention is that the annotation is on-screen, + // and so should have an annotation view and that it is fully on screen + CGRect annotationFrameAfterSelection = annotationViewAfterSelection.frame; + + XCTAssertNotNil(annotationViewAfterSelection); + + XCTAssert(CGRectContainsRectWithAccuracy(self.mapView.bounds, annotationFrameAfterSelection, 0.25), @"Mapview:%@ frame:%@", NSStringFromCGRect(self.mapView.bounds), NSStringFromCGRect(annotationFrameAfterSelection)); + + // Check the callout + if (showsCallout) { + UIView *calloutView = self.mapView.calloutViewForSelectedAnnotation; + XCTAssertNotNil(calloutView); + + // If kAnnotationScale == 0.25, then the following assert can fail. + // This is really a warning (see https://github.com/mapbox/mapbox-gl-native/issues/13744 ) + // If you need this NOT to fail the tests, consider replacing with MGLTestWarning + XCTAssert(expectCalloutToBeFullyOnscreen == CGRectContainsRectWithAccuracy(self.mapView.bounds, calloutView.frame, 0.25), + @"Expect contains:%d, Mapview:%@ annotation:%@ callout:%@", + expectCalloutToBeFullyOnscreen, + NSStringFromCGRect(self.mapView.bounds), + NSStringFromCGRect(annotationFrameAfterSelection), + NSStringFromCGRect(calloutView.frame)); + } + } + else { + // The map shouldn't have moved, so use equality (rather than an error check) + XCTAssertEqual(mapCenterBeforeSelection.latitude, mapCenterAfterSelection.latitude); + XCTAssertEqual(mapCenterBeforeSelection.longitude, mapCenterAfterSelection.longitude); + + // Annotation shouldn't have moved + CGPoint annotationPoint2 = [self.mapView convertCoordinate:point.coordinate toPointToView:self.mapView]; + CGFloat xDelta = fabs(annotationPoint2.x - annotationPoint.x); + CGFloat yDelta = fabs(annotationPoint2.y - annotationPoint.y); + + XCTAssert((xDelta < epsilon) && (yDelta < epsilon)); + + if (showsCallout) { + UIView *calloutView = self.mapView.calloutViewForSelectedAnnotation; + + if (annotationViewAfterSelection) { + XCTAssertNotNil(calloutView); + + // If kAnnotationScale == 0.25, then the following assert can fail. + // This is really a warning (see https://github.com/mapbox/mapbox-gl-native/issues/13744 ) + // If you need this NOT to fail the tests, consider replacing with MGLTestWarning + XCTAssert((expectCalloutToBeFullyOnscreen == CGRectContainsRectWithAccuracy(self.mapView.bounds, calloutView.frame, 0.25)), + @"Mapview:%@ annotation:%@ callout:%@", + NSStringFromCGRect(self.mapView.bounds), + NSStringFromCGRect(annotationViewAfterSelection.frame), + NSStringFromCGRect(calloutView.frame)); + } + else { + // If there's no annotation view, should we expect a callout? + XCTAssertNil(calloutView); + XCTAssertFalse(expectCalloutToBeFullyOnscreen); + } + } + } + + // Remove the annotation + [self.mapView removeAnnotation:point]; + + XCTAssert(self.mapView.annotations.count == 0); +} + +// See https://github.com/mapbox/mapbox-gl-native/pull/13727#issuecomment-454028698 +// What follows are tests based on this table. +// This is not a full-set of possible combinations, just the most important/likely +// ones +- (void)internalRunTests:(PanTestData*)testData +{ + // Test both animated and not-animated. + for (int i = 0; i<2; i++) { + int row = 0; + PanTestData *test = testData; + while (test->relativeCoord.x != FLT_MAX) { + NSString *activityTitle = [NSString stringWithFormat:@"Row %d/%d", row, i]; + [XCTContext runActivityNamed:activityTitle + block:^(id _Nonnull activity) { + [self internalTestOffscreenSelectionTitle:activityTitle + withTestData:*test + animateSelection:!i]; + }]; + ++test; + ++row; + } + } +} + +- (void)testBasicSelection { + // Tests moveIntoView:NO + // WITHOUT a callout + + PanTestData tests[] = { + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + // Offscreen + { {-1.0f, 0.5f}, NO, NO, NO, NO, NO }, + { { 2.0f, 0.5f}, NO, NO, NO, NO, NO }, + { { 0.5f,-1.0f}, NO, NO, NO, NO, NO }, + { { 0.5f, 2.0f}, NO, NO, NO, NO, NO }, + + // Partial + { { 0.0f, 0.5f}, NO, NO, NO, NO, NO }, + { { 1.0f, 0.5f}, NO, NO, NO, NO, NO }, + { { 0.5f, 0.0f}, NO, NO, NO, NO, NO }, + { { 0.5f, 1.0f}, NO, NO, NO, NO, NO }, + + // Onscreen + { { 0.5f, 0.5f}, NO, NO, NO, NO, NO }, + + PAN_TEST_TERMINATOR + }; + + [self internalRunTests:tests]; +} + +- (void)testBasicSelectionWithCallout { + // Tests moveIntoView:NO + // WITH the default callout (implements marginshint) + + PanTestData tests[] = { + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + { {-1.0f, 0.5f}, YES, YES, NO, NO, NO }, + { { 0.0f, 0.5f}, YES, YES, NO, NO, NO }, + { { 0.5f, 1.0f}, YES, YES, NO, NO, YES }, // Because annotation was off the bottom of screen, and callout is above annotation + { { 0.5f, 0.5f}, YES, YES, NO, NO, YES }, + + PAN_TEST_TERMINATOR + }; + + [self internalRunTests:tests]; +} + +- (void)testSelectionMoveIntoView { + // Tests moveIntoView:YES + // without a callout + + // From https://github.com/mapbox/mapbox-gl-native/pull/13727#issuecomment-454028698 + // + // | Annotation position | Has callout? | Callout implements `marginInsets...`? | Map pans when selected with moveIntoView=YES? | + // |---------------------|--------------|---------------------------------------|-----------------------------------------------| + // | Offscreen | No | n/a | Yes (no margins) | + // | Partially | No | n/a | No | + // | Onscreen | No | n/a | No | + // + + PanTestData tests[] = { + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + // Offscreen + { {-1.0f, 0.5f}, NO, NO, YES, YES, NO }, + { { 2.0f, 0.5f}, NO, NO, YES, YES, NO }, + { { 0.5f,-1.0f}, NO, NO, YES, YES, NO }, + { { 0.5f, 2.0f}, NO, NO, YES, YES, NO }, + + // Partial + { { 0.0f, 0.5f}, NO, NO, YES, NO, NO }, + { { 1.0f, 0.5f}, NO, NO, YES, NO, NO }, + { { 0.5f, 0.0f}, NO, NO, YES, NO, NO }, + { { 0.5f, 1.0f}, NO, NO, YES, NO, NO }, + + // Onscreen + { { 0.5f, 0.5f}, NO, NO, YES, NO, NO }, + + PAN_TEST_TERMINATOR + }; + + [self internalRunTests:tests]; +} + +- (void)testSelectionMoveIntoViewWithCallout { + // Tests moveIntoView:YES + // WITH the default callout (implements marginshint) + + // From https://github.com/mapbox/mapbox-gl-native/pull/13727#issuecomment-454028698 + // + // | Annotation position | Has callout? | Callout implements `marginInsets...`? | Map pans when selected with moveIntoView=YES? | + // |---------------------|--------------|---------------------------------------|-----------------------------------------------| + // | Offscreen | Yes | Yes | Yes to ensure callout is fully visible | + // | Partially | Yes | Yes | Yes to ensure callout is fully visible | + // | Onscreen | Yes | Yes | Yes, but *only* to ensure callout is fully visible | + // + + PanTestData tests[] = { + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + // Offscreen + { {-1.0f, 0.5f}, YES, YES, YES, YES, YES }, + { { 2.0f, 0.5f}, YES, YES, YES, YES, YES }, + { { 0.5f,-1.0f}, YES, YES, YES, YES, YES }, + { { 0.5f, 2.0f}, YES, YES, YES, YES, YES }, + + // Partial + { { 0.0f, 0.5f}, YES, YES, YES, YES, YES }, + { { 1.0f, 0.5f}, YES, YES, YES, YES, YES }, + { { 0.5f, 0.0f}, YES, YES, YES, YES, YES }, + { { 0.5f, 1.0f}, YES, YES, YES, YES, YES }, + + // Onscreen + { { 0.5f, 0.5f}, YES, YES, YES, NO, YES }, + + // Just at the edge of the screen. + // Expects to move, because although onscreen, callout would not be. + // However, if the scale is 0.25, then expectToPan should be NO, because + // of the width of the annotation + // + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + { {kAnnotationScale, 0.5f}, YES, YES, YES, (kAnnotationScale == 0.125f), YES }, + + PAN_TEST_TERMINATOR + }; + + [self internalRunTests:tests]; +} + +- (void)testSelectionMoveIntoViewWithBasicCallout { + // Tests moveIntoView:YES + // WITH a callout that DOES NOT implement marginshint + + // From https://github.com/mapbox/mapbox-gl-native/pull/13727#issuecomment-454028698 + // + // | Annotation position | Has callout? | Callout implements `marginInsets...`? | Map pans when selected with moveIntoView=YES? | + // |---------------------|--------------|---------------------------------------|-----------------------------------------------| + // | Offscreen | Yes | No | Yes, but only to show annotation (not callout) with no margins | + // | Partially | Yes | No | No | + // | Onscreen | Yes | No | No | + // + + PanTestData tests[] = { + // Coord showsCallout impl margins? moveIntoView expectMapToPan calloutOnScreen + // Offscreen + { {-1.0f, 0.5f}, YES, NO, YES, YES, NO }, + { { 2.0f, 0.5f}, YES, NO, YES, YES, NO }, + { { 0.5f,-1.0f}, YES, NO, YES, YES, NO }, + { { 0.5f, 2.0f}, YES, NO, YES, YES, YES }, // Because annotation was off the bottom of screen, and callout is above annotation + { { 2.0f, 2.0f}, YES, NO, YES, YES, NO }, + + // Partial + { { 0.0f, 0.5f}, YES, NO, YES, NO, NO }, + { { 1.0f, 0.5f}, YES, NO, YES, NO, NO }, + { { 0.5f, 0.0f}, YES, NO, YES, NO, NO }, + { { 0.5f, 1.0f}, YES, NO, YES, NO, YES }, // Because annotation was off the bottom of screen, and callout is above annotation + { { 1.0f, 1.0f}, YES, NO, YES, NO, NO }, + + // Onscreen + { { 0.5f, 0.5f}, YES, NO, YES, NO, YES }, + + PAN_TEST_TERMINATOR + }; + + [self internalRunTests:tests]; +} + +#pragma mark - Selection with an offset + - (void)testSelectingAnnotationWithCenterOffset { for (CGFloat dx = -100.0; dx <= 100.0; dx += 100.0 ) { @@ -66,8 +445,8 @@ // Check that the annotation is in the center of the view CGPoint annotationPoint = [self.mapView convertCoordinate:point.coordinate toPointToView:self.mapView]; - XCTAssertEqualWithAccuracy(annotationPoint.x, size.width/2.0, epsilon); - XCTAssertEqualWithAccuracy(annotationPoint.y, size.height/2.0, epsilon); + XCTAssertEqualWithAccuracy(annotationPoint.x, size.width/2.0, 0.25); + XCTAssertEqualWithAccuracy(annotationPoint.y, size.height/2.0, 0.25); // Now test taps around the annotation CGPoint tapPoint = CGPointMake(annotationPoint.x + offset.dx, annotationPoint.y + offset.dy); @@ -117,6 +496,30 @@ XCTAssertEqual(originalFrame.origin.y + offset.y, offsetFrame.origin.y); } +#pragma mark - Utilities + +- (void)runRunLoop { + [[NSRunLoop mainRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantFuture]]; +} + +- (void)waitFor:(NSTimeInterval)seconds { + XCTestExpectation *timerExpired = [self expectationWithDescription:@"Timer expires"]; + + NSTimer *timer = [NSTimer scheduledTimerWithTimeInterval:0.1 + target:self + selector:@selector(runRunLoop) + userInfo:nil + repeats:YES]; + + double duration = seconds * (double)NSEC_PER_SEC; + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)duration), dispatch_get_main_queue(), ^{ + [timerExpired fulfill]; + }); + + [self waitForExpectations:@[timerExpired] timeout:seconds + 1.0]; + [timer invalidate]; +} + - (void)waitForCollisionDetectionToRun { XCTAssertNil(self.renderFinishedExpectation, @"Incorrect test setup"); @@ -129,6 +532,8 @@ }); [self waitForExpectations:@[timerExpired, self.renderFinishedExpectation] timeout:5]; + + self.renderFinishedExpectation = nil; } @end diff --git a/platform/ios/Integration Tests/MGLMapViewIntegrationTest.h b/platform/ios/Integration Tests/MGLMapViewIntegrationTest.h index 2f459a5f44..dedafdb83a 100644 --- a/platform/ios/Integration Tests/MGLMapViewIntegrationTest.h +++ b/platform/ios/Integration Tests/MGLMapViewIntegrationTest.h @@ -16,6 +16,13 @@ #define MGLTestAssertNotNil(myself, expression, ...) \ _XCTPrimitiveAssertNotNil(myself, expression, @#expression, __VA_ARGS__) +#define MGLTestWarning(expression, format, ...) \ +({ \ + if (!(expression)) { \ + NSString *message = [NSString stringWithFormat:format, ##__VA_ARGS__]; \ + printf("warning: Test Case '%s' at line %d: '%s' %s\n", __PRETTY_FUNCTION__, __LINE__, #expression, message.UTF8String); \ + } \ +}) @interface MGLMapViewIntegrationTest : XCTestCase @property (nonatomic) MGLMapView *mapView; @@ -27,6 +34,8 @@ @property (nonatomic) void (^regionIsChanging)(MGLMapView *mapView); @property (nonatomic) void (^regionDidChange)(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated); @property (nonatomic) CGPoint (^mapViewUserLocationAnchorPoint)(MGLMapView *mapView); +@property (nonatomic) BOOL (^mapViewAnnotationCanShowCalloutForAnnotation)(MGLMapView *mapView, id annotation); +@property (nonatomic) id (^mapViewCalloutViewForAnnotation)(MGLMapView *mapView, id annotation); // Utility methods - (NSString*)validAccessToken; diff --git a/platform/ios/Integration Tests/MGLMapViewIntegrationTest.m b/platform/ios/Integration Tests/MGLMapViewIntegrationTest.m index 0138a255ec..77e678d3ef 100644 --- a/platform/ios/Integration Tests/MGLMapViewIntegrationTest.m +++ b/platform/ios/Integration Tests/MGLMapViewIntegrationTest.m @@ -108,6 +108,20 @@ return CGPointZero; } +- (BOOL)mapView:(MGLMapView *)mapView annotationCanShowCallout:(id)annotation { + if (self.mapViewAnnotationCanShowCalloutForAnnotation) { + return self.mapViewAnnotationCanShowCalloutForAnnotation(mapView, annotation); + } + return NO; +} + +- (id)mapView:(MGLMapView *)mapView calloutViewForAnnotation:(id)annotation { + if (self.mapViewCalloutViewForAnnotation) { + return self.mapViewCalloutViewForAnnotation(mapView, annotation); + } + return nil; +} + #pragma mark - Utilities - (void)waitForMapViewToFinishLoadingStyleWithTimeout:(NSTimeInterval)timeout { diff --git a/platform/ios/ios.xcodeproj/project.pbxproj b/platform/ios/ios.xcodeproj/project.pbxproj index 2c4d7f2eab..0945d08cc8 100644 --- a/platform/ios/ios.xcodeproj/project.pbxproj +++ b/platform/ios/ios.xcodeproj/project.pbxproj @@ -474,6 +474,8 @@ CA0C27942076CA19001CE5B7 /* MGLMapViewIntegrationTest.m in Sources */ = {isa = PBXBuildFile; fileRef = CA0C27932076CA19001CE5B7 /* MGLMapViewIntegrationTest.m */; }; CA1B4A512099FB2200EDD491 /* MGLMapSnapshotterTest.m in Sources */ = {isa = PBXBuildFile; fileRef = CA1B4A502099FB2200EDD491 /* MGLMapSnapshotterTest.m */; }; CA34C9C3207FD272005C1A06 /* MGLCameraTransitionTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = CA34C9C2207FD272005C1A06 /* MGLCameraTransitionTests.mm */; }; + CA45105A21EE2989000C37D5 /* MGLCompactCalloutView.m in Sources */ = {isa = PBXBuildFile; fileRef = DA8848451CBAFB9800AB86E3 /* MGLCompactCalloutView.m */; }; + CA45105B21EE29B3000C37D5 /* SMCalloutView.m in Sources */ = {isa = PBXBuildFile; fileRef = DA88488A1CBB037E00AB86E3 /* SMCalloutView.m */; }; CA4EB8C720863487006AB465 /* MGLStyleLayerIntegrationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CA4EB8C620863487006AB465 /* MGLStyleLayerIntegrationTests.m */; }; CA55CD41202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */ = {isa = PBXBuildFile; fileRef = CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */; settings = {ATTRIBUTES = (Public, ); }; }; CA55CD42202C16AA00CE7095 /* MGLCameraChangeReason.h in Headers */ = {isa = PBXBuildFile; fileRef = CA55CD3E202C16AA00CE7095 /* MGLCameraChangeReason.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -3034,10 +3036,12 @@ buildActionMask = 2147483647; files = ( CA4EB8C720863487006AB465 /* MGLStyleLayerIntegrationTests.m in Sources */, + CA45105B21EE29B3000C37D5 /* SMCalloutView.m in Sources */, CA34C9C3207FD272005C1A06 /* MGLCameraTransitionTests.mm in Sources */, 16376B0A1FFD9DAF0000563E /* MBGLIntegrationTests.m in Sources */, CA88DC3021C85D900059ED5A /* MGLStyleURLIntegrationTest.m in Sources */, CA0C27942076CA19001CE5B7 /* MGLMapViewIntegrationTest.m in Sources */, + CA45105A21EE2989000C37D5 /* MGLCompactCalloutView.m in Sources */, CAE7AD5520F46EF5003B6782 /* MGLMapSnapshotterSwiftTests.swift in Sources */, CA0C27922076C804001CE5B7 /* MGLShapeSourceTests.m in Sources */, 077061DA215DA00E000FEF62 /* MGLTestLocationManager.m in Sources */, diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index d188ffb91f..258f6eea87 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -152,9 +152,6 @@ const CGFloat MGLAnnotationImagePaddingForCallout = 1; const CGSize MGLAnnotationAccessibilityElementMinimumSize = CGSizeMake(10, 10); -/// Padding to edge of view that an offscreen annotation must have when being brought onscreen (by being selected) -const UIEdgeInsets MGLMapViewOffscreenAnnotationPadding = UIEdgeInsetsMake(-20.0f, -20.0f, -20.0f, -20.0f); - /// An indication that the requested annotation was not found or is nonexistent. enum { MGLAnnotationTagNotFound = UINT32_MAX }; @@ -229,6 +226,11 @@ public: /// Currently shown popover representing the selected annotation. @property (nonatomic) UIView *calloutViewForSelectedAnnotation; + +/// Anchor coordinate from which to present callout views (for example, for shapes this +/// could be the touch point rather than its centroid) +@property (nonatomic) CLLocationCoordinate2D anchorCoordinateForSelectedAnnotation; + @property (nonatomic) MGLUserLocationAnnotationView *userLocationAnnotationView; /// Indicates how thoroughly the map view is tracking the user location. @@ -4537,7 +4539,7 @@ public: - (BOOL)isMovingAnnotationIntoViewSupportedForAnnotation:(id)annotation animated:(BOOL)animated { // Consider delegating - return animated && [annotation isKindOfClass:[MGLPointAnnotation class]]; + return [annotation isKindOfClass:[MGLPointAnnotation class]]; } - (id )selectedAnnotation @@ -4644,12 +4646,15 @@ public: CGPoint originPoint = [self convertCoordinate:origin toPointToView:self]; calloutPositioningRect = { .origin = originPoint, .size = CGSizeZero }; } - - CGRect expandedPositioningRect = UIEdgeInsetsInsetRect(calloutPositioningRect, MGLMapViewOffscreenAnnotationPadding); + + CGRect expandedPositioningRect = calloutPositioningRect; // Used for callout positioning, and moving offscreen annotations onscreen. - CGRect constrainedRect = UIEdgeInsetsInsetRect(self.bounds, self.contentInset); + CGRect constrainedRect = self.contentFrame; + CGRect bounds = constrainedRect; + BOOL expandedPositioningRectToMoveCalloutIntoViewWithMargins = NO; + UIView *calloutView = nil; if ([annotation respondsToSelector:@selector(title)] && @@ -4720,37 +4725,55 @@ public: if (moveIntoView && [calloutView respondsToSelector:@selector(marginInsetsHintForPresentationFromRect:)]) { UIEdgeInsets margins = [calloutView marginInsetsHintForPresentationFromRect:calloutPositioningRect]; expandedPositioningRect = UIEdgeInsetsInsetRect(expandedPositioningRect, margins); + expandedPositioningRectToMoveCalloutIntoViewWithMargins = YES; } } + + if (!expandedPositioningRectToMoveCalloutIntoViewWithMargins) + { + // We don't have a callout (OR our callout didn't implement + // marginInsetsHintForPresentationFromRect: - in this case we need to + // ensure that partially off-screen annotations are NOT moved into view. + // + // We may want to create (and fallback to) an `MGLMapViewDelegate` version + // of the `-[MGLCalloutView marginInsetsHintForPresentationFromRect:] + // protocol method. + bounds = CGRectInset(bounds, -calloutPositioningRect.size.width, -calloutPositioningRect.size.height); + } if (moveIntoView) { moveIntoView = NO; - // Need to consider the content insets. - CGRect bounds = UIEdgeInsetsInsetRect(self.bounds, self.contentInset); - // Any one of these cases should trigger a move onscreen - if (CGRectGetMinX(calloutPositioningRect) < CGRectGetMinX(bounds)) - { - constrainedRect.origin.x = expandedPositioningRect.origin.x; + CGFloat minX = CGRectGetMinX(expandedPositioningRect); + + if (minX < CGRectGetMinX(bounds)) { + constrainedRect.origin.x = minX; moveIntoView = YES; } - else if (CGRectGetMaxX(calloutPositioningRect) > CGRectGetMaxX(bounds)) - { - constrainedRect.origin.x = CGRectGetMaxX(expandedPositioningRect) - constrainedRect.size.width; - moveIntoView = YES; + else { + CGFloat maxX = CGRectGetMaxX(expandedPositioningRect); + + if (maxX > CGRectGetMaxX(bounds)) { + constrainedRect.origin.x = maxX - CGRectGetWidth(constrainedRect); + moveIntoView = YES; + } } - if (CGRectGetMinY(calloutPositioningRect) < CGRectGetMinY(bounds)) - { - constrainedRect.origin.y = expandedPositioningRect.origin.y; + CGFloat minY = CGRectGetMinY(expandedPositioningRect); + + if (minY < CGRectGetMinY(bounds)) { + constrainedRect.origin.y = minY; moveIntoView = YES; } - else if (CGRectGetMaxY(calloutPositioningRect) > CGRectGetMaxY(bounds)) - { - constrainedRect.origin.y = CGRectGetMaxY(expandedPositioningRect) - constrainedRect.size.height; - moveIntoView = YES; + else { + CGFloat maxY = CGRectGetMaxY(expandedPositioningRect); + + if (maxY > CGRectGetMaxY(bounds)) { + constrainedRect.origin.y = maxY - CGRectGetHeight(constrainedRect); + moveIntoView = YES; + } } } @@ -4760,6 +4783,17 @@ public: constrainedToRect:constrainedRect animated:animateSelection]; + // Save the anchor coordinate + if ([annotation isKindOfClass:[MGLPointAnnotation class]]) { + self.anchorCoordinateForSelectedAnnotation = annotation.coordinate; + } + else { + // This is used for features like polygons, so that if the map is dragged + // the callout doesn't ping to its coordinate. + CGPoint anchorPoint = CGPointMake(CGRectGetMidX(calloutPositioningRect), CGRectGetMidY(calloutPositioningRect)); + self.anchorCoordinateForSelectedAnnotation = [self convertPoint:anchorPoint toCoordinateFromView:self]; + } + // notify delegate if ([self.delegate respondsToSelector:@selector(mapView:didSelectAnnotation:)]) { @@ -4824,8 +4858,18 @@ public: return CGRectNull; } + CLLocationCoordinate2D coordinate; + + if ((annotation == self.selectedAnnotation) && + CLLocationCoordinate2DIsValid(self.anchorCoordinateForSelectedAnnotation)) { + coordinate = self.anchorCoordinateForSelectedAnnotation; + } + else { + coordinate = annotation.coordinate; + } + if ([annotation isKindOfClass:[MGLMultiPoint class]]) { - CLLocationCoordinate2D origin = annotation.coordinate; + CLLocationCoordinate2D origin = coordinate; CGPoint originPoint = [self convertCoordinate:origin toPointToView:self]; return CGRectMake(originPoint.x, originPoint.y, MGLAnnotationImagePaddingForHitTest, MGLAnnotationImagePaddingForHitTest); } @@ -4840,7 +4884,7 @@ public: return CGRectZero; } - CGRect positioningRect = [self frameOfImage:image centeredAtCoordinate:annotation.coordinate]; + CGRect positioningRect = [self frameOfImage:image centeredAtCoordinate:coordinate]; positioningRect.origin.x -= 0.5; return CGRectInset(positioningRect, -MGLAnnotationImagePaddingForCallout, @@ -4895,6 +4939,7 @@ public: // clean up self.calloutViewForSelectedAnnotation = nil; self.selectedAnnotation = nil; + self.anchorCoordinateForSelectedAnnotation = kCLLocationCoordinate2DInvalid; // notify delegate if ([self.delegate respondsToSelector:@selector(mapView:didDeselectAnnotation:)]) @@ -6158,7 +6203,9 @@ public: annotationView = self.userLocationAnnotationView; } - CGRect positioningRect = annotationView ? annotationView.frame : [self positioningRectForCalloutForAnnotationWithTag:tag]; + CGRect positioningRect = annotationView ? + annotationView.frame : + [self positioningRectForCalloutForAnnotationWithTag:tag]; MGLAssert( ! CGRectIsNull(positioningRect), @"Positioning rect should not be CGRectNull by this point"); diff --git a/platform/ios/vendor/SMCalloutView/SMCalloutView.m b/platform/ios/vendor/SMCalloutView/SMCalloutView.m index 7f987c3355..66a7b5a7a7 100755 --- a/platform/ios/vendor/SMCalloutView/SMCalloutView.m +++ b/platform/ios/vendor/SMCalloutView/SMCalloutView.m @@ -269,6 +269,8 @@ NSTimeInterval const kMGLSMCalloutViewRepositionDelayForUIScrollView = 1.0/3.0; - (UIEdgeInsets)marginInsetsHintForPresentationFromRect:(CGRect)rect { + const CGFloat defaultMargin = 20.0f; + // form our subviews based on our content set so far [self rebuildSubviews]; @@ -281,16 +283,16 @@ NSTimeInterval const kMGLSMCalloutViewRepositionDelayForUIScrollView = 1.0/3.0; CGFloat horizontalMargin = fmaxf(0, ceilf((CALLOUT_MIN_WIDTH-rect.size.width)/2)); UIEdgeInsets insets = { - .top = 0.0f, - .right = -horizontalMargin, + .top = 0.0f, + .right = -defaultMargin - horizontalMargin, .bottom = 0.0f, - .left = -horizontalMargin + .left = -defaultMargin - horizontalMargin }; if (self.permittedArrowDirection == MGLSMCalloutArrowDirectionUp) - insets.bottom -= size.height; + insets.bottom -= (defaultMargin + size.height); else - insets.top -= size.height; + insets.top -= (defaultMargin + size.height); return insets; } diff --git a/platform/macos/CHANGELOG.md b/platform/macos/CHANGELOG.md index fab8e972fc..d1930cb940 100644 --- a/platform/macos/CHANGELOG.md +++ b/platform/macos/CHANGELOG.md @@ -6,7 +6,7 @@ * Fixed a crash when casting large numbers in `NSExpression`. ([#13580](https://github.com/mapbox/mapbox-gl-native/pull/13580)) * Added the `-[MGLShapeSource leavesOfCluster:offset:limit:]`, `-[MGLShapeSource childrenOfCluster:]`, `-[MGLShapeSource zoomLevelForExpandingCluster:]` methods for inspecting a cluster in an `MGLShapeSource`s created with the `MGLShapeSourceOptionClustered` option. Feature querying now returns clusters represented by `MGLPointFeatureCluster` objects (that conform to the `MGLCluster` protocol). ([#12952](https://github.com/mapbox/mapbox-gl-native/pull/12952) * Fixed a bug with `MGLMapView.visibleAnnotations` that resulted in incorrect results and performance degradation. ([#13745](https://github.com/mapbox/mapbox-gl-native/pull/13745)) - +* Fixed a bug where selecting partially on-screen annotations (without a callout) would move the map. ([#13727](https://github.com/mapbox/mapbox-gl-native/pull/13727)) ## 0.13.0 - December 20, 2018 diff --git a/platform/macos/src/MGLMapView.mm b/platform/macos/src/MGLMapView.mm index a5779aa602..56e768584d 100644 --- a/platform/macos/src/MGLMapView.mm +++ b/platform/macos/src/MGLMapView.mm @@ -2281,7 +2281,7 @@ public: - (BOOL)isMovingAnnotationIntoViewSupportedForAnnotation:(id)annotation animated:(BOOL)animated { // Consider delegating - return animated && [annotation isKindOfClass:[MGLPointAnnotation class]]; + return [annotation isKindOfClass:[MGLPointAnnotation class]]; } - (void)selectAnnotation:(id )annotation @@ -2326,22 +2326,28 @@ public: positioningRect.origin = [self convertCoordinate:origin toPointToView:self]; } - if (!moveIntoView && NSIsEmptyRect(NSIntersectionRect(positioningRect, self.bounds))) { - if (!NSEqualPoints(gesturePoint, NSZeroPoint)) { + BOOL shouldShowCallout = ([annotation respondsToSelector:@selector(title)] + && annotation.title + && !self.calloutForSelectedAnnotation.shown + && [self.delegate respondsToSelector:@selector(mapView:annotationCanShowCallout:)] + && [self.delegate mapView:self annotationCanShowCallout:annotation]); + + if (NSIsEmptyRect(NSIntersectionRect(positioningRect, self.bounds))) { + if (!moveIntoView && !NSEqualPoints(gesturePoint, NSZeroPoint)) { positioningRect = CGRectMake(gesturePoint.x, gesturePoint.y, positioningRect.size.width, positioningRect.size.height); } } + // Onscreen or partially on-screen + else if (!shouldShowCallout) { + moveIntoView = NO; + } self.selectedAnnotation = annotation; // For the callout to be shown, the annotation must have a title, its // callout must not already be shown, and the annotation must be able to // show a callout according to the delegate. - if ([annotation respondsToSelector:@selector(title)] - && annotation.title - && !self.calloutForSelectedAnnotation.shown - && [self.delegate respondsToSelector:@selector(mapView:annotationCanShowCallout:)] - && [self.delegate mapView:self annotationCanShowCallout:annotation]) { + if (shouldShowCallout) { NSPopover *callout = [self calloutForAnnotation:annotation]; // Hang the callout off the right edge of the annotation image’s @@ -2365,39 +2371,52 @@ public: NSRect (^edgeInsetsInsetRect)(NSRect, NSEdgeInsets) = ^(NSRect rect, NSEdgeInsets insets) { return NSMakeRect(rect.origin.x + insets.left, - rect.origin.y + insets.top, + rect.origin.y + insets.bottom, rect.size.width - insets.left - insets.right, rect.size.height - insets.top - insets.bottom); }; // Add padding around the positioning rect (in essence an inset from the edge of the viewport - NSRect expandedPositioningRect = edgeInsetsInsetRect(positioningRect, MGLMapViewOffscreenAnnotationPadding); + NSRect expandedPositioningRect = positioningRect; + + if (shouldShowCallout) { + // If we have a callout, expand this rect to include a buffer + expandedPositioningRect = edgeInsetsInsetRect(positioningRect, MGLMapViewOffscreenAnnotationPadding); + } // Used for callout positioning, and moving offscreen annotations onscreen. CGRect constrainedRect = edgeInsetsInsetRect(self.bounds, self.contentInsets); CGRect bounds = constrainedRect; // Any one of these cases should trigger a move onscreen - if (CGRectGetMinX(positioningRect) < CGRectGetMinX(bounds)) - { - constrainedRect.origin.x = expandedPositioningRect.origin.x; + CGFloat minX = CGRectGetMinX(expandedPositioningRect); + + if (minX < CGRectGetMinX(bounds)) { + constrainedRect.origin.x = minX; moveIntoView = YES; } - else if (CGRectGetMaxX(positioningRect) > CGRectGetMaxX(bounds)) - { - constrainedRect.origin.x = CGRectGetMaxX(expandedPositioningRect) - constrainedRect.size.width; - moveIntoView = YES; + else { + CGFloat maxX = CGRectGetMaxX(expandedPositioningRect); + + if (maxX > CGRectGetMaxX(bounds)) { + constrainedRect.origin.x = maxX - CGRectGetWidth(constrainedRect); + moveIntoView = YES; + } } - - if (CGRectGetMinY(positioningRect) < CGRectGetMinY(bounds)) - { - constrainedRect.origin.y = expandedPositioningRect.origin.y; + + CGFloat minY = CGRectGetMinY(expandedPositioningRect); + + if (minY < CGRectGetMinY(bounds)) { + constrainedRect.origin.y = minY; moveIntoView = YES; } - else if (CGRectGetMaxY(positioningRect) > CGRectGetMaxY(bounds)) - { - constrainedRect.origin.y = CGRectGetMaxY(expandedPositioningRect) - constrainedRect.size.height; - moveIntoView = YES; + else { + CGFloat maxY = CGRectGetMaxY(expandedPositioningRect); + + if (maxY > CGRectGetMaxY(bounds)) { + constrainedRect.origin.y = maxY - CGRectGetHeight(constrainedRect); + moveIntoView = YES; + } } if (moveIntoView) diff --git a/platform/macos/test/MGLAnnotationTests.m b/platform/macos/test/MGLAnnotationTests.m index 36a7aef9f0..9657ae0a44 100644 --- a/platform/macos/test/MGLAnnotationTests.m +++ b/platform/macos/test/MGLAnnotationTests.m @@ -11,7 +11,7 @@ - (void)setUp { [super setUp]; - _mapView = [[MGLMapView alloc] initWithFrame:CGRectMake(0, 0, 64, 64)]; + _mapView = [[MGLMapView alloc] initWithFrame:CGRectMake(0, 0, 256, 256)]; _mapView.delegate = self; } -- cgit v1.2.1