From 5b5549d644d026e093243b13e0770b9d34172ef9 Mon Sep 17 00:00:00 2001 From: Julian Rex Date: Tue, 24 Apr 2018 11:01:04 -0400 Subject: [ios, macos] Fix for camera movement when selecting visible annotations (#11731) --- .../xcshareddata/xcschemes/CI.xcscheme | 10 ++- platform/ios/src/MGLMapView.mm | 21 ++++- platform/ios/test/MGLAnnotationViewTests.m | 89 ++++++++++++++++++++++ platform/macos/macos.xcodeproj/project.pbxproj | 4 + platform/macos/src/MGLMapView.mm | 12 ++- platform/macos/test/MGLAnnotationTests.m | 52 +++++++++++++ 6 files changed, 182 insertions(+), 6 deletions(-) create mode 100644 platform/macos/test/MGLAnnotationTests.m diff --git a/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/CI.xcscheme b/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/CI.xcscheme index 4679378126..afba168676 100644 --- a/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/CI.xcscheme +++ b/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/CI.xcscheme @@ -54,7 +54,6 @@ buildConfiguration = "Debug" selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" - language = "" shouldUseLaunchSchemeArgsEnv = "YES"> + + + + + + @@ -84,7 +91,6 @@ buildConfiguration = "Debug" selectedDebuggerIdentifier = "Xcode.DebuggerFoundation.Debugger.LLDB" selectedLauncherIdentifier = "Xcode.DebuggerFoundation.Launcher.LLDB" - language = "" launchStyle = "0" useCustomWorkingDirectory = "NO" ignoresPersistentStateOnLaunch = "NO" diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index b7d0974872..c679979d37 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -4290,6 +4290,15 @@ public: moveOnscreen = [self isBringingAnnotationOnscreenSupportedForAnnotation:annotation animated:animateSelection]; } + // If we have an invalid positioning rect, we need to provide a suitable default. + // This (currently) happens if you select an annotation that has NOT yet been + // added. See https://github.com/mapbox/mapbox-gl-native/issues/11476 + if (CGRectIsNull(calloutPositioningRect)) { + CLLocationCoordinate2D origin = annotation.coordinate; + CGPoint originPoint = [self convertCoordinate:origin toPointToView:self]; + calloutPositioningRect = { .origin = originPoint, .size = CGSizeZero }; + } + CGRect expandedPositioningRect = UIEdgeInsetsInsetRect(calloutPositioningRect, MGLMapViewOffscreenAnnotationPadding); // Used for callout positioning, and moving offscreen annotations onscreen. @@ -4442,7 +4451,11 @@ public: { MGLAnnotationTag annotationTag = [self annotationTagForAnnotation:annotation]; CGRect positioningRect = [self positioningRectForCalloutForAnnotationWithTag:annotationTag]; - + + if (CGRectIsNull(positioningRect)) { + return positioningRect; + } + // For annotations which `coordinate` falls offscreen it will use the current tap point as anchor instead. if ( ! CGRectIntersectsRect(positioningRect, self.bounds) && annotation != self.userLocation) { @@ -4462,15 +4475,15 @@ public: id annotation = [self annotationWithTag:annotationTag]; if ( ! annotation) { - return CGRectZero; + return CGRectNull; } if ([annotation isKindOfClass:[MGLMultiPoint class]]) { CLLocationCoordinate2D origin = annotation.coordinate; CGPoint originPoint = [self convertCoordinate:origin toPointToView:self]; return CGRectMake(originPoint.x, originPoint.y, MGLAnnotationImagePaddingForHitTest, MGLAnnotationImagePaddingForHitTest); - } + UIImage *image = [self imageOfAnnotationWithTag:annotationTag].image; if ( ! image) { @@ -5743,6 +5756,8 @@ public: rect = annotationView.frame; } + NSAssert(!CGRectIsNull(rect), @"Positioning rect should not be CGRectNull by this point"); + CGPoint point = CGPointMake(CGRectGetMidX(rect), CGRectGetMinY(rect)); if ( ! CGPointEqualToPoint(calloutView.center, point)) { diff --git a/platform/ios/test/MGLAnnotationViewTests.m b/platform/ios/test/MGLAnnotationViewTests.m index 2f5963e66e..a2cc4227ed 100644 --- a/platform/ios/test/MGLAnnotationViewTests.m +++ b/platform/ios/test/MGLAnnotationViewTests.m @@ -3,6 +3,13 @@ static NSString * const MGLTestAnnotationReuseIdentifer = @"MGLTestAnnotationReuseIdentifer"; + +@interface MGLMapView (Tests) +@property (nonatomic) MGLCameraChangeReason cameraChangeReasonBitmask; +@end + + + @interface MGLCustomAnnotationView : MGLAnnotationView @end @@ -58,6 +65,7 @@ static NSString * const MGLTestAnnotationReuseIdentifer = @"MGLTestAnnotationReu @property (nonatomic) MGLMapView *mapView; @property (nonatomic, weak) MGLAnnotationView *annotationView; @property (nonatomic) NSInteger annotationSelectedCount; +@property (nonatomic) void (^prepareAnnotationView)(MGLAnnotationView*); @end @implementation MGLAnnotationViewTests @@ -152,6 +160,83 @@ static NSString * const MGLTestAnnotationReuseIdentifer = @"MGLTestAnnotationReu XCTAssertEqual(selectionCount, self.annotationSelectedCount, @"-mapView:didSelectAnnotation: should be called for each selection"); } +- (void)testSelectingOnscreenAnnotationThatHasNotBeenAdded { + // See https://github.com/mapbox/mapbox-gl-native/issues/11476 + + // This bug occurs under the following conditions: + // + // - There are content insets (e.g. navigation bar) for the compare against + // CGRectZero (now CGRectNull) + // - annotationView.enabled == NO - Currently this can happen if you use + // `-initWithFrame:` rather than one of the provided initializers + // + + self.prepareAnnotationView = ^(MGLAnnotationView *view) { + view.enabled = NO; + }; + + self.mapView.contentInset = UIEdgeInsetsMake(10.0, 10.0, 10.0, 10.0); + + MGLCameraChangeReason reasonBefore = self.mapView.cameraChangeReasonBitmask; + XCTAssert(reasonBefore == MGLCameraChangeReasonNone, @"Camera should not have moved at start of test"); + + // Create annotation + MGLPointFeature *point = [[MGLPointFeature alloc] init]; + point.title = NSStringFromSelector(_cmd); + point.coordinate = CLLocationCoordinate2DMake(0.0, 0.0); + + MGLCoordinateBounds coordinateBounds = [self.mapView convertRect:self.mapView.bounds toCoordinateBoundsFromView:self.mapView]; + XCTAssert(MGLCoordinateInCoordinateBounds(point.coordinate, coordinateBounds), @"The test point should be within the visible map view"); + + // Select on screen annotation (DO NOT ADD FIRST). + [self.mapView selectAnnotation:point animated:YES]; + + // Expect - the camera NOT to move. + MGLCameraChangeReason reasonAfter = self.mapView.cameraChangeReasonBitmask; + XCTAssert(reasonAfter == MGLCameraChangeReasonNone, @"Camera should not have moved"); +} + +- (void)checkDefaultPropertiesForAnnotationView:(MGLAnnotationView*)view { + XCTAssertNil(view.annotation); + XCTAssertNil(view.reuseIdentifier); + XCTAssertEqual(view.centerOffset.dx, 0.0); + XCTAssertEqual(view.centerOffset.dy, 0.0); + XCTAssertFalse(view.scalesWithViewingDistance); + XCTAssertFalse(view.rotatesToMatchCamera); + XCTAssertFalse(view.isSelected); + XCTAssert(view.isEnabled); + XCTAssertFalse(view.isDraggable); + XCTAssertEqual(view.dragState, MGLAnnotationViewDragStateNone); +} + +- (void)testAnnotationViewInitWithFrame { + CGRect frame = CGRectMake(10.0, 10.0, 100.0, 100.0); + MGLAnnotationView *view = [[MGLAnnotationView alloc] initWithFrame:frame]; + [self checkDefaultPropertiesForAnnotationView:view]; +} + +- (void)testAnnotationViewInitWithReuseIdentifier { + MGLAnnotationView *view = [[MGLAnnotationView alloc] initWithReuseIdentifier:nil]; + [self checkDefaultPropertiesForAnnotationView:view]; +} + +- (void)testSelectingADisabledAnnotationView { + self.prepareAnnotationView = ^(MGLAnnotationView *view) { + view.enabled = NO; + }; + + // Create annotation + MGLPointFeature *point = [[MGLPointFeature alloc] init]; + point.title = NSStringFromSelector(_cmd); + point.coordinate = CLLocationCoordinate2DMake(0.0, 0.0); + + XCTAssert(self.mapView.selectedAnnotations.count == 0, @"There should be 0 selected annotations"); + + [self.mapView selectAnnotation:point animated:NO]; + + XCTAssert(self.mapView.selectedAnnotations.count == 0, @"There should be 0 selected annotations"); +} + #pragma mark - MGLMapViewDelegate - - (MGLAnnotationView *)mapView:(MGLMapView *)mapView viewForAnnotation:(id)annotation @@ -163,6 +248,10 @@ static NSString * const MGLTestAnnotationReuseIdentifer = @"MGLTestAnnotationReu annotationView = [[MGLAnnotationView alloc] initWithAnnotation:annotation reuseIdentifier:MGLTestAnnotationReuseIdentifer]; } + if (self.prepareAnnotationView) { + self.prepareAnnotationView(annotationView); + } + _annotationView = annotationView; return annotationView; diff --git a/platform/macos/macos.xcodeproj/project.pbxproj b/platform/macos/macos.xcodeproj/project.pbxproj index d49e5e6f06..6a97794d85 100644 --- a/platform/macos/macos.xcodeproj/project.pbxproj +++ b/platform/macos/macos.xcodeproj/project.pbxproj @@ -95,6 +95,7 @@ 9654C12B1FFC38E000DB6A19 /* MGLPolyline_Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 9654C12A1FFC38E000DB6A19 /* MGLPolyline_Private.h */; }; 9654C12D1FFC394700DB6A19 /* MGLPolygon_Private.h in Headers */ = {isa = PBXBuildFile; fileRef = 9654C12C1FFC394700DB6A19 /* MGLPolygon_Private.h */; }; 96E027311E57C9A7004B8E66 /* Localizable.strings in Resources */ = {isa = PBXBuildFile; fileRef = 96E027331E57C9A7004B8E66 /* Localizable.strings */; }; + CA9461A620884CCB0015EB12 /* MGLAnnotationTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CA9461A520884CCB0015EB12 /* MGLAnnotationTests.m */; }; DA00FC8A1D5EEAC3009AABC8 /* MGLAttributionInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = DA00FC881D5EEAC3009AABC8 /* MGLAttributionInfo.h */; settings = {ATTRIBUTES = (Public, ); }; }; DA00FC8B1D5EEAC3009AABC8 /* MGLAttributionInfo.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA00FC891D5EEAC3009AABC8 /* MGLAttributionInfo.mm */; }; DA0CD58E1CF56F5800A5F5A5 /* MGLFeatureTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = DA0CD58D1CF56F5800A5F5A5 /* MGLFeatureTests.mm */; }; @@ -394,6 +395,7 @@ 96E027391E57C9B9004B8E66 /* sv */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = sv; path = sv.lproj/Localizable.strings; sourceTree = ""; }; 96E0273A1E57C9BB004B8E66 /* vi */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = vi; path = vi.lproj/Localizable.strings; sourceTree = ""; }; 96E0273B1E57C9BC004B8E66 /* pt-BR */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = "pt-BR"; path = "pt-BR.lproj/Localizable.strings"; sourceTree = ""; }; + CA9461A520884CCB0015EB12 /* MGLAnnotationTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; name = MGLAnnotationTests.m; path = test/MGLAnnotationTests.m; sourceTree = SOURCE_ROOT; }; DA00FC881D5EEAC3009AABC8 /* MGLAttributionInfo.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLAttributionInfo.h; sourceTree = ""; }; DA00FC891D5EEAC3009AABC8 /* MGLAttributionInfo.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MGLAttributionInfo.mm; sourceTree = ""; }; DA0CD58D1CF56F5800A5F5A5 /* MGLFeatureTests.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = MGLFeatureTests.mm; path = ../../darwin/test/MGLFeatureTests.mm; sourceTree = ""; }; @@ -1076,6 +1078,7 @@ 4031AD001E9FD61000A3EA26 /* Test Helpers */, 4031ACFA1E9EB39A00A3EA26 /* Swift Integration */, DA8F257D1D51C5F40010E6B5 /* Styling */, + CA9461A520884CCB0015EB12 /* MGLAnnotationTests.m */, DAEDC4311D6033F1000224FF /* MGLAttributionInfoTests.m */, DAEDC4361D606291000224FF /* MGLAttributionButtonTests.m */, DA35A2C11CCA9F4A00E826B2 /* MGLClockDirectionFormatterTests.m */, @@ -1601,6 +1604,7 @@ DAE6C3D31CC34C9900DB3429 /* MGLOfflinePackTests.m in Sources */, DA87A9A51DCACC5000810D09 /* MGLLineStyleLayerTests.mm in Sources */, DA87A9A31DCACC5000810D09 /* MGLRasterStyleLayerTests.mm in Sources */, + CA9461A620884CCB0015EB12 /* MGLAnnotationTests.m in Sources */, DA87A9991DC9D88400810D09 /* MGLTileSetTests.mm in Sources */, DA35A2A81CC9F41600E826B2 /* MGLCoordinateFormatterTests.m in Sources */, DAE7DEC41E24549F007505A6 /* MGLNSStringAdditionsTests.m in Sources */, diff --git a/platform/macos/src/MGLMapView.mm b/platform/macos/src/MGLMapView.mm index 9cab9a76da..9a9e19c363 100644 --- a/platform/macos/src/MGLMapView.mm +++ b/platform/macos/src/MGLMapView.mm @@ -2249,8 +2249,16 @@ public: // The annotation's anchor will bounce to the current click. NSRect positioningRect = [self positioningRectForCalloutForAnnotationWithTag:annotationTag]; + // Check for invalid (zero) positioning rect + if (NSEqualRects(positioningRect, NSZeroRect)) { + CLLocationCoordinate2D origin = annotation.coordinate; + positioningRect.origin = [self convertCoordinate:origin toPointToView:self]; + } + if (!moveOnscreen && NSIsEmptyRect(NSIntersectionRect(positioningRect, self.bounds))) { - positioningRect = CGRectMake(gesturePoint.x, gesturePoint.y, positioningRect.size.width, positioningRect.size.height); + if (!NSEqualPoints(gesturePoint, NSZeroPoint)) { + positioningRect = CGRectMake(gesturePoint.x, gesturePoint.y, positioningRect.size.width, positioningRect.size.height); + } } self.selectedAnnotation = annotation; @@ -2464,6 +2472,8 @@ public: if (callout) { NSRect rect = [self positioningRectForCalloutForAnnotationWithTag:_selectedAnnotationTag]; + NSAssert(!NSEqualRects(rect, NSZeroRect), @"Positioning rect should be non-zero"); + if (!NSIsEmptyRect(NSIntersectionRect(rect, self.bounds))) { // It's possible that the current callout hasn't been presented (since the original diff --git a/platform/macos/test/MGLAnnotationTests.m b/platform/macos/test/MGLAnnotationTests.m new file mode 100644 index 0000000000..36a7aef9f0 --- /dev/null +++ b/platform/macos/test/MGLAnnotationTests.m @@ -0,0 +1,52 @@ +#import +#import + +@interface MGLAnnotationTests : XCTestCase +@property (nonatomic) MGLMapView *mapView; +@property (nonatomic) BOOL centerCoordinateDidChange; +@end + +@implementation MGLAnnotationTests + +- (void)setUp +{ + [super setUp]; + _mapView = [[MGLMapView alloc] initWithFrame:CGRectMake(0, 0, 64, 64)]; + _mapView.delegate = self; +} + +- (void)testSelectingOnscreenAnnotationThatHasNotBeenAdded { + // See https://github.com/mapbox/mapbox-gl-native/issues/11476 + + // This bug occurs under the following conditions: + // + // - There are content insets (e.g. navigation bar) for the compare against + // NSZeroRect + + self.mapView.contentInsets = NSEdgeInsetsMake(10.0, 10.0, 10.0, 10.0); + + // Create annotation + MGLPointFeature *point = [[MGLPointFeature alloc] init]; + point.title = NSStringFromSelector(_cmd); + point.coordinate = CLLocationCoordinate2DMake(0.0, 0.0); + + MGLCoordinateBounds coordinateBounds = [self.mapView convertRect:self.mapView.bounds toCoordinateBoundsFromView:self.mapView]; + XCTAssert(MGLCoordinateInCoordinateBounds(point.coordinate, coordinateBounds), @"The test point should be within the visible map view"); + + [self.mapView addObserver:self forKeyPath:@"centerCoordinate" options:NSKeyValueObservingOptionNew context:_cmd]; + XCTAssertFalse(self.centerCoordinateDidChange, @"Center coordinate should not have changed at this point"); + + // Select on screen annotation (DO NOT ADD FIRST). + [self.mapView selectAnnotation:point]; + + XCTAssertFalse(self.centerCoordinateDidChange, @"Center coordinate should not have changed after selecting a visible annotation"); +} + +- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context { + if ((context == @selector(testSelectingOnscreenAnnotationThatHasNotBeenAdded)) && + (object == self.mapView)) { + self.centerCoordinateDidChange = YES; + } +} + +@end -- cgit v1.2.1