Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor timer code into separate utility class #53

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions MapboxMobileEvents.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@
AC611F281FCDED2F00ABBF6D /* MMEEventLogReportViewController.m in Sources */ = {isa = PBXBuildFile; fileRef = AC611F261FCDED2F00ABBF6D /* MMEEventLogReportViewController.m */; };
ACB6F21E1F7BF71B0032A916 /* MMELocationManagerTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = ACB6F21D1F7BF71B0032A916 /* MMELocationManagerTests.mm */; };
ACD86DA01F96A77700DD2A8D /* MMEAPIClientTests.mm in Sources */ = {isa = PBXBuildFile; fileRef = ACD86D9F1F96A77700DD2A8D /* MMEAPIClientTests.mm */; };
ACDB23FD20F416D100D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.h in Headers */ = {isa = PBXBuildFile; fileRef = ACDB23FB20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.h */; };
ACDB23FE20F416D100D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.m in Sources */ = {isa = PBXBuildFile; fileRef = ACDB23FC20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.m */; };
ACE4F01A1FD6102100035880 /* MMEUINavigation.h in Headers */ = {isa = PBXBuildFile; fileRef = ACE4F0181FD6102100035880 /* MMEUINavigation.h */; };
ACE4F01B1FD6102100035880 /* MMEUINavigation.m in Sources */ = {isa = PBXBuildFile; fileRef = ACE4F0191FD6102100035880 /* MMEUINavigation.m */; };
/* End PBXBuildFile section */
Expand Down Expand Up @@ -292,6 +294,8 @@
AC611F261FCDED2F00ABBF6D /* MMEEventLogReportViewController.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MMEEventLogReportViewController.m; sourceTree = "<group>"; };
ACB6F21D1F7BF71B0032A916 /* MMELocationManagerTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = MMELocationManagerTests.mm; sourceTree = "<group>"; };
ACD86D9F1F96A77700DD2A8D /* MMEAPIClientTests.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = MMEAPIClientTests.mm; sourceTree = "<group>"; };
ACDB23FB20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MMEBackgroundLocationServiceTimeoutHandler.h; sourceTree = "<group>"; };
ACDB23FC20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MMEBackgroundLocationServiceTimeoutHandler.m; sourceTree = "<group>"; };
ACE4F0181FD6102100035880 /* MMEUINavigation.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MMEUINavigation.h; sourceTree = "<group>"; };
ACE4F0191FD6102100035880 /* MMEUINavigation.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = MMEUINavigation.m; sourceTree = "<group>"; };
/* End PBXFileReference section */
Expand Down Expand Up @@ -395,6 +399,8 @@
40FB43761EF9EA7800EC5BC0 /* MMEEventsConfiguration.m */,
40C9E2791EA542BC00744FE7 /* MMEEventsManager.h */,
40C9E27A1EA542BC00744FE7 /* MMEEventsManager.m */,
ACDB23FB20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.h */,
ACDB23FC20F416D000D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.m */,
40C9E27D1EA5473F00744FE7 /* MMELocationManager.h */,
40C9E27E1EA5473F00744FE7 /* MMELocationManager.m */,
40FB437A1EFAFAE900EC5BC0 /* MMETimerManager.h */,
Expand Down Expand Up @@ -578,6 +584,7 @@
40F133541F20051E007B4096 /* NSData+MMEGZIP.h in Headers */,
40AE58431EA6C86C0046B437 /* MMEUIApplicationWrapper.h in Headers */,
40C611871F18319000E30A6C /* TSKPinningValidatorCallback.h in Headers */,
ACDB23FD20F416D100D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.h in Headers */,
40C611471F18319000E30A6C /* configuration_utils.h in Headers */,
40AE585A1EA9373F0046B437 /* MMEUniqueIdentifier.h in Headers */,
409115351F16BCD200F4250B /* MMECategoryLoader.h in Headers */,
Expand Down Expand Up @@ -797,6 +804,7 @@
40C611801F18319000E30A6C /* TrustKit.m in Sources */,
40C9E27C1EA542BC00744FE7 /* MMEEventsManager.m in Sources */,
40AE58571EA932DB0046B437 /* MMEConstants.m in Sources */,
ACDB23FE20F416D100D84DB1 /* MMEBackgroundLocationServiceTimeoutHandler.m in Sources */,
40AE586A1EA953970046B437 /* CLLocation+MMEMobileEvents.m in Sources */,
40AE58441EA6C86C0046B437 /* MMEUIApplicationWrapper.m in Sources */,
4036EFBF1ED37D56009C40BA /* MMEEventLogger.m in Sources */,
Expand Down
21 changes: 21 additions & 0 deletions MapboxMobileEvents/MMEBackgroundLocationServiceTimeoutHandler.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#import <Foundation/Foundation.h>

