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 5 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
122 changes: 82 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,17 @@ public class UBLocationManager: NSObject {
/// :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
}
}
}
Comment on lines +64 to +72
Copy link

Choose a reason for hiding this comment

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

Refactor the delegates method to improve readability by using a single return statement.

-        if onlyActive {
-            return delegateWrappers.values.compactMap {
-                if $0.wantsUpdate(for: usage, isBackground: appIsInBackground) {
-                    return $0.delegate
-                } else {
-                    return nil
-                }
-            }
-        } else {
-            return delegateWrappers.values.compactMap(\.delegate)
-        }
+        return delegateWrappers.values.compactMap { wrapper in
+            onlyActive && !wrapper.wantsUpdate(for: usage, isBackground: appIsInBackground) ? nil : wrapper.delegate
+        }

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 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 func delegates(onlyActive: Bool = false, usage: Set<LocationMonitoringUsage>? = nil) -> [UBLocationManagerDelegate] {
return delegateWrappers.values.compactMap { wrapper in
onlyActive && !wrapper.wantsUpdate(for: usage, isBackground: appIsInBackground) ? nil : wrapper.delegate
}
}

private var regionDelegates: [UBLocationManagerDelegate] {
delegateWrappers.values.filter { $0.usage.containsRegions }.compactMap(\.delegate)
}

private var permissionRequestCallback: ((LocationPermissionRequestResult) -> Void)?
Expand All @@ -80,13 +90,15 @@ public class UBLocationManager: NSObject {
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 +186,7 @@ public class UBLocationManager: NSObject {

/// 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 +285,7 @@ public class UBLocationManager: NSObject {
super.init()

setupLocationManager()
setupAppLifeCycleNotifications()
}

/// Applies the initial configuration for the location manager
Expand All @@ -285,11 +298,31 @@ public class UBLocationManager: NSObject {
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`)
///
/// - Parameters:
Expand All @@ -304,7 +337,7 @@ public class UBLocationManager: NSObject {
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 +346,7 @@ public class UBLocationManager: NSObject {
delegate.locationManager(self, requiresPermission: minimumAuthorizationLevelRequired)
return
}
startLocationMonitoringWithoutChecks(delegate)
startLocationMonitoringWithoutChecks(delegate, usage: usage)
case .denied,
.restricted:
stopLocationMonitoring()
Expand Down Expand Up @@ -345,20 +378,20 @@ public class UBLocationManager: NSObject {
/// 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 +402,37 @@ public class UBLocationManager: NSObject {
/// 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 +495,12 @@ public class UBLocationManager: NSObject {
}

/// :nodoc:
private func startLocationMonitoringWithoutChecks(_ delegate: UBLocationManagerDelegate) {
private func startLocationMonitoringWithoutChecks(_ delegate: UBLocationManagerDelegate, usage: Set<LocationMonitoringUsage>) {
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 +509,9 @@ public class UBLocationManager: NSObject {
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 +535,7 @@ public class UBLocationManager: NSObject {
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 +577,15 @@ extension UBLocationManager: CLLocationManagerDelegate {

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 +624,7 @@ extension UBLocationManager: CLLocationManagerDelegate {
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 +646,26 @@ extension UBLocationManager: CLLocationManagerDelegate {
}

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 regionDelegates {
delegate.locationManager(self, didEnterRegion: region)
}
}

public func locationManager(_ manager: CLLocationManager, didExitRegion region: CLRegion) {
for delegate in delegates {
for delegate in regionDelegates {
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 +674,7 @@ extension UBLocationManager: CLLocationManagerDelegate {
// 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 +751,11 @@ extension Set where Element == UBLocationManager.LocationMonitoringUsage {
}
}

/// :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