[ios] Fixes an issue that caused the ornaments ignore contentInset property. #15584

fabian-guerra · 2019-09-07T03:23:57Z

This PR Fixes the following issues:

#14827 due to the fact that each bug fix adds its corresponding tests.
#12143 Removes the dependency of the now deprecated UIViewController. automaticallyAdjustsScrollViewInsets.

Tasks:

Fixes ornaments placement when automaticallyAdjustContentInset is set to YES and the contentInset is different from the safeArea.
The property automaticallyAdjustsScrollViewInsets it's been deprecated and moved to UIScrollView the fix I made adds automaticallyAdjustContentInset as part of the mapView that will control this behavior. If set to YES the contentInset will be equal to the view's safeArea (on iOS 11+) or top/bottom layout guides (iOS < 11) if set to NO it will use any value as long as it's not negative.
Fix an issue with the camera persisting edgePadding.

astojilj

Comments to reconsider related to API design. Tests looks good.

Fixes #12827

Is the reference wrong?

One more thing to reconsider:
If making new property, why not making it align more to what we have in Android. Following the frequent iOS API changes related to content inset and trying to mimic it in our SDK turned out to be an expensive maintenance effort. How about deprecating current in favour of common API with Android?

astojilj · 2019-09-09T06:38:01Z

platform/ios/src/MGLMapView.h

+ The default value of this property is `YES`.
+
+ */
+@property (assign) BOOL automaticallyAdjustContentInset;


Let's reconsider using existing contentInsetAdjustmentBehavior instead of adding new property.

@property(nonatomic,assign) BOOL automaticallyAdjustsScrollViewInsets API_DEPRECATED("Use UIScrollView's contentInsetAdjustmentBehavior instead", ios(7.0,11.0),tvos(7.0,11.0)); // Defaults to YES

Yes, please — this would address #12143. Note that contentInsetAdjustmentBehavior is iOS 11+, so we will still need to support both.

@astojilj @friedbunny automaticallyAdjustsScrollViewInsets is a property that belongs to a controller which all views are added to. If we use the UIScrollViews version then we will consider that property only if the root view of a mapView is a scrollView which may not be the case.

It is a problem using an Apple SDK property to control the behavior of the map's sdk because we can't add a proper deprecation notice for it.

In my comment https://github.com/mapbox/mapbox-gl-native/pull/15584/files#r322363031 I stated why using the API described in https://github.com/mapbox/mapbox-gl-native/pull/15584/files#r322082400 is not feasible.

I am keeping the property, and supporting the previous automaticallyAdjustsScrollViewInsets

Rename this property to automaticallyAdjustsContentInset, so it looks like a property rather than an action method.

The new UIScrollView property’s type is an enumeration rather than a Boolean. Do we anticipate adding more possible values in the future?

The new UIScrollView property’s type is an enumeration rather than a Boolean. Do we anticipate adding more possible values in the future?

@1ec5 no, do you have in mind any use case that will require an enumeration instead?

astojilj · 2019-09-09T06:41:47Z

platform/ios/src/MGLMapView.mm

-        // This map view is an immediate child of a view controller’s content view.
-        viewController = (UIViewController *)self.superview.nextResponder;
-    }
+        adjustedContentInsets = self.safeAreaInsets;


https://developer.apple.com/documentation/uikit/uiscrollview/2902259-adjustedcontentinset?language=objc

adjustedContentInset:

Use this property to obtain the adjusted area in which to draw content. The contentInsetAdjustmentBehavior property determines whether the safe area insets are included in the adjustment. The safe area insets are then added to the values in the contentInset property to obtain the final value of this property.

Is it different from expected, that we are not adding contentInset to safeAreaInsets but storing adjustedContentInsets (including SafeArea) to contentInsets?

This variable has no relation with the one from the official api. The method adjustContentInset resizes the mapView accordingly to events such as rotation or view changes, this variable is used to calculate those changes and set the new contentInsets. When automaticallyAdjustContentInset or the previous automaticallyAdjustsScrollViewInsets is set to YES then it will calculate the safe area which on iOS 11+ is easier.

