Refactor timer code into separate utility class #53
Conversation
| @property (nonatomic) BOOL hostAppHasBackgroundCapability; | ||
| @property (nonatomic) MMEEventsConfiguration *configuration; | ||
|
|
||
| @end | ||
|
|
||
| @implementation MMELocationManager | ||
|
|
||
| - (void)dealloc { | ||
| _locationManager.delegate = nil; |
There was a problem hiding this comment.
Shouldn't we call stopUpdatingLocation here?
There was a problem hiding this comment.
This is a great question. I had all the stop... variants in here, but since the location manager is being deallocated (and its delegate is going too) I'm assuming* that there's no need to call these. It's not clear from the documentation, but I think it's a fair assumption.
Do you know?
*uh oh.
| return; | ||
| } | ||
|
|
||
| if (![delegate timeoutHandlerShouldCheckForTimeout:self]) { |
There was a problem hiding this comment.
While this is an internal application, would it be good if we check first if the delegate already implements the method?
There was a problem hiding this comment.
These aren't optional protocol methods, so there's no need to check to see if the delegate already implements it. The compiler will warn if the client hasn't implemented them.
Since these are really internal methods, I didn't see a need to make them optional.
| __typeof__(self) strongSelf = weakself; | ||
|
|
||
| [strongSelf stopTimer]; | ||
| [strongSelf.delegate timeoutHandlerBackgroundTaskDidExpire:strongSelf]; |
| @class MMEBackgroundLocationServiceTimeoutHandler; | ||
| @protocol MMEUIApplicationWrapper; | ||
|
|
||
| @protocol MMEBackgroundLocationServiceTimeoutHandlerDelegate |
There was a problem hiding this comment.
I hate this protocol name 😢 - any suggestions @rclee @fabian-guerra. Maybe replace HandlerDelegate with just one of Protocol, Delegate, Handler?
There was a problem hiding this comment.
MMEBackgroundLocationServiceTimeoutDelegate works for me.
|
Assigning to @rclee so that further tests can be run. |
|
Closing, in preference of a Please re-open if necessary. |
This PR moves the timer code out of
MMELocationManagerto avoid circular references.It also ensures that the location manager's
delegateproperty is set to nil to avoid a crash. (CLLocationManager'sdelegateproperty is declared asassignso needs to be manuallynil'd.)We may want to move this secondary bug fix into a separate PR, so we merge faster, but both of these changes are related to circular references so seemed appropriate.
@rclee I wanted to get this PR in, even though we're still discussing the related telemetry crash at #50.
Update:
Please note, there is a similar change we need to make in https://github.com/mapbox/mapbox-gl-native, both of these bugs could account for the above crash. (I'll update this PR with that change when I create it.)Update 2: The introduction of a custom location manager (via the
MGLCLLocationManagerprotocol in mapbox/mapbox-gl-native#12013) does away for the related change I mention above).