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

[ios] Don't use negative content insets #4504

Closed

Conversation

friedbunny
Copy link
Contributor

When a map view was smaller than the entire viewport, negative content insets would be applied. This had the effect of making the visual "center" of every map the center of the viewport, ignoring the map view’s true center. Negative content insets would have only been valid if the map view extended outside of its frame, which cannot happen.

Applying negative content insets would erroneously offset showAnnotations:, setCenterCoordinate:, “follow” user tracking mode, and so on.

Fixes #4440.

9e7aad7d00ef09cf74f6d08f4d1e4189d613ac5a also slightly tweaks annotation padding behavior so that smaller views use proportional padding.

/cc @1ec5

@friedbunny friedbunny self-assigned this Mar 29, 2016
@friedbunny friedbunny added bug iOS Mapbox Maps SDK for iOS ✓ ready for review labels Mar 29, 2016
@friedbunny friedbunny added this to the ios-v3.2.0 milestone Mar 29, 2016
/// Returns edge insets with negative values replaced with 0.
- (UIEdgeInsets)removeNegativeInsets:(UIEdgeInsets)insets
{
return UIEdgeInsetsMake((insets.top > 0 ? insets.top : 0), (insets.left > 0 ? insets.left : 0),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be expressed with the MIN macro for brevity. And since it's only used in one place, it should be inclined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course I mean the MAX macro. Definitely keep the comment. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will incline the MAXes. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Force pushed these changes at 0b3c67d.

@friedbunny friedbunny force-pushed the fb-content-insets-4440 branch from 9e7aad7 to 88af52f Compare March 29, 2016 03:29
@friedbunny
Copy link
Contributor Author

The iOS build on Bitrise is failing with xcode test exit code: 65 (and nothing else). 😞

@friedbunny
Copy link
Contributor Author

Seeing tests fail intermittently on my machine, too:

Failing tests:
    -[MapViewTests testContentInsetsWithTinyMapView]
** TEST FAILED **

make: *** [itest] Error 65

@friedbunny friedbunny force-pushed the fb-content-insets-4440 branch from 9e91e6c to 9e8cc83 Compare March 29, 2016 06:01
@friedbunny
Copy link
Contributor Author

Tests pass reliably (five in a row on Bitrise) after switching to KVO in 9e91e6c886dc15257093d032df3f6c83e947df84, which I rebased back into 183f55e.

@"map should not have bottom content inset");
return YES;
}];
[self waitForExpectationsWithTimeout:5.0 handler:nil];
Copy link
Contributor

Choose a reason for hiding this comment

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

Five seconds seems rather generous. Is it necessary to make this test take 10 seconds for a frame change that should be instant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll lower this to 1.0.

This is mainly a buffer against slow CI machines. waitForExpectationsWithTimeout:handler: should never add more than a tick or two to the execution time, no matter the timeout length — if it exceeds the timeout, it’s a failure and everything stops.

I think xcpretty is swallowing our test output and the elapsed time per test on Bitrise, but locally this test completes in the blink of an eye.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a map view was smaller than the entire viewport, negative content insets would be applied. Negative content insets would only be valid if the map view extended outside of its frame, which cannot happen.

Fixes #4440.
@friedbunny friedbunny force-pushed the fb-content-insets-4440 branch from 203849f to 9f24ebd Compare March 29, 2016 18:54
@1ec5
Copy link
Contributor

1ec5 commented Mar 29, 2016

Merged as d20795b and 593f3d5.

@1ec5 1ec5 closed this Mar 29, 2016
@friedbunny
Copy link
Contributor Author

This PR got jumped by another into release-ios-3.2.0-android-4.0.0 and I neglected to force-push again into this branch. Merged per above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants