Summarize internal UserDefaults observation class
#181
Conversation
|
As outlined in #136 (comment), I was looking to have class that handles the minimum amount of logic for the "low-level" observation handling for a single key, and then rather have another class that handles the high-level logic with multiple keys. It's generally beneficial to wrap low-level/legacy code in a wrapper to make it easier to reason about. |
hank121314
force-pushed
the
refactor/summarize-userdefaults-observation
branch
2 times, most recently
from
c9971b8
to
eecf057
Compare
Got your point! |
hank121314
force-pushed
the
refactor/summarize-userdefaults-observation
branch
from
eecf057
to
dfdddca
Compare
Sources/Defaults/Observation.swift
Outdated
|
|
||
| init(object: UserDefaults, key: String, callback: @escaping Callback) { | ||
| self.object = object | ||
| init(suite: NSObject, key: String) { |
There was a problem hiding this comment.
Why not use the specific type?
| init(suite: NSObject, key: String) { | |
| init(suite: UserDefaults, key: String) { |
Sources/Defaults/Observation.swift
Outdated
| private let callback: Callback | ||
| private var isObserving = false | ||
| final class SuiteKeyPair: Hashable { | ||
| weak var suite: NSObject? |
There was a problem hiding this comment.
| weak var suite: NSObject? | |
| weak var suite: UserDefaults? |
Sources/Defaults/Observation.swift
Outdated
| private weak var object: UserDefaults? | ||
| private let key: String | ||
| static var observationContext = 0 | ||
| private weak var suite: NSObject? |
There was a problem hiding this comment.
| private weak var suite: NSObject? | |
| private weak var suite: UserDefaults? |
Sources/Defaults/Observation.swift
Outdated
| func invalidate() { | ||
| if isObserving { | ||
| object?.removeObserver(self, forKeyPath: key, context: nil) | ||
| isObserving = false | ||
| guard isObserving else { | ||
| return | ||
| } | ||
|
|
||
| object = nil | ||
| suite?.removeObserver(self, forKeyPath: name) | ||
| isObserving = false | ||
| suite = nil | ||
| } |
There was a problem hiding this comment.
Unrelated to this specific pull request, but I wonder if this is thread-safe. It may be smart to put a lock on it?
There was a problem hiding this comment.
Put a lock on it. Maybe we should consider to use AtomicBoolean after swift 6?
There was a problem hiding this comment.
Maybe we should consider to use AtomicBoolean after swift 6?
👍
hank121314
force-pushed
the
refactor/summarize-userdefaults-observation
branch
from
2031332
to
fbf740b
Compare
|
Looks great now 👍 |
Description
This PR fixes: #170.
Summarize all internal
UserDefaultsrelated observation classes into one.Introduce a new class,
Defaults.UserDefaultsAnyKeysObservation, which can observe multiple keys and can add or remove the keys being observed(useful withDefaults.iCloud).