@class MMEBackgroundLocationServiceTimeoutHandler;
@protocol MMEUIApplicationWrapper;

@protocol MMEBackgroundLocationServiceTimeoutDelegate
- (BOOL)timeoutHandlerShouldCheckForTimeout:(MMEBackgroundLocationServiceTimeoutHandler *)handler;
- (void)timeoutHandlerDidTimeout:(MMEBackgroundLocationServiceTimeoutHandler *)handler;
- (void)timeoutHandlerBackgroundTaskDidExpire:(MMEBackgroundLocationServiceTimeoutHandler *)handler;

@end

@interface MMEBackgroundLocationServiceTimeoutHandler: NSObject

@property (nonatomic, weak) id<MMEBackgroundLocationServiceTimeoutDelegate> delegate;
@property (nonatomic, readonly) NSTimer *timer;
- (instancetype)initWithApplication:(id<MMEUIApplicationWrapper>)application;
- (void)startTimer;
- (void)stopTimer;
@end

87 changes: 87 additions & 0 deletions MapboxMobileEvents/MMEBackgroundLocationServiceTimeoutHandler.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#import "MMEBackgroundLocationServiceTimeoutHandler.h"
#import "MMEUIApplicationWrapper.h"

static const NSTimeInterval MMELocationManagerHibernationTimeout = 300.0;
static const NSTimeInterval MMELocationManagerHibernationPollInterval = 5.0;

@interface MMEBackgroundLocationServiceTimeoutHandler ()

@property (nonatomic) id<MMEUIApplicationWrapper> application;
@property (nonatomic) NSDate *expiration;
@property (nonatomic, readwrite) NSTimer *timer;
@property (nonatomic) UIBackgroundTaskIdentifier backgroundTaskId;

@end

#pragma mark - MMEBackgroundLocationServiceTimeoutHandler

@implementation MMEBackgroundLocationServiceTimeoutHandler

- (instancetype)initWithApplication:(id<MMEUIApplicationWrapper>)application {
self = [super init];
if (self) {
_application = application;
}
return self;
}

- (void)timeoutAllowedCheck:(NSTimer *)timer {
id<MMEBackgroundLocationServiceTimeoutDelegate> delegate = self.delegate;

if (!delegate) {
dispatch_async(dispatch_get_main_queue(), ^{
[self stopTimer];
});
return;
}

if (![delegate timeoutHandlerShouldCheckForTimeout:self]) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is an internal application, would it be good if we check first if the delegate already implements the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.expiration = [[NSDate date] dateByAddingTimeInterval:MMELocationManagerHibernationTimeout];
return;
}

if (!self.expiration) {
return;
}

NSTimeInterval timeIntervalSinceTimeoutAllowed = [[NSDate date] timeIntervalSinceDate:self.expiration];
if (timeIntervalSinceTimeoutAllowed > 0) {
dispatch_async(dispatch_get_main_queue(), ^{
[self stopTimer];
[delegate timeoutHandlerDidTimeout:self];
});
}
}

- (void)startTimer {
if (self.timer) {
// Changed from return, to ensure we get a new background task. This matches
// the previous behaviour (prior to refactoring)
[self stopTimer];
}

__weak __typeof__(self) weakself = self;
NSAssert(self.backgroundTaskId == UIBackgroundTaskInvalid, @"Background task Id should be invalid");
self.backgroundTaskId = [self.application beginBackgroundTaskWithExpirationHandler:^{
__typeof__(self) strongSelf = weakself;

[strongSelf stopTimer];
[strongSelf.delegate timeoutHandlerBackgroundTaskDidExpire:strongSelf];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

}];

self.expiration = [[NSDate date] dateByAddingTimeInterval:MMELocationManagerHibernationTimeout];
self.timer = [NSTimer scheduledTimerWithTimeInterval:MMELocationManagerHibernationPollInterval target:self selector:@selector(timeoutAllowedCheck:) userInfo:nil repeats:YES];
}

- (void)stopTimer {
if (UIBackgroundTaskInvalid != self.backgroundTaskId) {
[self.application endBackgroundTask:self.backgroundTaskId];
self.backgroundTaskId = UIBackgroundTaskInvalid;
}

[self.timer invalidate];
self.timer = nil;
self.expiration = nil;
}

