-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add completion handlers to animated MGLMapView methods #14381
Conversation
} | ||
else if (self.userTrackingState == MGLUserTrackingStateChanged) | ||
{ | ||
// Subsequent updates get a more subtle animation. | ||
[self didUpdateLocationIncrementallyAnimated:animated]; | ||
[self didUpdateLocationIncrementallyAnimated:animated completionHandler:completion]; | ||
} | ||
[self unrotateIfNeededAnimated:YES]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method call can result in a snap-to-north animation. Would it be surprising if the animation could occur after the completion handler executes? Accounting for this edge case may complicate the code somewhat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably at the very last chained animation but I don't have a strong opinion against that. However, unrotate is private so it shouldn't complex to hook up too much.
[mapView showAnnotations:@[annotation] edgePadding:UIEdgeInsetsZero animated:YES completionHandler:^{ | ||
[expectation fulfill]; | ||
}]; | ||
[self waitForExpectations:@[expectation] timeout:1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We’ve had problems with expectations timing out in the past on CI. Since this method doesn’t accept a duration that can be set to 0 (should it?), I’d remove the expectation at the first sign of trouble on CI.
@@ -41,4 +41,104 @@ - (void)testCoordinateBoundsConversion { | |||
XCTAssertTrue(CGRectIntersectsRect(spanningBoundsRect, rightAntimeridianBoundsRect), @"Something"); | |||
} | |||
|
|||
- (void)testUserTrackingModeCompletion { | |||
__block BOOL completed = NO; | |||
[mapView setUserTrackingMode:MGLUserTrackingModeNone animated:NO completionHandler:^{ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
381d005
to
03f718c
Compare
03f718c
to
595b9c2
Compare
direction, | ||
MGLStringFromBOOL(animated)); | ||
[self setVisibleCoordinates:coordinates count:count edgePadding:insets direction:direction duration:animated ? MGLAnimationDuration : 0 animationTimingFunction:nil]; | ||
[self setVisibleCoordinates:coordinates count:count edgePadding:insets direction:self.direction duration:animated ? MGLAnimationDuration : 0 animationTimingFunction:nil]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.direction
is the current value, not the new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in this version of the method, no new direction
is passed in, so we keep the current one. Ideally this would say −1, but the method incorrectly clamps negative values to 0: #14574.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I overlooked the signature change in the changeset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % one nit
595b9c2
to
64ba23f
Compare
What's stopping this from merging? |
Mainly, a review from someone who will have to maintain it over the longterm. 😅 My general concerns here are: Maybe don’t add so much “new” API areaI think I’d like to see us deprecate the old completion-less methods, if only to reduce the cognitive burden of readers, autocompleters, and, eventually, maintainers. This’ll mean updating documentation sooner, rather than later, but that should be part of validating these changes, anyway. Touching so many fundamental parts of platform functionalityMost of the functionality touched here is specific to our darwin SDKs, so we have to rely exclusively on the tests we write and the QA testing we do ourselves — there’s not much core or Android backstop. Asynchronous code can be relatively onerous to write stable tests against, but we need this functionality to be extremely solid and well-covered. To that end, before we can accept this, I think we have to rigorously verify that each new method works as expected (and will continue to do so). That may be easier if this PR were split into smaller pieces, or if multiple reviewers were tasked with a subset of the new API. |
I’m on board with this idea. There are way too many As far as autocompletion goes, the primary effect of all these redundant methods is the same effect as a Swift method that specifies default values for most of its arguments. That’s a pretty reasonable practice in Swift, but I can understand the reluctance to keep it here in Objective-C, where there’s a significant code overhead. I don’t think deprecating those redundant methods would reduce the size of this PR or the number of methods that it would add. Of the methods that this PR adds, is there any that you’d leave out?
MGLMapViewTests includes automated unit tests for many of the added methods. But mainly it tests edge cases, because I’m most worried about missing one of the many early return cases and because we don’t want these tests to run too long. |
64ba23f
to
4fc539c
Compare
4fc539c
to
9c93e06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failures in -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout]
need to be investigated and addressed.
/mapbox-gl-native/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m:165: error: -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout] : ((expectCalloutToBeFullyOnscreen == CGRectContainsRectWithAccuracy(self.mapView.bounds, calloutView.frame, 0.25)) is true) failed - Expect contains:1, Mapview:{{0, 0}, {414, 896}} annotation:{{5.5948931262606649, 392}, {51, 112}} callout:{{-9.4051068737393351, 335}, {87, 57}}
/mapbox-gl-native/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m:150: error: -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout] : ((CGRectContainsRectWithAccuracy(self.mapView.bounds, annotationFrameAfterSelection, 0.25)) is true) failed - Mapview:{{0, 0}, {414, 896}} frame:{{181.5, 796}, {51, 112}}
/mapbox-gl-native/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m:150: error: -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout] : ((CGRectContainsRectWithAccuracy(self.mapView.bounds, annotationFrameAfterSelection, 0.25)) is true) failed - Mapview:{{0, 0}, {414, 896}} frame:{{181.5, 784.5}, {51, 112}}
/mapbox-gl-native/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m:142: error: -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout] : (((latitudeDelta > epsilon) || (longitudeDelta > epsilon)) is true) failed - Deltas: lat=0.000000, long=0.000000
/mapbox-gl-native/platform/ios/Integration Tests/Annotation Tests/MGLAnnotationViewIntegrationTests.m:142: error: -[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout] : (((latitudeDelta > epsilon) || (longitudeDelta > epsilon)) is true) failed - Deltas: lat=0.000000, long=0.000000
Test Case '-[MGLAnnotationViewIntegrationTests testSelectionMoveIntoViewWithCallout]' failed (3.520 seconds).
I pushed df6d7e7 updating this test battery to use replacement -[MGLMapView selectAnnotation:moveIntoView:animateSelection:completion:]
method, per #14381 (comment).
/cc @julianrex
*/ | ||
- (void)selectAnnotation:(id <MGLAnnotation>)annotation moveIntoView:(BOOL)moveIntoView animateSelection:(BOOL)animateSelection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is public API added in #13689 by @julianrex, but undocumented. This was evidently left undocumented so that we could change it at will, so this breaking change is fine.
Ticketed out this method for more discussion in #14904.
|
Deprecations are happening in #14959 and await the merging of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @julianrex has indicated that the integration test failures here are separate, let’s move ahead with merging this and testing it out in release-p
pre-releases.
df6d7e7
to
082aa34
Compare
082aa34
to
87fa146
Compare
Rebased this on current |
Added variants of several animated MGLMapView methods that accept completion handlers. This change does exacerbate the proliferation of camera-related methods in MGLMapView to a degree, but it’s in keeping with the principle that asynchronous operations should have completion handlers. Developers should use these methods instead of the camera-related MGLMapViewDelegate methods for more readable and less buggy code. Eventually, we may be able to call the delegate methods only in response to a user gesture or location update, in keeping with Cocoa best practices.
The specific methods added are:
-[MGLMapView setVisibleCoordinateBounds:edgePadding:animated:completionHandler:]
-[MGLMapView setContentInset:animated:completionHandler:]
(iOS)-[MGLMapView setContentInsets:animated:completionHandler:]
(macOS)-[MGLMapView setUserTrackingMode:animated:completionHandler:]
(iOS)-[MGLMapView setTargetCoordinate:animated:completionHandler:]
(iOS)-[MGLMapView showAnnotations:edgePadding:animated:completionHandler:]
-[MGLMapView selectAnnotation:animated:completionHandler:]
(iOS)Please double-check that completion handlers are only called on the main thread, and that I haven’t overlooked any code paths where these methods can return without calling the completion handler or where they can call the completion handler synchronously before calling it asynchronously as well. What I would give for a safe Objective-C equivalent to Swift’s
defer
keyword…To do:
Fixes #5839 and fixes #5840.
/cc @mapbox/maps-ios @frederoni @vincethecoder