Added additional comments. Disabled pending tests in code, rather than via scheme.

3 files changed, 73 insertions, 60 deletions

diff --git a/platform/ios/Integration Tests/MGLCameraTransitionTests.mm b/platform/ios/Integration Tests/MGLCameraTransitionTests.mm
index 6552c07d96..d5c288fbb9 100644
--- a/platform/ios/Integration Tests/MGLCameraTransitionTests.mm
+++ b/platform/ios/Integration Tests/MGLCameraTransitionTests.mm
@@ -110,55 +110,6 @@
     XCTAssertEqualWithAccuracy(self.mapView.direction, 0.0, 0.001, @"Camera should have reset to north. %0.3f", self.mapView.direction);
 }
 
-- (void)testContinuallyResettingNorthInIsChanging {
-
-    // Reset to non-zero, prior to testing
-    [self.mapView setDirection:45 animated:NO];
-
-    XCTestExpectation *expectation = [self expectationWithDescription:@"regionDidChange expectation"];
-    expectation.expectedFulfillmentCount = 2;
-    expectation.assertForOverFulfill = YES;
-
-    self.regionIsChanging = ^(MGLMapView *mapView) {
-        [mapView resetNorth];
-    };
-
-    self.regionDidChange = ^(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated) {
-        [expectation fulfill];
-    };
-
-    [self.mapView setDirection:90 animated:YES];
-    [self waitForExpectations:@[expectation] timeout:1.5];
-
-    XCTAssertEqualWithAccuracy(self.mapView.direction, 0.0, 0.001, @"Camera should have reset to north. %0.3f", self.mapView.direction);
-}
-
-
-- (void)testContinuallySettingCoordinateInIsChanging {
-
-    // Reset to non-zero, prior to testing
-    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(0.0, 0.0) animated:NO];
-
-    XCTestExpectation *expectation = [self expectationWithDescription:@"regionDidChange expectation"];
-    expectation.expectedFulfillmentCount = 2;
-    expectation.assertForOverFulfill = YES;
-
-    __weak typeof(self) weakself = self;
-
-    self.regionIsChanging = ^(MGLMapView *mapView) {
-        [weakself.mapView setCenterCoordinate:CLLocationCoordinate2DMake(-40.0, -40.0) animated:YES];
-    };
-
-    self.regionDidChange = ^(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated) {
-        [expectation fulfill];
-    };
-
-    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(40.0, 40.0) animated:YES];
-    [self waitForExpectations:@[expectation] timeout:1.5];
-
-    XCTAssertEqualWithAccuracy(self.mapView.direction, 0.0, 0.001, @"Camera should have reset to north. %0.3f", self.mapView.direction);
-}
-
 - (void)testSetCenterCancelsTransitions {
     XCTestExpectation *cameraIsInDCExpectation = [self expectationWithDescription:@"camera reset to DC"];
 
@@ -383,4 +334,65 @@
     XCTAssert(delegateCallCount == 2, @"Expecting 2 regionDidChange callbacks, got %ld", delegateCallCount); // Once for the setDirection and once for the reset north
 }
 
