Navigation Menu

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

Add option to configure CLLocationManager activityType #2149

Closed
wants to merge 1 commit into from

Conversation

boundsj
Copy link
Contributor

@boundsj boundsj commented Aug 21, 2015

This is a proof of concept to illustrate (and get feedback about) exposing location manager options so a client can have some say about how active the location manager will be when it runs in the background.

Since the MGLMapboxEvents is a singleton and because it has not (historically) been directly exposed
to client code, it does not lend itself to configuration. I worked around this (for now) by proxying
config calls via the MGLMapView which is sort of the main interface to the library for a client. This approach may actually be ok but the names I chose for the API should probably be made better. We could also consider creating some sort of configuration object that a user creates and passes in. The important point is that the configuration must happen before MGLAccountManager.setAccessToken is called since that is what actually starts the metrics location tracking process.

In a client app, the usage of this new feature looks like this:

// the new kid
MGLMapView.setActivityType(.AutomotiveNavigation)

MGLAccountManager.setAccessToken(SiriusAccessToken)

In short; this works but the impl feels a little weird because of MGLMapboxEvents singleton-ness and also because of the call dependency that leaks through to the clients. Ideally, I would want some sort
of setUp: API that takes an access token and config options but, currently, MGLAccountManager
does not seem like the right object to build something like setUp on top of.

Finally, it might be even more useful to use something like this pattern to allow users to set distanceFilter and desiredAccuracy. Those options might do even more to mellow out the background activity and not having control over them (as a client) would make me somewhat concerned. Don't want this to turn into a 747 flight deck though

flightdeck

@1ec5
Copy link
Contributor

1ec5 commented Aug 21, 2015

cc @incanus @friedbunny @bleege

Since the MGLMapboxEvents is a singleton and because it has not (historically) been directly exposed
to client code, it does not lend itself to configuration. I worked around this (for now) by proxying
config calls via the MGLMapView which is sort of the main interface to the library for a client.

At one point, we did expose MGLMapboxEvents publicly, as a property of MGLAccountManager I think. We gradually privatized it in #1491, #1553, and #1580, because there were no options for developers to configure (other than the access token). With the addition of activityType (and presumably other options like it), I’d be open to exposing the singleton MGLMapboxEvents publicly once again.

Hanging the options off of MGLMapView raises some interesting questions. There can be multiple MGLMapViews in an application but only one MGLMapboxEvents; setting the property on one map view will in that sense affect another (and you’d expect KVO notifications to arise from that). This is the reason we factored out the access token into MGLAccountManager. On the other hand, what if MGLMapView made use of activityType for its own CLLocationManager? Maybe the user dot’s updates would better match the current mode of transportation.

It might make sense for all the CLLocationManagers – the one owned by the singleton MGLMapboxEvents, as well as the ones owned by all the MGLMapViews – to be on the same page with respect to these options. Would it be weird if we made them properties of MGLAccountManager, and maybe rename MGLAccountManager to something less account-specific?

We could also consider creating some sort of configuration object that a user creates and passes in. The important point is that the configuration must happen before MGLAccountManager.setAccessToken is called since that is what actually starts the metrics location tracking process.

If the access token is set in Info.plist, -[MGLAccountManager setAccessToken:] gets called as soon as the framework loads. Have you considered adding the ability to specify an activityType inside Info.plist?

@1ec5 1ec5 added feature iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries labels Aug 21, 2015
@boundsj
Copy link
Contributor Author

boundsj commented Aug 21, 2015

@1ec5

It might make sense for all the CLLocationManagers – the one owned by the singleton MGLMapboxEvents, as well as the ones owned by all the MGLMapViews – to be on the same page with respect to these options. Would it be weird if we made them properties of MGLAccountManager, and maybe rename MGLAccountManager to something less account-specific?

It does seem like having all of the location managers work the same way make sense. I think this may really depend on what is sufficient for metrics collection. I do think that your suggestion of a consolidation to (and rename of) MGLAccountManager probably makes a lot of sense.

Have you considered adding the ability to specify an activityType inside Info.plist?

I did not consider that yet but that seems like it is required!

@boundsj boundsj force-pushed the expose-clmanager-configuration branch from 9c2a0e6 to 70e1fa6 Compare August 22, 2015 00:14
@boundsj
Copy link
Contributor Author

boundsj commented Aug 22, 2015

@1ec5 latest commit has the revisions based on this thread and our conversations today. Hopefully a little closer. In the client app, you can now set the option like this:

screen shot 2015-08-21 at 5 22 53 pm

This commit renames MGLAccountManager to MGLConfigurationManager
since the name "account" was too specific. In addition, a new
setting was added to the configuration manager to allow a user
to specify a desired `activityType` to be used by location manager
instances in the library. This was added using existing conventions
so that the value can be specified by the API or in the Info.plist
in the client application. It's worth noting that, for backwards
compatibility reasons, MGLAccountManager was kept around
with deprecation warnings and just serves as a wrapper for
MGLConfigurationManager.

Finally, a few unrelated changes were made to clean up an unused
property in the MGLConfigurationManager_Private header and to
make constants for string values used in MGLConfigurationManager
(what was MGLAccountManager).
@boundsj boundsj force-pushed the expose-clmanager-configuration branch from 70e1fa6 to 15b2c60 Compare August 22, 2015 00:26
@boundsj boundsj closed this Aug 22, 2015
@jfirebaugh jfirebaugh deleted the expose-clmanager-configuration branch August 25, 2015 20:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature iOS Mapbox Maps SDK for iOS telemetry Integration with Mapbox Telemetry libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants