feature/AppStorage development is done #69
Conversation
| Button("Set Value") { | ||
| self.initalBool = !self.initalBool | ||
| } | ||
| }.padding()*/ |
There was a problem hiding this comment.
There is a lot of commented code. This can get confusing so it's better to remove if not needed.
| class AppStorageValueHolder<Value>{ | ||
| public var storage: UserDefaults = UserDefaults.standard | ||
| public var key = AppStorageDefaultKey.defaultKey | ||
| var getDataFromStorage:(()->Value) |
There was a problem hiding this comment.
this and many other properties are missing proper spacing. Example:
getDataFromStorage:(
should be
getDataFromStorage: (
()->Value
should be
() -> Value
| self.setDataInStorage(newValue) | ||
| } | ||
| } | ||
| init(value: Value , key:String,store: UserDefaults? = nil) where Value == Bool{ |
There was a problem hiding this comment.
This and other functions have irregular spacing. Please check them.
| } | ||
|
|
||
| } | ||
| @propertyWrapper public struct AppStorage<Value> : DynamicProperty { |
There was a problem hiding this comment.
DynamicProperty is used for auto populated wrappers. For AppStorage this is not needed.
|
|
||
| } | ||
| @propertyWrapper public struct AppStorage<Value> : DynamicProperty { | ||
| func update(context: Context) { |
There was a problem hiding this comment.
This function seems redundant so you can remove it with above comment.
| var _wrappedValue: AppStorageValueHolder<Value> | ||
| public var wrappedValue: Value{ | ||
| get{ | ||
| EnvironmentHolder.currentBodyViewBinderStack.last?.registerStateNotification(origin: _wrappedValue) |
There was a problem hiding this comment.
The origin of this state notification is simply an instance of the value holder inside this property wrapper. This means that only changes to this property wrapper will propagate view updates. If 2 or more @AppStorage property wrappers that point to the same property exist, the change in one of them won't notify the other.
In order to fix this, the origin of the notification needs to be a common object that is associated with the key, instead of just an instance created here. You can do this by creating a multiton, aka a static dictionary that holds identifier classes for each key.
After doing this, please add unit tests for this wrapper including the case I described above.
| } | ||
| } | ||
| self.setDataInStorage = { (value) in | ||
| if value == Optional<Int>.none { |
There was a problem hiding this comment.
If the value is nil, you should set nil inside user defaults, the same goes for all the other setter closures in all of the initializers
| } | ||
| let tempStorage = self.storage | ||
| self.getDataFromStorage = { () ->Value in | ||
| if let _ = tempStorage.value(forKey: key){ |
There was a problem hiding this comment.
you can do
tempStorage.value(forKey: key) != nil instead
| self.setDataInStorage = { (value) in | ||
| tempStorage.set(value, forKey: key) | ||
| } | ||
| guard let _ = self.storage.value(forKey: key) else { |
There was a problem hiding this comment.
is a bit strange to use a guard at the end of the function. It may be better to use
if storage.value(forKey:key) == nil
| tempStorage.set(value, forKey: key) | ||
| } | ||
| guard let _ = self.storage.value(forKey: key) else { | ||
| self.value = value |
There was a problem hiding this comment.
The value here should be a default value for this property wrapper, not for the user defaults itself. It's better to store this default value somewhere else, and keep the user defaults value the way it was (nil).
Finish AppStorage development