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

nil unsafe delegates and data sources at dealloc #2

Merged
merged 2 commits into from
Apr 21, 2015

Conversation

wooster
Copy link
Contributor

@wooster wooster commented Apr 15, 2015

This isn't a comprehensive review. There are probably some I missed.

System objects with unsafe references to their delegates may outlive the
lifetime of their delegates (eg when animating), sending messages to
deallocated instances. To be safe, if you assign your object as a delegate
to an unsafe delegate you also need to manually manage that unsafe
reference and prevent it from outliving your object.

This isn't a comprehensive review. There are probably some I missed.

System objects with unsafe references to their delegates may outlive the
lifetime of their delegates (eg when animating), sending messages to
deallocated instances. To be safe, if you assign your object as a delegate
to an unsafe delegate you also need to manually manage that unsafe
reference and prevent it from outliving your object.
@@ -126,6 +127,7 @@ - (void)startTracking
if ([CLLocationManager locationServicesEnabled] == YES)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • (void)startTracking
    {
    if ([CLLocationManager locationServicesEnabled] == YES)
    {
    APCLogDebug(@"Start location tracking");
    •    self.locationManager.delegate = nil;
      
      self.locationManager = [[CLLocationManager alloc] init];
      self.locationManager.delegate = self;

just curious why you're allocating a whole new CLLocationManager every time you start and stop tracking, rather than creating it once and then starting/stopping SignificantLocationChanges as needed...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'd be a valid approach as well. Easy to just check if self.locationManager isn't nil if you wanted to do it that way.

Copy link
Member

Choose a reason for hiding this comment

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

Raised #7 for doing this in future.

@jwe-apple jwe-apple merged commit 2a75142 into ResearchKit:master Apr 21, 2015
@jwe-apple
Copy link
Member

Reviewed by Erin, merged.

jwe-apple pushed a commit that referenced this pull request May 22, 2015
Erin-Mounts added a commit to Erin-Mounts/AppCore that referenced this pull request Jun 24, 2015
YuanZhu-apple pushed a commit that referenced this pull request Jul 17, 2015
Refactor valueString methods
Erin-Mounts added a commit to Erin-Mounts/AppCore that referenced this pull request Jul 31, 2015
…en_eligible_to_email_verification

Add Share This App option in verify email
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants