Send a turnstile event using Snapshotter #908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
macdrevx
reviewed
Dec 7, 2021
macdrevx
reviewed
Dec 7, 2021
jmkiley
force-pushed
the
jk/snapshotter-turnstile
branch
2 times, most recently
from
cec5a9c
to
e627c4c
Compare
macdrevx
reviewed
Dec 8, 2021
macdrevx
reviewed
Dec 8, 2021
macdrevx
reviewed
Dec 8, 2021
macdrevx
reviewed
Dec 8, 2021
macdrevx
force-pushed
the
jk/snapshotter-turnstile
branch
from
64f88e2
to
d57cd4f
Compare
macdrevx
reviewed
Dec 14, 2021
| @@ -32,6 +32,7 @@ public class Snapshotter { | |||
| self.options = options | |||
| mapSnapshotter = MapSnapshotter(options: MapboxCoreMaps.MapSnapshotOptions(options)) | |||
| style = Style(with: mapSnapshotter) | |||
| EventsManager(accessToken: options.resourceOptions.accessToken).sendTurnstile() | |||
There was a problem hiding this comment.
This is the important change. This approach matches Android, but there are a few questions we need to think about:
- EventsManager is a thin wrapper around
MMEEventsManager.shared(). Each time anEventsManageris initialized, it will callMMEEventsManager.shared().initialize(…). Is it okay for that to happen more than once (will happen with each snapshotter and map view that is created). What if you are using different access tokens for different purposes in the same app?- Based on https://github.com/mapbox/mapbox-events-ios/blob/main/Sources/MapboxMobileEvents/MMEEventsManager.m#L92-L101, it seems like subsequent calls to initialize will just update the access token, but won't repeat any other setup.
- One problem with having more than one instance of
EventsManageris that each instance will observe changes to the"MGLMapboxMetricsEnabled"user defaults key and will callflushupon receiving a memory warning. These behaviors only need to happen once perMMEEventsManagerinstance.- This MME design means we need to convert EventsManager to a singleton as well to avoid doing more work than necessary. @vesh93 I'd love to see us move away from using a singleton pattern. It's a bad match for our initialization APIs which would lead you to believe there's nothing stopping you from using multiple access tokens simultaneously in the same process.
cc: @vesh93
There was a problem hiding this comment.
Updated to convert EventsManager to a singleton.
vayesh
approved these changes
Dec 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist:
Summary of changes
Snapshotterwill now send a turnstile event upon initialization. This mirrors mapbox/mapbox-maps-android#920EventsManagerhas been simplified and converted to a singleton to address the comment below.