Global app state manipulation

[simonseyer]

Mapbox Navigation SDK version: Master branch

In the RouteController.init the battery monitoring is enabled (UIDevice.current.isBatteryMonitoringEnabled = true) and in the deinit it is disabled again (UIDevice.current.isBatteryMonitoringEnabled = false).

This manipulation of a global app state is breaking features of us that rely on battery monitoring. This at least has to be configurable. Even better would be if the configuration would be delegated to the integrating app (delegate.routeController(_ controller, requiresBatteryMonitoring: true/false)).

I only read a subset of your code yet, but I also recognized a hard dependency to the CLLocationManger and its types. This also causes some inconveniences for us because our location is coming from different sources. My suggestion would be to have a protocol which defines just the requirements you have (a location with lat/long & accuracy and true heading I guess) and provide a default implementation with a CLLocationManager.

In general it would be best (from my point of view) to not rely on any system library and to delegate all the required informations/actions to the app code while still providing default implemenations. This would provide a lot of flexibility (that we more or less need to integrate your SDK into our rather complex app) while still keeping the "works out of the box" experience.

[akitchen]

Hello @simonseyer and thank you very much for opening this issue. Your feedback is greatly appreciated!

Regarding the issues you raised, we are aware and in agreement regarding both the issue of manipulating global shared state as well as the location manager coupling. Note that the latter we have also been discussing in our Maps SDK (issue) (PR) -- your feedback on this approach would be most welcome as to whether something similar in our Navigation SDK would meet your needs.

Regarding the manipulation of -[UIDevice isBatteryMonitoringEnabled], we have been discussing a solution in #1476 . We use battery monitoring in aspects of our code, but can introduce delegation to control whether to disable it when finished.

[1ec5]

Note that battery monitoring is currently used not only for setting CLLocationManager options, but also for throttling NavigationMapView’s frame rate when the device isn’t plugged in.

[simonseyer]

Thanks for the immediate reply. I’m happy that you are sharing my opinion about this topic. I can only look into the mentioned ticket/pr on Monday but will be happy to give feedback then.

[simonseyer]

I reviewed the PR and it goes in the right direction from my point of view. However, I would just suggest getting rid of the CoreLocation dependency entirely. Added a respective comment on the PR.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale]

Stale.