Allow developers to control whether battery monitoring is enabled #1476
Conversation
|
Please don’t merge this as-is, as it hasn’t been discussed, groomed or prioritized yet. This PR might not adequately address the issue - instead of introducing more state on RouteController, this should probably be delegated as requested. |
|
My thoughts here: If we require battery monitoring for our own features, we should optionally delegate the decision to disable it when finished. |
|
@akitchen how does something that looks like: optional func routeController(_ routeController: RouteController, willChangeBatteryMonitoringState from: Bool, to: Bool) -> BoolIf implemented, we will adhere to whatever value the developer returns. |
|
Don’t know what the |
|
@simonseyer @akitchen moved to a delegate in c66becd. |
|
|
||
| super.init() | ||
|
|
||
| let monitorBatteryValue = delegate?.routeController?(self, willChangeBatteryMonitoringState: true) ?? true |
There was a problem hiding this comment.
delegate is always nil here.
|
@akitchen updated and exposed the delegate only when deinit is called now. |
| - returns: A bool representing the value you would like `UIDevice.isBatteryMonitoringEnabled` set to. | ||
| */ | ||
| @objc(routeControllerWillDisableBatteryMonitoring:) | ||
| optional func routeControllerWillDisableBatteryMonitoring(_ routeController: RouteController) -> Bool |
There was a problem hiding this comment.
The logic here might be confusing. Typically, a delegate method with "will" or "should" in the name returns a boolean to indicate whether to proceed with the described action.
So, a false response to routeControllerWillDisableBatteryMonitoring() would mean don't disable it. This might be more clear if we change "will" to "should", but in either case I think the logic should be the other way around from how it is currently implemented here.
| @@ -316,7 +316,7 @@ open class RouteController: NSObject { | |||
| sendOutstandingFeedbackEvents(forceAll: true) | |||
| suspendNotifications() | |||
|
|
|||
| let monitorBatteryValue = delegate?.routeControllerWillDisableBatteryMonitoring?(self) ?? false | |||
| let monitorBatteryValue = delegate?.routeControllerShouldDisableBatteryMonitoring?(self) ?? false | |||
There was a problem hiding this comment.
Now the semantics are still wrong: If I return false because it shouldn't be disabled, it will actually be disabled and the other way around.
There was a problem hiding this comment.
Yes; it's not correct to pass this value into the next line. The heuristic should look like:
- If the delegate doesn't implement
routeControllerShouldDisableBatteryMonitoring(), go ahead and disable it. - If the delegate implements
routeControllerShouldDisableBatteryMonitoring()and returns true, disable it. - If the delegate implements
routeControllerShouldDisableBatteryMonitoring()and returns false, don't disable it.
| Implementing this method will allow developers to change whether battery monitoring is disabled when `RouteController` is deinited. | ||
|
|
||
| - parameter routeController: The route controller that will change the state of battery monitoring. | ||
| - returns: A bool representing the value you would like `UIDevice.isBatteryMonitoringEnabled` set to. |
There was a problem hiding this comment.
The return value here indicates whether to disable battery monitoring when the RouteController is deinited, not the value to set it to. If this method is not implemented, battery monitoring will be disabled when navigation ends.
With that doc change (or something close to it) I think we're good to merge this.
|
LGTM - thanks for the doc updates @bsudekum |
Addresses part 1 of #1474
If an app sets
UIDevice.isBatteryMonitoringEnabledon their own, we should not reset this property when initing/deiniting.This new prop makes it so developers can control whether this
UIDevice.isBatteryMonitoringEnabledis enabled internally./cc @mapbox/navigation-ios @simonseyer