L1031: safeArea is stored to contentInset, which looks like violating quoted documentation above.

contentInset has relation to official UIScrollView but it might be simpler if not having, let's rename it to padding and keep in sync with Android - certainly simpler then trying to mimic all the iOS scroll view API and maintain it after every release change.

contentInset was meant to behave like the property in UIViewController that's why I'm not violating any feature.

astojilj · 2019-09-09T06:44:12Z

platform/ios/src/MGLMapView.mm


-    if ( ! viewController.automaticallyAdjustsScrollViewInsets)


This is breaking change for people using automaticallyAdjustsScrollViewInsets.
We are not replacing it by contentInsetAdjustmentBehavior that documentation suggests:

@Property(nonatomic,assign) BOOL automaticallyAdjustsScrollViewInsets API_DEPRECATED("Use UIScrollView's contentInsetAdjustmentBehavior instead", ios(7.0,11.0),tvos(7.0,11.0)); // Defaults to YES

Just a comment to reconsider if releasing this in minor release.

Indeed — we need to keep supporting automaticallyAdjustsScrollViewInsets, especially for apps that have a minimum deployment version lower than iOS 11.

Any ideas on how to signal that we will not use this variable?

It’s a semver major change if we stop, so that’s not really an option right now. When a developer hits this code path, it could emit a console warning once that tells them we’re deprecating support for this.

I added the warning.

Has @friedbunny's server concern here been addressed?

@julianrex yes we are supporting both properties and logging a warning.

astojilj · 2019-09-09T06:49:26Z

platform/ios/src/MGLMapView.mm