+#pragma mark - Pending tests
+
+- (void)disabled_testContinuallyResettingNorthInIsChangingPENDING {
+
+    // See https://github.com/mapbox/mapbox-gl-native/pull/11614
+    // This test currently fails, unsurprisingly, since we're continually
+    // setting the camera to the same parameters during its update.
+    //
+    // Possible solutions/expectations:
+    // - If you set camera parameters that match the *current* target parameters
+    //      then the transition could be a no-op. We'd need to consider any completion
+    //      block
+    //  - Ideally we would detect this case and disallow it.
+
+    // Reset to non-zero, prior to testing
+    [self.mapView setDirection:45 animated:NO];
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"regionDidChange expectation"];
+    expectation.expectedFulfillmentCount = 2;
+    expectation.assertForOverFulfill = YES;
+
+    self.regionIsChanging = ^(MGLMapView *mapView) {
+        [mapView resetNorth];
+    };
+
+    self.regionDidChange = ^(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated) {
+        [expectation fulfill];
+    };
+
+    [self.mapView setDirection:90 animated:YES];
+    [self waitForExpectations:@[expectation] timeout:1.5];
+
+    XCTAssertEqualWithAccuracy(self.mapView.direction, 0.0, 0.001, @"Camera should have reset to north. %0.3f", self.mapView.direction);
+}
+
+- (void)disabled_testContinuallySettingCoordinateInIsChangingPENDING {
+    // See above comment in `-disabled_testContinuallyResettingNorthInIsChangingPENDING`
+
+    // Reset to non-zero, prior to testing
+    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(0.0, 0.0) animated:NO];
+
+    XCTestExpectation *expectation = [self expectationWithDescription:@"regionDidChange expectation"];
+    expectation.expectedFulfillmentCount = 2;
+    expectation.assertForOverFulfill = YES;
+
+    __weak typeof(self) weakself = self;
+
+    self.regionIsChanging = ^(MGLMapView *mapView) {
+        [weakself.mapView setCenterCoordinate:CLLocationCoordinate2DMake(-40.0, -40.0) animated:YES];
+    };
+
+    self.regionDidChange = ^(MGLMapView *mapView, MGLCameraChangeReason reason, BOOL animated) {
+        [expectation fulfill];
+    };
+
+    [self.mapView setCenterCoordinate:CLLocationCoordinate2DMake(40.0, 40.0) animated:YES];
+    [self waitForExpectations:@[expectation] timeout:1.5];
+
+    XCTAssertEqualWithAccuracy(self.mapView.direction, 0.0, 0.001, @"Camera should have reset to north. %0.3f", self.mapView.direction);
+}
+
 @end
diff --git a/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/Integration Test Harness.xcscheme b/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/Integration Test Harness.xcscheme
index ad1f9556be..1638592557 100644
--- a/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/Integration Test Harness.xcscheme
+++ b/platform/ios/ios.xcodeproj/xcshareddata/xcschemes/Integration Test Harness.xcscheme
@@ -37,14 +37,6 @@
                BlueprintName = "integration"
                ReferencedContainer = "container:ios.xcodeproj">
             </BuildableReference>
-            <SkippedTests>
-               <Test
-                  Identifier = "MBCameraTransitionTests/testContinuallyResettingNorthInIsChanging">
-               </Test>
-               <Test
-                  Identifier = "MBCameraTransitionTests/testContinuallySettingCoordinateInIsChanging">
-               </Test>
-            </SkippedTests>
          </TestableReference>
       </Testables>
       <MacroExpansion>
diff --git a/src/mbgl/map/transform.cpp b/src/mbgl/map/transform.cpp
index 0e58edd154..da8e243d91 100644
--- a/src/mbgl/map/transform.cpp
+++ b/src/mbgl/map/transform.cpp
@@ -628,17 +628,23 @@ bool Transform::inTransition() const {
 }
 
 void Transform::updateTransitions(const TimePoint& now) {
+
     // Use a temporary function to ensure that the transitionFrameFn lambda is
     // called only once per update.
-    //
-    // This addresses the symptons of https://github.com/mapbox/mapbox-gl-native/issues/11180
+
+    // This addresses the symptoms of https://github.com/mapbox/mapbox-gl-native/issues/11180
     // where setting a shape source to nil (or similar) in the `onCameraIsChanging`
     // observer function causes `Map::Impl::onUpdate()` to be called which
     // in turn calls this function (before the current iteration has completed),
-    // leading to an infinite loop.
+    // leading to an infinite loop. See https://github.com/mapbox/mapbox-gl-native/issues/5833
+    // for a similar, related, issue.
     //
     // By temporarily nulling the `transitionFrameFn` (and then restoring it
     // after the temporary has been called) we stop this recursion.
+    //
+    // It's important to note that the scope of this change is stop the above
+    // crashes. It doesn't address any potential deeper issue (for example
+    // user error, how often and when transition callbacks are called).
 
     auto transition = std::move(transitionFrameFn);
     transitionFrameFn = nullptr;
@@ -655,6 +661,9 @@ void Transform::updateTransitions(const TimePoint& now) {
             finish();
         }
     } else if (!transitionFrameFn) {
+        // We have to check `transitionFrameFn` is nil here, since a new transition
+        // may have been triggered in a user callback (from the transition call
+        // above)
         transitionFrameFn = std::move(transition);
     }
 }