@end
84 changes: 54 additions & 30 deletions MapboxMobileEvents/MMELocationManager.m
Original file line number Diff line number Diff line change
@@ -1,39 +1,44 @@
#import "MMELocationManager.h"
#import "MMEBackgroundLocationServiceTimeoutHandler.h"
#import "MMEUIApplicationWrapper.h"
#import "MMEDependencyManager.h"
#import "MMEEventsService.h"
#import "MMEEventsConfiguration.h"
#import <CoreLocation/CoreLocation.h>

static const NSTimeInterval MMELocationManagerHibernationTimeout = 300.0;
static const NSTimeInterval MMELocationManagerHibernationPollInterval = 5.0;

const CLLocationDistance MMELocationManagerDistanceFilter = 5.0;
const CLLocationDistance MMERadiusAccuracyMax = 300.0;

NSString * const MMELocationManagerRegionIdentifier = @"MMELocationManagerRegionIdentifier.fence.center";

@interface MMELocationManager () <CLLocationManagerDelegate>
@interface MMELocationManager () <CLLocationManagerDelegate, MMEBackgroundLocationServiceTimeoutDelegate>

@property (nonatomic) id<MMEUIApplicationWrapper> application;
@property (nonatomic) CLLocationManager *locationManager;
@property (nonatomic, getter=isUpdatingLocation, readwrite) BOOL updatingLocation;
@property (nonatomic) NSDate *backgroundLocationServiceTimeoutAllowedDate;
@property (nonatomic) NSTimer *backgroundLocationServiceTimeoutTimer;
@property (nonatomic) MMEBackgroundLocationServiceTimeoutHandler *backgroundLocationServiceTimeoutTimerWrapper;
@property (nonatomic) BOOL hostAppHasBackgroundCapability;
@property (nonatomic) MMEEventsConfiguration *configuration;

@end

@implementation MMELocationManager

- (void)dealloc {
_locationManager.delegate = nil;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we call stopUpdatingLocation here?

Copy link
Contributor Author

@julianrex julianrex Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[_backgroundLocationServiceTimeoutTimerWrapper stopTimer];
}