@@ -1015,6 +1033,12 @@ - (void)setContentInset:(UIEdgeInsets)contentInset animated:(BOOL)animated

 - (void)setContentInset:(UIEdgeInsets)contentInset animated:(BOOL)animated completionHandler:(nullable void (^)(void))completion
 {
+    // makes sure the insets don't have negative values that could hide the ornaments
+    // thus violating our ToS
+    contentInset = UIEdgeInsetsMake(fmaxf(contentInset.top, 0),


this is diverging from iOS contentInset - negative values are allowed in contentInset. How about fixing ornaments positioning (clamping position to visible content area) somewhere else or documenting the change here, too.

astojilj · 2019-09-09T06:52:13Z

platform/ios/test/MGLMapViewContentInsetTests.m

+    XCTAssertTrue(UIEdgeInsetsEqualToEdgeInsets(self.mapView.contentInset, contentInset));
+    XCTAssertEqualWithAccuracy(self.mapView.centerCoordinate.latitude, center.latitude, 0.01);
+    XCTAssertEqualWithAccuracy(self.mapView.centerCoordinate.longitude, center.longitude, 0.01);
+    CGPoint shiftedPoint = [self.mapView convertCoordinate:center toPointToView:self.mapView];


Looks good.

fabian-guerra · 2019-09-09T18:45:38Z

@astojilj yes is wrong, fixed the reference. Thanks.

astojilj · 2019-09-10T17:12:30Z

platform/ios/src/MGLMapView.mm

+        adjustedContentInsets = self.safeAreaInsets;
+
+    } else {
+        adjustedContentInsets.top = viewController.topLayoutGuide.length;


Is this part is working properly also for >= 11?
If it is, let's have one implementation and avoidDependency to safeArea.

1ec5

Remember to add a changelog entry about the behaviors that changed and the new property.

Are any of these changes relevant to macOS? If so, could you make the corresponding changes or ticket that out as tail work? I’d like to keep the two implementations as close as possible; the introduction of this new property should actually help with that. Thanks!

1ec5 · 2019-10-02T18:33:26Z

platform/ios/src/MGLMapView.mm

@@ -3798,6 +3875,27 @@ - (MGLMapCamera *)cameraThatFitsCoordinateBounds:(MGLCoordinateBounds)bounds edg
    return [self cameraForCameraOptions:cameraOptions];
 }

+- (MGLMapCamera *)camera:(MGLMapCamera *)camera fittingCoordinate:(CLLocationCoordinate2D)coordinate edgePadding:(UIEdgeInsets)insets {


This method seems to be unused?

1ec5 · 2019-10-02T18:33:54Z

platform/ios/src/MGLMapView.mm

+                                 self.contentInset.right - self.safeMapViewContentInsets.right);
+
+        // makes sure the insets don't have negative values that could hide the ornaments
+        // thus violating our ToS


Cool, hadn’t considered that possibility before.

1ec5 · 2019-10-02T18:34:47Z

platform/ios/src/MGLMapView.h

 When the map view’s superview is an instance of `UIViewController` whose
 `automaticallyAdjustsScrollViewInsets` property is `YES`, the value of this
 property may be overridden at any time.
+
+ The usage of `automaticallyAdjustsScrollViewInsets` it is been deprecated


Nit: Split this into two sentences and replace “it is been” with “has been”. (×3)

1ec5 · 2019-10-02T18:37:29Z

platform/ios/src/MGLMapView.mm

+
+    // TODO: This warning should be removed when automaticallyAdjustsScrollViewInsets is removed from
+    // the UIViewController api.
+    NSLog(@"%@ WARNING UIViewController.automaticallyAdjustsScrollViewInsets is deprecated use MGLMapView.automaticallyAdjustContentInset instead.",


Should this log message be guarded by a dispatch_once, or is repetition desirable in this case?

1ec5 · 2019-10-02T18:39:07Z

platform/ios/src/MGLMapView.h

 When the map view’s superview is an instance of `UIViewController` whose
 `automaticallyAdjustsScrollViewInsets` property is `YES`, the value of this
 property may be overridden at any time.
+
+ The usage of `automaticallyAdjustsScrollViewInsets` it is been deprecated
+ use the map view’s property `automaticallyAdjustContentInset`instead.


“Use the MGLMapView.automaticallyAdjustsContentInset property instead.”

1ec5 · 2019-10-02T18:40:51Z

platform/ios/src/MGLMapView.h

+ The default value of this property is `YES`.
+
+ */
+@property (assign) BOOL automaticallyAdjustContentInset;


Rename this property to automaticallyAdjustsContentInset, so it looks like a property rather than an action method.

1ec5 · 2019-10-02T18:44:23Z

platform/ios/src/MGLMapView.h

+ The default value of this property is `YES`.
+
+ */
+@property (assign) BOOL automaticallyAdjustContentInset;


The new UIScrollView property’s type is an enumeration rather than a Boolean. Do we anticipate adding more possible values in the future?

julianrex

Looks good - just the questions below.

julianrex · 2019-10-02T18:10:20Z

platform/ios/src/MGLMapView.mm

+
+    // TODO: This warning should be removed when automaticallyAdjustsScrollViewInsets is removed from
+    // the UIViewController api.
+    NSLog(@"%@ WARNING UIViewController.automaticallyAdjustsScrollViewInsets is deprecated use MGLMapView.automaticallyAdjustContentInset instead.",


Use MGLLogInfo here?

We want the warning logged into the console directly using our logging framework can't warranty that.

julianrex · 2019-10-02T18:18:53Z

platform/ios/src/MGLMapView.mm

+        automaticallyAdjustContentInset = viewController.automaticallyAdjustsScrollViewInsets;
+    }
+
+    if (! automaticallyAdjustContentInset) {


Should this be if (automaticallyAdjustContentInset)?

If I had an else option it would be. In this case I will subtract the safe areas only if automaticallyAdjustContentInset is NO

Thanks for clarifying, I had misread the - for a + 😄

julianrex · 2019-10-02T18:24:42Z

platform/ios/src/MGLMapView.mm


-    if ( ! viewController.automaticallyAdjustsScrollViewInsets)


Has @friedbunny's server concern here been addressed?

Fixed an issue that caused ornaments ignore the contentInset. Added a new property automaticallyAdjustContentInset that has the same purpose as UIViewController. automaticallyAdjustsScrollViewInsets. This was changed due to the latter being deprecated.

The property automaticallyAdjustsScrollViewInsets overrode automaticallyAdjustsScrollViewInsets which caused a breaking change. This is fixed to consider the legacy property when calculating the content insets and added tests for both cases.

Fixed an issue that caused a discrepancy between the contentInset in MGLMapView and the padding in the transformation state. When padding is passed through methods such as setCamera it’s persisted. This fix resets the contentInsets.

…ion.

astojilj · 2019-10-06T04:42:29Z

@fabian-guerra Sorry for late review. Was this patch verified against the latest version of navigation-ios including the mapbox/mapbox-navigation-ios#2211 patch, on both iOS device and CarPlay?

cc @1ec5

fabian-guerra · 2019-10-07T17:57:55Z

@astojilj no. We fixed other content insets issues and decided that the issue affecting navigation will be addressed on a SEMVER.

fabian-guerra added bug iOS Mapbox Maps SDK for iOS needs changelog Indicates PR needs a changelog entry prior to merging. labels Sep 7, 2019

fabian-guerra added this to the release-ristretto milestone Sep 7, 2019

fabian-guerra requested review from friedbunny, julianrex and a team September 7, 2019 03:23

fabian-guerra self-assigned this Sep 7, 2019

astojilj reviewed Sep 9, 2019

View reviewed changes

fabian-guerra force-pushed the fabian-insets-14827 branch from 410d64f to 2018fb7 Compare September 9, 2019 22:30

astojilj reviewed Sep 10, 2019

View reviewed changes

julianrex modified the milestones: release-ristretto, release-sangria Sep 26, 2019

fabian-guerra force-pushed the fabian-insets-14827 branch from 2018fb7 to 309e15d Compare September 27, 2019 19:12

fabian-guerra requested a review from 1ec5 September 27, 2019 19:20

fabian-guerra force-pushed the fabian-insets-14827 branch from 309e15d to 53eaf64 Compare September 30, 2019 22:32

1ec5 reviewed Oct 2, 2019

View reviewed changes

julianrex approved these changes Oct 2, 2019

View reviewed changes

fabian-guerra force-pushed the fabian-insets-14827 branch from 53eaf64 to 7321375 Compare October 3, 2019 22:26

fabian-guerra removed the needs changelog Indicates PR needs a changelog entry prior to merging. label Oct 3, 2019

fabian-guerra force-pushed the fabian-insets-14827 branch from 3b8200a to 5042565 Compare October 3, 2019 22:52

fabian-guerra added 7 commits October 4, 2019 16:22

[ios] Add mapView content inset tests.

82fa316

[ios] Fix pinch test.

1cfddc0

[ios] Update automaticallyAdjustsScrollViewInsets name and documentat…

950a80e

…ion.

[ios] Update changelog.

4539135

fabian-guerra force-pushed the fabian-insets-14827 branch from 5042565 to 4539135 Compare October 4, 2019 23:24

fabian-guerra merged commit 5c565a5 into master Oct 5, 2019

fabian-guerra deleted the fabian-insets-14827 branch October 5, 2019 00:13

pappalar mentioned this pull request Nov 12, 2019

Mapview.content inset are not working as expected on 5.5.0 mapbox/mapbox-gl-native-ios#59

Closed

[ios] Fixes an issue that caused the ornaments ignore contentInset property. #15584

[ios] Fixes an issue that caused the ornaments ignore contentInset property. #15584

Conversation

fabian-guerra commented Sep 7, 2019 • edited Loading

Tasks:

astojilj left a comment • edited Loading

Choose a reason for hiding this comment

astojilj Sep 9, 2019 • edited by friedbunny Loading

Choose a reason for hiding this comment

friedbunny Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-guerra commented Sep 9, 2019

astojilj Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julianrex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astojilj commented Oct 6, 2019 • edited Loading

fabian-guerra commented Oct 7, 2019

fabian-guerra commented Sep 7, 2019 •

edited

Loading

astojilj left a comment •

edited

Loading

astojilj Sep 9, 2019 •

edited by friedbunny

Loading

friedbunny Sep 9, 2019 •

edited

Loading

friedbunny Sep 9, 2019 •

edited

Loading

astojilj Sep 10, 2019 •

edited

Loading

astojilj commented Oct 6, 2019 •

edited

Loading