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

CLLocationManager warnings fix #414

Merged
merged 5 commits into from
Aug 22, 2024
Merged

CLLocationManager warnings fix #414

merged 5 commits into from
Aug 22, 2024

Conversation

ninarg
Copy link
Contributor

@ninarg ninarg commented Aug 20, 2024

Why?

We have a runtime warning when using CLLocationManager's locationServicesEnabled()

Screenshot 2024-08-20 at 08 43 17

What?

According to Apple's documentation, we should use a delegate function locationManagerDidChangeAuthorization(_:) to check the app's initial authorization state. The system guarantees to call it after init of CLLocationManager.

Added a new LocationService to share logic between polygon and radius filter. The users of it can now get the authorization status, but the difference from before is that it's now async, since we have to wait for the delegate function to be called first.

Show me

No UI change.

@ninarg ninarg requested review from a team, teddyfahi and Breiby and removed request for a team August 20, 2024 07:04
Copy link
Member

@Breiby Breiby left a comment

Choose a reason for hiding this comment

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

LGTM! 🙌

Sources/Charcoal/Filters/Map/LocationService.swift Outdated Show resolved Hide resolved
Comment on lines +18 to +20
return await withCheckedContinuation { continuation in
authorizationContinuations.append(continuation)
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we weakify self here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently it's not needed. Here's what ChatGPT says:

In this particular case, you do not need to weakly capture self inside the withCheckedContinuation block. Here’s why:

1. Continuation Blocks and Memory Management:

  • The withCheckedContinuation block will be executed immediately and is not stored or retained beyond the duration of the call itself.
  • Since the continuation is handled immediately and there’s no possibility of a strong reference cycle being created (where self would keep the block alive and the block would keep self alive), there's no need to weakly capture self in this context.

2. Scope and Lifetime:

  • The self reference in the block is used to append the continuation to the authorizationContinuations array. Since the block is not stored and is only used in the context of this method, it won’t outlive the scope of the authorizationStatus method call.

3. Best Practices:

  • Weakly capturing self is necessary when the block is retained by self or by an object that could cause a retain cycle, but that’s not the case here. The continuation is a temporary construct used to manage the async/await behavior and doesn’t introduce a strong reference cycle.

Conclusion:
In this scenario, you can safely capture self strongly within the withCheckedContinuation block. There’s no risk of a retain cycle, and the continuation is not retained beyond the method scope.

If the withCheckedContinuation were capturing self for an asynchronous operation that might last longer than the method scope (for example, if you were passing a closure to another long-lived object that could retain it), then you would need to consider capturing self weakly. But in this specific case, it’s not necessary.

Handle future cases to enum

Co-authored-by: Benjamin Tegelaár-Breiby <Breiby@users.noreply.github.com>
@ninarg ninarg merged commit a8d0a9d into master Aug 22, 2024
2 checks passed
@ninarg ninarg deleted the locationmanager-warnings-fix branch August 22, 2024 06:39
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.

2 participants