-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[iOS] Add turnstile events for Snapshotter #13476
Conversation
@@ -74,6 +75,7 @@ - (instancetype)initWithImage:(nullable MGLImage *)image scale:(CGFloat)scale po | |||
_latLngForFn = std::move(latLngForFn); | |||
_scale = scale; | |||
_image = image; | |||
[MGLMapboxEvents pushTurnstileEvent]; |
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.
I think this should be in -startWithQueue:completionHandler:
- an MGLMapSnapshotter
may be allocated and not used, or used multiple times.
Perhaps sending the turnstile event somewhere around
mapbox-gl-native/platform/darwin/src/MGLMapSnapshotter.mm
Lines 201 to 210 in 53351c2
self.completion = completion; | |
self.resultQueue = queue; | |
self.cancelled = NO; | |
__weak __typeof__(self) weakSelf = self; | |
// mbgl::Scheduler::GetCurrent() scheduler means "run callback on current (ie UI/main) thread" | |
// capture weakSelf to avoid retain cycle if callback is never called (ie snapshot cancelled) | |
_snapshotCallback = std::make_unique<mbgl::Actor<mbgl::MapSnapshotter::Callback>>( |
would make sense?
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.
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.
@julianrex I think the behavior should consist with MGLMapview
did.
mapbox-gl-native/platform/ios/src/MGLMapView.mm
Lines 629 to 632 in 8bc362e
if ([UIApplication sharedApplication].applicationState != UIApplicationStateBackground) { | |
[MGLMapboxEvents pushTurnstileEvent]; | |
[MGLMapboxEvents pushEvent:MMEEventTypeMapLoad withAttributes:@{}]; | |
} |
The turnstile events send in the initializer of mapview.
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.
@julianrex @tobrun What's your opinion? Can I merge 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.
We can go ahead and merge this for consistency (with the existing landed Android PR) - though I'd like to fully understand why it shouldn't be in the start...
method.
Is there a ticket tracking this question? We should get to a resolution. /cc @lloydsheng @julianrex |
Snapshotter (i.e. https://www.mapbox.com/android-docs/maps/overview/snapshotter) is part of the Maps SDK and should be sending turnstile events before usage on both iOS and Android (Snapshotter requests need the same access token we use for regular tile requests).
We should add turnstile events for Snapshotter.