- (instancetype)init {
self = [super init];
if (self) {
_application = [[MMEUIApplicationWrapper alloc] init];
NSArray *backgroundModes = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"UIBackgroundModes"];
_hostAppHasBackgroundCapability = [backgroundModes containsObject:@"location"];
_configuration = [[MMEEventsService sharedService] configuration];

_backgroundLocationServiceTimeoutTimerWrapper = [[MMEBackgroundLocationServiceTimeoutHandler alloc] initWithApplication:_application];
_backgroundLocationServiceTimeoutTimerWrapper.delegate = self;
}
return self;
}
Expand All @@ -52,6 +57,10 @@ - (void)startUpdatingLocation {

- (void)stopUpdatingLocation {
if ([self isUpdatingLocation]) {

// Stop the timer
[self stopBackgroundTimeoutTimer];

[self.locationManager stopUpdatingLocation];
[self.locationManager stopMonitoringSignificantLocationChanges];
[self.locationManager stopMonitoringVisits];
Expand Down Expand Up @@ -85,6 +94,15 @@ - (void)setMetricsEnabledForInUsePermissions:(BOOL)metricsEnabledForInUsePermiss

#pragma mark - Utilities

- (void)setLocationManager:(CLLocationManager *)locationManager {
if (locationManager == _locationManager) {
return;
}

_locationManager.delegate = nil;
_locationManager = locationManager;
}

- (void)configurePassiveLocationManager {
self.locationManager.delegate = self;
self.locationManager.desiredAccuracy = kCLLocationAccuracyThreeKilometers;
Expand Down Expand Up @@ -132,31 +150,12 @@ - (void)startLocationServices {
}
}

- (void)timeoutAllowedCheck {
if (!self.isUpdatingLocation) {
return;
}

if (self.application.applicationState == UIApplicationStateActive ||
self.application.applicationState == UIApplicationStateInactive ) {
[self startBackgroundTimeoutTimer];
return;
}

NSTimeInterval timeIntervalSinceTimeoutAllowed = [[NSDate date] timeIntervalSinceDate:self.backgroundLocationServiceTimeoutAllowedDate];
if (timeIntervalSinceTimeoutAllowed > 0) {
[self.locationManager stopUpdatingLocation];
self.backgroundLocationServiceTimeoutAllowedDate = nil;
if ([self.delegate respondsToSelector:@selector(locationManagerBackgroundLocationUpdatesDidTimeout:)]) {
[self.delegate locationManagerBackgroundLocationUpdatesDidTimeout:self];
}
}
- (void)startBackgroundTimeoutTimer {
[self.backgroundLocationServiceTimeoutTimerWrapper startTimer];
}

- (void)startBackgroundTimeoutTimer {
[self.backgroundLocationServiceTimeoutTimer invalidate];
self.backgroundLocationServiceTimeoutAllowedDate = [[NSDate date] dateByAddingTimeInterval:MMELocationManagerHibernationTimeout];
self.backgroundLocationServiceTimeoutTimer = [NSTimer scheduledTimerWithTimeInterval:MMELocationManagerHibernationPollInterval target:self selector:@selector(timeoutAllowedCheck) userInfo:nil repeats:YES];
- (void)stopBackgroundTimeoutTimer {
[self.backgroundLocationServiceTimeoutTimerWrapper stopTimer];
}

- (void)establishRegionMonitoringForLocation:(CLLocation *)location {
Expand Down Expand Up @@ -202,10 +201,35 @@ - (void)locationManager:(CLLocationManager *)manager didVisit:(CLVisit *)visit {
}

- (void)locationManagerDidPauseLocationUpdates:(CLLocationManager *)locationManager {
// TODO: Should we stop the background timer here for completeness?
// [self stopBackgroundTimeoutTimer];

if ([self.delegate respondsToSelector:@selector(locationManagerBackgroundLocationUpdatesDidAutomaticallyPause:)]) {
[self.delegate locationManagerBackgroundLocationUpdatesDidAutomaticallyPause:self];
}
}

@end
#pragma mark - MMEBackgroundLocationServiceTimeoutDelegate

- (BOOL)timeoutHandlerShouldCheckForTimeout:(__unused MMEBackgroundLocationServiceTimeoutHandler *)handler {
return self.isUpdatingLocation && (self.application.applicationState == UIApplicationStateBackground);
}

- (void)timeoutHandlerDidTimeout:(__unused MMEBackgroundLocationServiceTimeoutHandler *)handler {
if ([self.delegate respondsToSelector:@selector(locationManagerBackgroundLocationUpdatesDidTimeout:)]) {
[self.delegate locationManagerBackgroundLocationUpdatesDidTimeout:self];
}

[self.locationManager stopUpdatingLocation];
}

- (void)timeoutHandlerBackgroundTaskDidExpire:(__unused MMEBackgroundLocationServiceTimeoutHandler *)handler {
// Do we need a delegate method here (i.e. do we need an event for background task expiry?)
NSAssert(!handler.timer, @"Timer should be nil by this point");

[self.locationManager stopUpdatingLocation];
}



@end
16 changes: 12 additions & 4 deletions MapboxMobileEvents/MMENamespacedDependencies.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
#define MMEAPIClient __NS_SYMBOL(MMEAPIClient)
#endif

#ifndef MMEBackgroundLocationServiceTimeoutHandler
#define MMEBackgroundLocationServiceTimeoutHandler __NS_SYMBOL(MMEBackgroundLocationServiceTimeoutHandler)
#endif

#ifndef MMECategoryLoader
#define MMECategoryLoader __NS_SYMBOL(MMECategoryLoader)
#endif
Expand Down Expand Up @@ -188,6 +192,10 @@
#define MMEAPIClient __NS_SYMBOL(MMEAPIClient)
#endif

#ifndef MMEBackgroundLocationServiceTimeoutDelegate
#define MMEBackgroundLocationServiceTimeoutDelegate __NS_SYMBOL(MMEBackgroundLocationServiceTimeoutDelegate)
#endif

#ifndef MMELocationManager
#define MMELocationManager __NS_SYMBOL(MMELocationManager)
#endif
Expand Down Expand Up @@ -584,10 +592,6 @@
#define MMELoggerShareableHTML __NS_SYMBOL(MMELoggerShareableHTML)
#endif

#ifndef kMMEReachabilityChangedNotification
#define kMMEReachabilityChangedNotification __NS_SYMBOL(kMMEReachabilityChangedNotification)
#endif

#ifndef MMELocationManagerDistanceFilter
#define MMELocationManagerDistanceFilter __NS_SYMBOL(MMELocationManagerDistanceFilter)
#endif
Expand All @@ -596,6 +600,10 @@
#define MMERadiusAccuracyMax __NS_SYMBOL(MMERadiusAccuracyMax)
#endif

#ifndef kMMEReachabilityChangedNotification
#define kMMEReachabilityChangedNotification __NS_SYMBOL(kMMEReachabilityChangedNotification)
#endif

#ifndef MMELocationManagerRegionIdentifier
#define MMELocationManagerRegionIdentifier __NS_SYMBOL(MMELocationManagerRegionIdentifier)
#endif
Expand Down