From e890af196c5c8a348c53fdf4158cba012d0f609e Mon Sep 17 00:00:00 2001 From: Lloyd Sheng Date: Wed, 13 Mar 2019 13:44:51 +0800 Subject: Fix issues found in review --- platform/ios/app/MBXOrnamentsViewController.m | 75 ++++++++++++------------- platform/ios/src/MGLMapView.h | 17 +++--- platform/ios/src/MGLMapView.mm | 80 +++++++++++++-------------- platform/ios/src/MGLScaleBar.mm | 12 ++-- platform/ios/test/MGLMapViewLayoutTests.m | 30 ++-------- platform/ios/test/MGLMapViewScaleBarTests.m | 4 +- 6 files changed, 98 insertions(+), 120 deletions(-) diff --git a/platform/ios/app/MBXOrnamentsViewController.m b/platform/ios/app/MBXOrnamentsViewController.m index 489b3062b6..45288af0ab 100644 --- a/platform/ios/app/MBXOrnamentsViewController.m +++ b/platform/ios/app/MBXOrnamentsViewController.m @@ -13,43 +13,44 @@ @implementation MBXOrnamentsViewController - (void)setCurrentPositionIndex:(NSInteger)currentPositionIndex { - NSArray *ornamentPositions = @[@[ - @(MGLOrnamentPositionTopLeft), - @(MGLOrnamentPositionTopRight), - @(MGLOrnamentPositionBottomRight), - @(MGLOrnamentPositionBottomLeft) - ], - @[ - @(MGLOrnamentPositionTopRight), - @(MGLOrnamentPositionBottomRight), - @(MGLOrnamentPositionBottomLeft), - @(MGLOrnamentPositionTopLeft) - ], - @[ - @(MGLOrnamentPositionBottomRight), - @(MGLOrnamentPositionBottomLeft), - @(MGLOrnamentPositionTopLeft), - @(MGLOrnamentPositionTopRight) - ], - @[ - @(MGLOrnamentPositionBottomLeft), - @(MGLOrnamentPositionTopLeft), - @(MGLOrnamentPositionTopRight), - @(MGLOrnamentPositionBottomRight) - ], - @[ - @(MGLOrnamentPositionTopLeft), - @(MGLOrnamentPositionTopRight), - @(MGLOrnamentPositionBottomRight), - @(MGLOrnamentPositionBottomLeft) - ], - ]; - NSArray *currentPosition = ornamentPositions[currentPositionIndex]; - self.mapView.scaleBarPosition = [currentPosition[0] integerValue]; - self.mapView.compassViewPosition = [currentPosition[1] integerValue]; - self.mapView.logoViewPosition = [currentPosition[2] integerValue]; - self.mapView.attributionButtonPosition = [currentPosition[3] integerValue]; - + MGLOrnamentPosition ornamentPositions[5][4] = { + { + MGLOrnamentPositionTopLeft, + MGLOrnamentPositionTopRight, + MGLOrnamentPositionBottomRight, + MGLOrnamentPositionBottomLeft + }, + { + MGLOrnamentPositionTopRight, + MGLOrnamentPositionBottomRight, + MGLOrnamentPositionBottomLeft, + MGLOrnamentPositionTopLeft + }, + { + MGLOrnamentPositionBottomRight, + MGLOrnamentPositionBottomLeft, + MGLOrnamentPositionTopLeft, + MGLOrnamentPositionTopRight + }, + { + MGLOrnamentPositionBottomLeft, + MGLOrnamentPositionTopLeft, + MGLOrnamentPositionTopRight, + MGLOrnamentPositionBottomRight + }, + { + MGLOrnamentPositionTopLeft, + MGLOrnamentPositionTopRight, + MGLOrnamentPositionBottomRight, + MGLOrnamentPositionBottomLeft + } + }; + MGLOrnamentPosition *currentPosition = ornamentPositions[currentPositionIndex]; + self.mapView.scaleBarPosition = currentPosition[0]; + self.mapView.compassViewPosition = currentPosition[1]; + self.mapView.logoViewPosition = currentPosition[2]; + self.mapView.attributionButtonPosition = currentPosition[3]; + _currentPositionIndex = currentPositionIndex; } diff --git a/platform/ios/src/MGLMapView.h b/platform/ios/src/MGLMapView.h index c554c9951e..3678f755ee 100644 --- a/platform/ios/src/MGLMapView.h +++ b/platform/ios/src/MGLMapView.h @@ -276,14 +276,14 @@ MGL_EXPORT IB_DESIGNABLE @property (nonatomic, readonly) UIView *scaleBar; /** - The postion of the scale bar. The default value is `MGLOrnamentPositionTopLeft`. + The position of the scale bar. The default value is `MGLOrnamentPositionTopLeft`. */ @property (nonatomic, assign) MGLOrnamentPosition scaleBarPosition; /** A `CGPoint` indicating the position offset of the scale bar. */ -@property (nonatomic, assign) CGPoint scaleBarOffset; +@property (nonatomic, assign) CGPoint scaleBarMargins; /** A control indicating the map’s direction and allowing the user to manipulate @@ -292,14 +292,14 @@ MGL_EXPORT IB_DESIGNABLE @property (nonatomic, readonly) UIImageView *compassView; /** - The postion of the compass view. The default value is `MGLOrnamentPositionTopRight`. + The position of the compass view. The default value is `MGLOrnamentPositionTopRight`. */ @property (nonatomic, assign) MGLOrnamentPosition compassViewPosition; /** A `CGPoint` indicating the position offset of the compass. */ -@property (nonatomic, assign) CGPoint compassViewOffset; +@property (nonatomic, assign) CGPoint compassViewMargins; /** The Mapbox logo, positioned in the lower-left corner. @@ -313,15 +313,14 @@ MGL_EXPORT IB_DESIGNABLE @property (nonatomic, readonly) UIImageView *logoView; /** - The postion of the logo view. The default value is `MGLOrnamentPositionBottomLeft`. + The position of the logo view. The default value is `MGLOrnamentPositionBottomLeft`. */ @property (nonatomic, assign) MGLOrnamentPosition logoViewPosition; /** A `CGPoint` indicating the position offset of the logo. */ -@property (nonatomic, assign) CGPoint logoViewOffset; - +@property (nonatomic, assign) CGPoint logoViewMargins; /** A view showing legally required copyright notices and telemetry settings, @@ -349,14 +348,14 @@ MGL_EXPORT IB_DESIGNABLE @property (nonatomic, readonly) UIButton *attributionButton; /** - The postion of the attribution button. The default value is `MGLOrnamentPositionBottomRight`. + The position of the attribution button. The default value is `MGLOrnamentPositionBottomRight`. */ @property (nonatomic, assign) MGLOrnamentPosition attributionButtonPosition; /** A `CGPoint` indicating the position offset of the attribution. */ -@property (nonatomic, assign) CGPoint attributionButtonOffset; +@property (nonatomic, assign) CGPoint attributionButtonMargins; /** Show the attribution and telemetry action sheet. diff --git a/platform/ios/src/MGLMapView.mm b/platform/ios/src/MGLMapView.mm index 08007b5b6d..3a786e54d0 100644 --- a/platform/ios/src/MGLMapView.mm +++ b/platform/ios/src/MGLMapView.mm @@ -96,7 +96,7 @@ const MGLExceptionName MGLMissingLocationServicesUsageDescriptionException = @"M const MGLExceptionName MGLUserLocationAnnotationTypeException = @"MGLUserLocationAnnotationTypeException"; const MGLExceptionName MGLResourceNotFoundException = @"MGLResourceNotFoundException"; -const CGPoint MGLDefaultOrnamentPositionOffset = CGPointMake(8, 8); +const CGPoint MGLOrnamentDefaultPositionOffset = CGPointMake(8, 8); /// Indicates the manner in which the map view is tracking the user location. typedef NS_ENUM(NSUInteger, MGLUserTrackingState) { @@ -482,7 +482,7 @@ public: [self addSubview:_logoView]; _logoViewConstraints = [NSMutableArray array]; _logoViewPosition = MGLOrnamentPositionBottomLeft; - _logoViewOffset = MGLDefaultOrnamentPositionOffset; + _logoViewMargins = MGLOrnamentDefaultPositionOffset; // setup attribution // @@ -498,7 +498,7 @@ public: UILongPressGestureRecognizer *attributionLongPress = [[UILongPressGestureRecognizer alloc] initWithTarget:self action:@selector(showAttribution:)]; [_attributionButton addGestureRecognizer:attributionLongPress]; _attributionButtonPosition = MGLOrnamentPositionBottomRight; - _attributionButtonOffset = MGLDefaultOrnamentPositionOffset; + _attributionButtonMargins = MGLOrnamentDefaultPositionOffset; // setup compass // @@ -513,7 +513,7 @@ public: [self addSubview:_compassView]; _compassViewConstraints = [NSMutableArray array]; _compassViewPosition = MGLOrnamentPositionTopRight; - _compassViewOffset = MGLDefaultOrnamentPositionOffset; + _compassViewMargins = MGLOrnamentDefaultPositionOffset; // setup scale control // @@ -522,7 +522,7 @@ public: [self addSubview:_scaleBar]; _scaleBarConstraints = [NSMutableArray array]; _scaleBarPosition = MGLOrnamentPositionTopLeft; - _scaleBarOffset = MGLDefaultOrnamentPositionOffset; + _scaleBarMargins = MGLOrnamentDefaultPositionOffset; [self installConstraints]; @@ -774,9 +774,8 @@ public: [self installScaleBarConstraints]; } -- (void)setScaleBarOffset:(CGPoint)scaleBarOffset { - NSAssert(scaleBarOffset.x >= 0 && scaleBarOffset.y >= 0, @"The position offset of the scale bar should not be negative."); - _scaleBarOffset = scaleBarOffset; +- (void)setScaleBarMargins:(CGPoint)scaleBarOffset { + _scaleBarMargins = scaleBarOffset; [self installScaleBarConstraints]; } @@ -785,9 +784,8 @@ public: [self installCompassViewConstraints]; } -- (void)setCompassViewOffset:(CGPoint)compassViewOffset { - NSAssert(compassViewOffset.x >= 0 && compassViewOffset.y >= 0, @"The position offset of the compass should not be negative."); - _compassViewOffset = compassViewOffset; +- (void)setCompassViewMargins:(CGPoint)compassViewOffset { + _compassViewMargins = compassViewOffset; [self installCompassViewConstraints]; } @@ -796,9 +794,8 @@ public: [self installLogoViewConstraints]; } -- (void)setLogoViewOffset:(CGPoint)logoViewOffset { - NSAssert(logoViewOffset.x >= 0 && logoViewOffset.y >= 0, @"The position offset of the logo should not be negative."); - _logoViewOffset = logoViewOffset; +- (void)setLogoViewMargins:(CGPoint)logoViewOffset { + _logoViewMargins = logoViewOffset; [self installLogoViewConstraints]; } @@ -807,9 +804,8 @@ public: [self installAttributionButtonConstraints]; } -- (void)setAttributionButtonOffset:(CGPoint)attributionButtonOffset { - NSAssert(attributionButtonOffset.x >= 0 && attributionButtonOffset.y >= 0, @"The position offset of the attribution should not be negative."); - _attributionButtonOffset = attributionButtonOffset; +- (void)setAttributionButtonMargins:(CGPoint)attributionButtonOffset { + _attributionButtonMargins = attributionButtonOffset; [self installAttributionButtonConstraints]; } @@ -831,11 +827,11 @@ public: return nil; } -- (void)installConstraintsWithView:(UIView *)view - constraints:(NSMutableArray *)constraints - position:(MGLOrnamentPosition)position - size:(CGSize)size - offset:(CGPoint)offset { +- (void)updateConstraintsForOrnament:(UIView *)view + constraints:(NSMutableArray *)constraints + position:(MGLOrnamentPosition)position + size:(CGSize)size + offset:(CGPoint)offset { NSMutableArray *updatedConstraints = [NSMutableArray array]; if (@available(iOS 11.0, *)) { @@ -1032,38 +1028,38 @@ public: - (void)installCompassViewConstraints { // compass view - [self installConstraintsWithView:self.compassView - constraints:self.compassViewConstraints - position:self.compassViewPosition - size:self.compassView.bounds.size - offset:self.compassViewOffset]; + [self updateConstraintsForOrnament:self.compassView + constraints:self.compassViewConstraints + position:self.compassViewPosition + size:self.compassView.bounds.size + offset:self.compassViewMargins]; } - (void)installScaleBarConstraints { // scale bar view - [self installConstraintsWithView:self.scaleBar - constraints:self.scaleBarConstraints - position:self.scaleBarPosition - size:self.scaleBar.intrinsicContentSize - offset:self.scaleBarOffset]; + [self updateConstraintsForOrnament:self.scaleBar + constraints:self.scaleBarConstraints + position:self.scaleBarPosition + size:self.scaleBar.intrinsicContentSize + offset:self.scaleBarMargins]; } - (void)installLogoViewConstraints { // logo view - [self installConstraintsWithView:self.logoView - constraints:self.logoViewConstraints - position:self.logoViewPosition - size:self.logoView.bounds.size - offset:self.logoViewOffset]; + [self updateConstraintsForOrnament:self.logoView + constraints:self.logoViewConstraints + position:self.logoViewPosition + size:self.logoView.bounds.size + offset:self.logoViewMargins]; } - (void)installAttributionButtonConstraints { // attribution button - [self installConstraintsWithView:self.attributionButton - constraints:self.attributionButtonConstraints - position:self.attributionButtonPosition - size:self.attributionButton.bounds.size - offset:self.attributionButtonOffset]; + [self updateConstraintsForOrnament:self.attributionButton + constraints:self.attributionButtonConstraints + position:self.attributionButtonPosition + size:self.attributionButton.bounds.size + offset:self.attributionButtonMargins]; } - (BOOL)isOpaque diff --git a/platform/ios/src/MGLScaleBar.mm b/platform/ios/src/MGLScaleBar.mm index f34d0a44e7..c39fce8ce6 100644 --- a/platform/ios/src/MGLScaleBar.mm +++ b/platform/ios/src/MGLScaleBar.mm @@ -185,7 +185,7 @@ static const CGFloat MGLFeetPerMeter = 3.28084; #pragma mark - Dimensions - (CGSize)intrinsicContentSize { - return CGSizeMake(self.actualWidth + self.lastLabelWidth/2, 16); + return self.actualWidth > 0 ? CGSizeMake(ceil(self.actualWidth + self.lastLabelWidth/2), 16) : CGSizeZero; } - (CGFloat)actualWidth { @@ -260,9 +260,9 @@ static const CGFloat MGLFeetPerMeter = 3.28084; [self updateVisibility]; self.row = [self preferredRow]; - - CGSize size = self.intrinsicContentSize; - self.bounds = CGRectMake(0, 0, size.width, size.height); + + [self invalidateIntrinsicContentSize]; + [self setNeedsLayout]; } - (void)updateVisibility { @@ -412,7 +412,7 @@ static const CGFloat MGLFeetPerMeter = 3.28084; } self.containerView.frame = CGRectMake(CGRectGetMinX(self.bars.firstObject.frame), - CGRectGetMaxY(self.bounds)-MGLBarHeight, + self.intrinsicContentSize.height-MGLBarHeight, self.actualWidth, MGLBarHeight+self.borderWidth*2); @@ -429,7 +429,7 @@ static const CGFloat MGLFeetPerMeter = 3.28084; NSUInteger i = RTL ? self.bars.count : 0; for (UIView *label in self.labelViews) { CGFloat xPosition = round(barWidth * i - CGRectGetMidX(label.bounds) + self.borderWidth); - CGFloat yPosition = round(0.5 * (CGRectGetMaxY(self.bounds) - MGLBarHeight)); + CGFloat yPosition = round(0.5 * (self.intrinsicContentSize.height - MGLBarHeight)); CGRect frame = label.frame; frame.origin.x = xPosition; diff --git a/platform/ios/test/MGLMapViewLayoutTests.m b/platform/ios/test/MGLMapViewLayoutTests.m index 753a409a74..d902665768 100644 --- a/platform/ios/test/MGLMapViewLayoutTests.m +++ b/platform/ios/test/MGLMapViewLayoutTests.m @@ -7,7 +7,7 @@ @interface MGLOrnamentTestData : NSObject @property (nonatomic) MGLOrnamentPosition position; -@property (nonatomic) CGPoint offset; +@property (nonatomic) CGPoint margins; @property (nonatomic) CGPoint expectedOrigin; @end @@ -17,7 +17,7 @@ + (instancetype)createWithPostion:(MGLOrnamentPosition)position offset:(CGPoint)offset expectedOrigin:(CGPoint)expectedOrigin { MGLOrnamentTestData *data = [[MGLOrnamentTestData alloc] init]; data.position = position; - data.offset = offset; + data.margins = offset; data.expectedOrigin = expectedOrigin; return data; } @@ -127,24 +127,6 @@ XCTAssertEqualWithAccuracy(CGRectGetMinY(logoView.frame), expectedLogoOriginY, accuracy); } -- (void)testOrnamentPlacementInvalidArgument { - XCTAssertThrows([self.mapView setCompassViewOffset:CGPointMake(-4, -4)]); - XCTAssertThrows([self.mapView setCompassViewOffset:CGPointMake(-4, 0)]); - XCTAssertThrows([self.mapView setCompassViewOffset:CGPointMake(0, -4)]); - - XCTAssertThrows([self.mapView setScaleBarOffset:CGPointMake(-4, -4)]); - XCTAssertThrows([self.mapView setScaleBarOffset:CGPointMake(-4, 0)]); - XCTAssertThrows([self.mapView setScaleBarOffset:CGPointMake(0, -4)]); - - XCTAssertThrows([self.mapView setAttributionButtonOffset:CGPointMake(-4, -4)]); - XCTAssertThrows([self.mapView setAttributionButtonOffset:CGPointMake(-4, 0)]); - XCTAssertThrows([self.mapView setAttributionButtonOffset:CGPointMake(0, -4)]); - - XCTAssertThrows([self.mapView setLogoViewOffset:CGPointMake(-4, -4)]); - XCTAssertThrows([self.mapView setLogoViewOffset:CGPointMake(-4, 0)]); - XCTAssertThrows([self.mapView setLogoViewOffset:CGPointMake(0, -4)]); -} - - (NSArray *)makeTestDataListWithView:(UIView *)view margin:(CGFloat)margin { CGFloat bottomSafeAreaInset = 0.0; if (@available(iOS 11.0, *)) { @@ -177,7 +159,7 @@ for (MGLOrnamentTestData *testData in testDataList) { self.mapView.compassViewPosition = testData.position; - self.mapView.compassViewOffset = testData.offset; + self.mapView.compassViewMargins = testData.margins; //invoke layout [self.superView setNeedsLayout]; @@ -202,7 +184,7 @@ for (MGLOrnamentTestData *testData in testDataList) { self.mapView.scaleBarPosition = testData.position; - self.mapView.scaleBarOffset = testData.offset; + self.mapView.scaleBarMargins = testData.margins; //invoke layout [self.superView setNeedsLayout]; @@ -227,7 +209,7 @@ for (MGLOrnamentTestData *testData in testDataList) { self.mapView.attributionButtonPosition = testData.position; - self.mapView.attributionButtonOffset = testData.offset; + self.mapView.attributionButtonMargins = testData.margins; //invoke layout [self.superView setNeedsLayout]; @@ -252,7 +234,7 @@ for (MGLOrnamentTestData *testData in testDataList) { self.mapView.logoViewPosition = testData.position; - self.mapView.logoViewOffset = testData.offset; + self.mapView.logoViewMargins = testData.margins; //invoke layout [self.superView setNeedsLayout]; diff --git a/platform/ios/test/MGLMapViewScaleBarTests.m b/platform/ios/test/MGLMapViewScaleBarTests.m index 11d1187263..50864beb4d 100644 --- a/platform/ios/test/MGLMapViewScaleBarTests.m +++ b/platform/ios/test/MGLMapViewScaleBarTests.m @@ -35,7 +35,7 @@ XCTAssertFalse(scaleBar.hidden); // Scale bar should not be visible at default zoom (~z0), but it should be ready. - XCTAssertFalse(CGRectIsEmpty(scaleBar.frame)); + XCTAssertFalse(CGSizeEqualToSize(scaleBar.intrinsicContentSize, CGSizeZero)); XCTAssertEqual(scaleBar.alpha, 0); self.mapView.zoomLevel = 15; @@ -49,7 +49,7 @@ XCTAssertFalse(scaleBar.hidden); // Directly setting `.hidden` after the map has finished initializing will not update the scale bar. - XCTAssertTrue(CGRectIsEmpty(scaleBar.frame)); + XCTAssertTrue(CGSizeEqualToSize(scaleBar.intrinsicContentSize, CGSizeZero)); // ... but triggering any camera event will update it. self.mapView.zoomLevel = 1; -- cgit v1.2.1