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

Stop location monitoring when app goes to background for usages that don't require 'always' permission #64

Merged
merged 7 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 87 additions & 40 deletions Sources/UBLocation/UBLocationManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import CoreLocation
import UBFoundation
import UIKit
Copy link

Choose a reason for hiding this comment

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

Document the reason for importing UIKit, as it's not immediately clear from the context.

+ // UIKit is imported for handling app lifecycle events.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
import UIKit
// UIKit is imported for handling app lifecycle events.
import UIKit


/// An object defining methods that handle events related to GPS location.
public protocol UBLocationManagerDelegate: CLLocationManagerDelegate {
Expand Down Expand Up @@ -60,8 +61,14 @@
/// :nodoc:
private var delegateWrappers: [ObjectIdentifier: UBLocationManagerDelegateWrapper] = [:]

private var delegates: [UBLocationManagerDelegate] {
delegateWrappers.values.compactMap(\.delegate)
private func delegates(onlyActive: Bool = false, usage: Set<LocationMonitoringUsage>? = nil) -> [UBLocationManagerDelegate] {
delegateWrappers.values.compactMap { wrapper in
if onlyActive {
wrapper.wantsUpdate(for: usage, isBackground: appIsInBackground) ? wrapper.delegate : nil
} else {
wrapper.delegate
}
}
}

private var permissionRequestCallback: ((LocationPermissionRequestResult) -> Void)?
Expand All @@ -80,13 +87,15 @@
case showSettings
}

/// The union of the usages for all the delegaets
private var usage: Set<LocationMonitoringUsage> {
/// The union of the usages for all the delegates
private var allUsages: Set<LocationMonitoringUsage> {
delegateWrappers.values
.map(\.usage)
.reduce([]) { $0.union($1) }
}

private var appIsInBackground: Bool = UIApplication.shared.applicationState == .background
Copy link

Choose a reason for hiding this comment

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

Consider removing or guarding debug notifications with a compile-time flag to prevent them from being included in production builds.

-            LocalNotificationHelper.showDebugNotification(title: "Background notifyDelegates", body: "Delegate Count: \(delegates(onlyActive: true).count)")
+            #if DEBUG
+            LocalNotificationHelper.showDebugNotification(title: "Background notifyDelegates", body: "Delegate Count: \(delegates(onlyActive: true).count)")
+            #endif

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
private var appIsInBackground: Bool = UIApplication.shared.applicationState == .background
private var appIsInBackground: Bool = UIApplication.shared.applicationState == .background


/// Allows logging all the changes in authorization status, separately from any delegates
public var logLocationPermissionChange: ((CLAuthorizationStatus) -> Void)?
private var authorizationStatus: CLAuthorizationStatus? {
Expand Down Expand Up @@ -174,7 +183,7 @@

/// Does this location manager use the location in the background?
public var usesLocationInBackground: Bool {
usage.requiresBackgroundLocation
allUsages.requiresBackgroundLocation
}

/// The amount of seconds after which a location obtained by `CLLocationManager` should be considered stale
Expand Down Expand Up @@ -273,6 +282,7 @@
super.init()

setupLocationManager()
setupAppLifeCycleNotifications()
}

/// Applies the initial configuration for the location manager
Expand All @@ -285,9 +295,29 @@
locationManager.pausesLocationUpdatesAutomatically = false

// Only applies if the "Always" authorization is granted and `allowsBackgroundLocationUpdates`
if #available(iOS 11.0, *) {
locationManager.showsBackgroundLocationIndicator = true
locationManager.showsBackgroundLocationIndicator = true
}

private func setupAppLifeCycleNotifications() {
NotificationCenter.default.addObserver(self, selector: #selector(appDidEnterBackground), name: UIApplication.didEnterBackgroundNotification, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(appDidBecomeActive), name: UIApplication.didBecomeActiveNotification, object: nil)
}
Comment on lines +300 to +303
Copy link

Choose a reason for hiding this comment

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

Ensure app lifecycle notifications are properly removed to prevent memory leaks.

+ deinit {
+     NotificationCenter.default.removeObserver(self)
+ }

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
private func setupAppLifeCycleNotifications() {
NotificationCenter.default.addObserver(self, selector: #selector(appDidEnterBackground), name: UIApplication.didEnterBackgroundNotification, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(appDidBecomeActive), name: UIApplication.didBecomeActiveNotification, object: nil)
}
private func setupAppLifeCycleNotifications() {
NotificationCenter.default.addObserver(self, selector: #selector(appDidEnterBackground), name: UIApplication.didEnterBackgroundNotification, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(appDidBecomeActive), name: UIApplication.didBecomeActiveNotification, object: nil)
}
deinit {
NotificationCenter.default.removeObserver(self)
}


@objc private func appDidEnterBackground() {
appIsInBackground = true

if allUsages.containsLocation, !allUsages.contains(.backgroundLocation) {
locationManager.stopUpdatingLocation()
}
if allUsages.containsHeading, !allUsages.contains(.backgroundHeading) {
locationManager.stopUpdatingHeading()
}
}

@objc private func appDidBecomeActive() {
appIsInBackground = false

startLocationMonitoringForAllDelegates()
}

/// Start monitoring location service events (varies by `usage`)
Expand All @@ -304,7 +334,7 @@
let minimumAuthorizationLevelRequired = usage.minimumAuthorizationLevelRequired
switch authStatus {
case .authorizedAlways:
startLocationMonitoringWithoutChecks(delegate)
startLocationMonitoringWithoutChecks(delegate, usage: usage)
case .authorizedWhenInUse:
guard minimumAuthorizationLevelRequired == .whenInUse else {
if canAskForPermission {
Expand All @@ -313,7 +343,7 @@
delegate.locationManager(self, requiresPermission: minimumAuthorizationLevelRequired)
return
}
startLocationMonitoringWithoutChecks(delegate)
startLocationMonitoringWithoutChecks(delegate, usage: usage)
case .denied,
.restricted:
stopLocationMonitoring()
Expand Down Expand Up @@ -345,20 +375,20 @@
/// This method can be called as a safety measure to ensure location updates
/// A good place to call this method is a location button in map app
public func restartLocationMonitoring() {
if usage.containsLocation {
if allUsages.containsLocation {
locationManager.startUpdatingLocation()
}
if usage.contains(.significantChange), locationManager.significantLocationChangeMonitoringAvailable() {
if allUsages.contains(.significantChange), locationManager.significantLocationChangeMonitoringAvailable() {
locationManager.startMonitoringSignificantLocationChanges()
}
if usage.contains(.visits) {
if allUsages.contains(.visits) {
locationManager.startMonitoringVisits()
}
if usage.containsHeading {
if allUsages.containsHeading {
locationManager.startUpdatingHeading()
}
if usage.containsRegions {
for region in usage.regionsToMonitor {
if allUsages.containsRegions {
for region in allUsages.regionsToMonitor {
if !locationManager.monitoredRegions.contains(region) {
locationManager.startMonitoring(for: region)
}
Expand All @@ -369,37 +399,37 @@
/// Stops monitoring location service events and removes the delegate
public func stopLocationMonitoring(forDelegate delegate: UBLocationManagerDelegate) {
let id = ObjectIdentifier(delegate)
if let delegate = delegateWrappers.removeValue(forKey: id) {
stopLocationMonitoring(delegate.usage)
if let wrapper = delegateWrappers.removeValue(forKey: id) {
stopLocationMonitoring(wrapper.usage, delegate: delegate)
}

self.startLocationMonitoringForAllDelegates()

assert(!delegateWrappers.isEmpty || usage == [])
assert(!delegateWrappers.isEmpty || allUsages == [])
}

/// Stops monitoring all location service events
private func stopLocationMonitoring(_ usage: Set<LocationMonitoringUsage>? = nil) {
let usage = usage ?? self.usage
private func stopLocationMonitoring(_ usage: Set<LocationMonitoringUsage>? = nil, delegate: UBLocationManagerDelegate? = nil) {
let usg = usage ?? allUsages

timedOut = false
locationTimer?.invalidate()
locationTimer = nil

if usage.containsLocation {
if usg.containsLocation {
locationManager.stopUpdatingLocation()
}
if usage.contains(.significantChange), locationManager.significantLocationChangeMonitoringAvailable() {
if usg.contains(.significantChange), locationManager.significantLocationChangeMonitoringAvailable() {
locationManager.stopMonitoringSignificantLocationChanges()
}
if usage.contains(.visits) {
if usg.contains(.visits) {
locationManager.stopMonitoringVisits()
}
if usage.containsHeading {
if usg.containsHeading {
locationManager.stopUpdatingHeading()
}
if usage.containsRegions {
for region in usage.regionsToMonitor {
if usg.containsRegions {
for region in usg.regionsToMonitor {
locationManager.stopMonitoring(for: region)
}
}
Expand Down Expand Up @@ -462,10 +492,20 @@
}

/// :nodoc:
private func startLocationMonitoringWithoutChecks(_ delegate: UBLocationManagerDelegate) {
private func startLocationMonitoringWithoutChecks(_ delegate: UBLocationManagerDelegate, usage: Set<LocationMonitoringUsage>) {
guard locationManager.locationServicesEnabled() else {
let requiredAuthorizationLevel = usage.minimumAuthorizationLevelRequired
delegate.locationManager(self, requiresPermission: requiredAuthorizationLevel)
return
}

let id = ObjectIdentifier(delegate)

Check warning on line 502 in Sources/UBLocation/UBLocationManager.swift

View workflow job for this annotation

GitHub Actions / Build & Tests

initialization of immutable value 'id' was never used; consider replacing with assignment to '_' or removing it

if usage.containsLocation {
locationManager.startUpdatingLocation()
startLocationTimer()
if !appIsInBackground || usage.contains(.backgroundLocation) {
locationManager.startUpdatingLocation()
startLocationTimer()
}
}
if usage.contains(.significantChange), locationManager.significantLocationChangeMonitoringAvailable() {
locationManager.startMonitoringSignificantLocationChanges()
Expand All @@ -474,7 +514,9 @@
locationManager.startMonitoringVisits()
}
if usage.containsHeading {
locationManager.startUpdatingHeading()
if !appIsInBackground || usage.contains(.backgroundHeading) {
locationManager.startUpdatingHeading()
}
}
if usage.containsRegions {
for region in usage.regionsToMonitor {
Expand All @@ -498,7 +540,7 @@
private func startLocationFreshTimers() {
freshLocationTimers.forEach { $0.invalidate() }

freshLocationTimers = delegates.compactMap { delegate in
freshLocationTimers = delegates(onlyActive: true).compactMap { delegate in
guard let time = delegate.locationManagerMaxFreshAge else { return nil }

return Timer.scheduledTimer(withTimeInterval: time, repeats: false, block: { [weak self, weak delegate] _ in
Expand Down Expand Up @@ -540,15 +582,15 @@

self.startLocationMonitoringForAllDelegates()

if Self.hasRequiredAuthorizationLevel(forUsage: usage) {
if Self.hasRequiredAuthorizationLevel(forUsage: allUsages) {
let permission: AuthorizationLevel = authorization == .authorizedAlways ? .always : .whenInUse

for delegate in delegates {
for delegate in delegates() {
delegate.locationManager(self, grantedPermission: permission, accuracy: accuracyLevel)
}
}
if authorization == .denied {
for delegate in delegates {
for delegate in delegates() {
delegate.locationManager(permissionDeniedFor: self)
}
}
Expand Down Expand Up @@ -587,7 +629,7 @@
guard let lastLocation = locations.last else { return }
self.lastLocation = lastLocation

for delegate in delegates {
for delegate in delegates(onlyActive: true, usage: [.foregroundLocation, .backgroundLocation]) {
let filteredLocations: [CLLocation]
if let filteredAccuracy = delegate.locationManagerFilterAccuracy {
let targetAccuracy = (filteredAccuracy > 0 ? filteredAccuracy : 10)
Expand All @@ -609,26 +651,26 @@
}

public func locationManager(_: CLLocationManager, didVisit visit: CLVisit) {
for delegate in delegates {
for delegate in delegates(onlyActive: true, usage: [.visits]) {
delegate.locationManager(self, didVisit: visit)
}
}

public func locationManager(_ manager: CLLocationManager, didEnterRegion region: CLRegion) {
for delegate in delegates {
for delegate in delegates(onlyActive: true) {
delegate.locationManager(self, didEnterRegion: region)
}
}

public func locationManager(_ manager: CLLocationManager, didExitRegion region: CLRegion) {
for delegate in delegates {
for delegate in delegates(onlyActive: true) {
delegate.locationManager(self, didExitRegion: region)
}
}

public func locationManager(_: CLLocationManager, didUpdateHeading newHeading: CLHeading) {
lastHeading = newHeading
for delegate in delegates {
for delegate in delegates(onlyActive: true, usage: [.foregroundHeading, .backgroundHeading]) {
delegate.locationManager(self, didUpdateHeading: newHeading)
}
}
Expand All @@ -637,7 +679,7 @@
// This might be some temporary error. Just report it but do not stop
// monitoring as it could be some temporary error and we just have to
// wait for the next event
for delegate in delegates {
for delegate in delegates(onlyActive: true) {
delegate.locationManager(self, didFailWithError: error)
}
}
Expand Down Expand Up @@ -714,6 +756,11 @@
}
}

/// :nodoc:
var requiresBackgroundUpdates: Bool {
contains(.significantChange) || contains(.visits) || containsRegions || requiresBackgroundLocation
}

/// :nodoc:
var containsLocation: Bool {
contains(.foregroundLocation) || contains(.backgroundLocation)
Expand Down
10 changes: 10 additions & 0 deletions Sources/UBLocation/UBLocationManagerDelegateWrapper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,14 @@ class UBLocationManagerDelegateWrapper {
self.delegate = delegate
self.usage = usage
}

func wantsUpdate(for usg: Set<UBLocationManager.LocationMonitoringUsage>?, isBackground: Bool) -> Bool {
if let usg {
guard !usage.isDisjoint(with: usg) else { return false }
let union = usage.union(usg)
return isBackground ? union.requiresBackgroundUpdates : true
} else {
return isBackground ? usage.requiresBackgroundUpdates : true
}
}
Copy link

Choose a reason for hiding this comment

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

Add documentation to the wantsUpdate method to explain its purpose, parameters, and return value.

+    /// Determines whether an update is needed based on the specified usage and background status.
+    ///
+    /// - Parameters:
+    ///   - usg: The set of `LocationMonitoringUsage` to consider for the update.
+    ///   - isBackground: A Boolean value indicating whether the app is currently in the background.
+    /// - Returns: A Boolean value indicating whether an update is needed.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
func wantsUpdate(for usg: Set<UBLocationManager.LocationMonitoringUsage>?, isBackground: Bool) -> Bool {
if let usg {
guard !usage.isDisjoint(with: usg) else { return false }
let union = usage.union(usg)
return isBackground ? union.requiresBackgroundUpdates : true
} else {
return isBackground ? usage.requiresBackgroundUpdates : true
}
}
/// Determines whether an update is needed based on the specified usage and background status.
///
/// - Parameters:
/// - usg: The set of `LocationMonitoringUsage` to consider for the update.
/// - isBackground: A Boolean value indicating whether the app is currently in the background.
/// - Returns: A Boolean value indicating whether an update is needed.
func wantsUpdate(for usg: Set<UBLocationManager.LocationMonitoringUsage>?, isBackground: Bool) -> Bool {
if let usg {
guard !usage.isDisjoint(with: usg) else { return false }
let union = usage.union(usg)
return isBackground ? union.requiresBackgroundUpdates : true
} else {
return isBackground ? usage.requiresBackgroundUpdates : true
}
}

}
Loading