From 4db6e68d79b00d3ae4a4cae400ab2dbe9cc30fe0 Mon Sep 17 00:00:00 2001 From: Alfonso Grillo Date: Fri, 21 Apr 2023 12:46:04 +0200 Subject: [PATCH] Refactor UserSetting property wrapper to support Codable (#814) * Add UserPreference property wrapper * Delete UserSetting property wrapper * Add PlistRepresentable support * Add more convenience inits * Add documentation * Add UTs * Fix flaky tests * Fix more flaky tests * Fix double optional problem in UserPreference * Refine UTs * Delete cache in UserPreference * Conform Date to PlistRepresentable --- .../Sources/Application/AppSettings.swift | 22 +- ElementX/Sources/Other/UserPreference.swift | 194 ++++++++++++++++++ .../Other/UserSettingPropertyWrapper.swift | 139 ------------- .../RoomScreen/View/Style/TimelineStyle.swift | 2 +- UnitTests/Sources/InvitesViewModelTests.swift | 4 +- .../Sources/RoomDetailsViewModelTests.swift | 4 +- UnitTests/Sources/UserPreferenceTests.swift | 168 +++++++++++++++ 7 files changed, 378 insertions(+), 155 deletions(-) create mode 100644 ElementX/Sources/Other/UserPreference.swift delete mode 100644 ElementX/Sources/Other/UserSettingPropertyWrapper.swift create mode 100644 UnitTests/Sources/UserPreferenceTests.swift diff --git a/ElementX/Sources/Application/AppSettings.swift b/ElementX/Sources/Application/AppSettings.swift index 8fe678b30..1f7bda484 100644 --- a/ElementX/Sources/Application/AppSettings.swift +++ b/ElementX/Sources/Application/AppSettings.swift @@ -26,7 +26,7 @@ final class AppSettings: ObservableObject { case enableInAppNotifications case pusherProfileTag case shouldCollapseRoomStateEvents - case startChatFlowEnabled = "showStartChatFlow" + case startChatFlowEnabled case startChatUserSuggestionsEnabled case mediaUploadingFlowEnabled case invitesFlowEnabled @@ -66,7 +66,7 @@ final class AppSettings: ObservableObject { /// The last known version of the app that was launched on this device, which is /// used to detect when migrations should be run. When `nil` the app may have been /// deleted between runs so should clear data in the shared container and keychain. - @UserSetting(key: UserDefaultsKeys.lastVersionLaunched.rawValue, defaultValue: nil, persistIn: store) + @UserPreference(key: UserDefaultsKeys.lastVersionLaunched, storageType: .userDefaults(store)) var lastVersionLaunched: String? /// The default homeserver address used. This is intentionally a string without a scheme @@ -126,24 +126,24 @@ final class AppSettings: ObservableObject { } /// `true` when the user has opted in to send analytics. - @UserSetting(key: UserDefaultsKeys.enableAnalytics.rawValue, defaultValue: false, persistIn: store) + @UserPreference(key: UserDefaultsKeys.enableAnalytics, defaultValue: false, storageType: .userDefaults(store)) var enableAnalytics // MARK: - Room Screen - @UserSettingRawRepresentable(key: UserDefaultsKeys.timelineStyle.rawValue, defaultValue: TimelineStyle.bubbles, persistIn: store) + @UserPreference(key: UserDefaultsKeys.timelineStyle, defaultValue: TimelineStyle.bubbles, storageType: .userDefaults(store)) var timelineStyle - @UserSetting(key: UserDefaultsKeys.shouldCollapseRoomStateEvents.rawValue, defaultValue: true, persistIn: nil) + @UserPreference(key: UserDefaultsKeys.shouldCollapseRoomStateEvents, defaultValue: true, storageType: .volatile) var shouldCollapseRoomStateEvents // MARK: - Notifications - @UserSetting(key: UserDefaultsKeys.timelineStyle.rawValue, defaultValue: true, persistIn: store) + @UserPreference(key: UserDefaultsKeys.timelineStyle, defaultValue: true, storageType: .userDefaults(store)) var enableInAppNotifications /// Tag describing which set of device specific rules a pusher executes. - @UserSetting(key: UserDefaultsKeys.pusherProfileTag.rawValue, defaultValue: nil, persistIn: store) + @UserPreference(key: UserDefaultsKeys.pusherProfileTag, storageType: .userDefaults(store)) var pusherProfileTag: String? // MARK: - Other @@ -154,19 +154,19 @@ final class AppSettings: ObservableObject { // MARK: Start Chat - @UserSetting(key: UserDefaultsKeys.startChatFlowEnabled.rawValue, defaultValue: false, persistIn: store) + @UserPreference(key: UserDefaultsKeys.startChatFlowEnabled, defaultValue: false, storageType: .userDefaults(store)) var startChatFlowEnabled - @UserSetting(key: UserDefaultsKeys.startChatUserSuggestionsEnabled.rawValue, defaultValue: false, persistIn: nil) + @UserPreference(key: UserDefaultsKeys.startChatUserSuggestionsEnabled, defaultValue: false, storageType: .volatile) var startChatUserSuggestionsEnabled // MARK: Media Uploading - @UserSetting(key: UserDefaultsKeys.mediaUploadingFlowEnabled.rawValue, defaultValue: false, persistIn: nil) + @UserPreference(key: UserDefaultsKeys.mediaUploadingFlowEnabled, defaultValue: false, storageType: .volatile) var mediaUploadingFlowEnabled // MARK: Invites - @UserSetting(key: UserDefaultsKeys.invitesFlowEnabled.rawValue, defaultValue: false, persistIn: store) + @UserPreference(key: UserDefaultsKeys.invitesFlowEnabled, defaultValue: false, storageType: .userDefaults(store)) var invitesFlowEnabled } diff --git a/ElementX/Sources/Other/UserPreference.swift b/ElementX/Sources/Other/UserPreference.swift new file mode 100644 index 000000000..8f5c8d076 --- /dev/null +++ b/ElementX/Sources/Other/UserPreference.swift @@ -0,0 +1,194 @@ +// +// Copyright 2023 New Vector Ltd +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Combine +import Foundation + +/// Property wrapper that allows to store data into a keyed storage. +/// It also exposes a Combine publisher for listening to value changes. +/// The publisher isn't supposed to skip consecutive duplicates if any, +/// there is no concept of Equatable at this level. +@propertyWrapper +final class UserPreference { + private let key: String + private var keyedStorage: any KeyedStorage + private let defaultValue: T + private let subject: PassthroughSubject = .init() + + init(key: String, defaultValue: T, keyedStorage: any KeyedStorage) { + self.key = key + self.defaultValue = defaultValue + self.keyedStorage = keyedStorage + } + + var wrappedValue: T { + get { + keyedStorage[key] ?? defaultValue + } + set { + keyedStorage[key] = newValue + subject.send(wrappedValue) + } + } + + var projectedValue: AnyPublisher { + subject + .prepend(wrappedValue) + .eraseToAnyPublisher() + } +} + +// MARK: - UserPreference convenience initializers + +extension UserPreference { + enum StorageType { + case userDefaults(UserDefaults = .standard) + case volatile + } + + convenience init(key: String, defaultValue: T, storageType: StorageType) { + let storage: any KeyedStorage + + switch storageType { + case .userDefaults(let userDefaults): + storage = UserDefaultsStorage(userDefaults: userDefaults) + case .volatile: + storage = [String: T]() + } + + self.init(key: key, defaultValue: defaultValue, keyedStorage: storage) + } + + convenience init(key: R, defaultValue: T, storageType: StorageType) where R.RawValue == String { + self.init(key: key.rawValue, defaultValue: defaultValue, storageType: storageType) + } + + convenience init(key: String, storageType: StorageType) where T: ExpressibleByNilLiteral { + self.init(key: key, defaultValue: nil, storageType: storageType) + } + + convenience init(key: R, storageType: StorageType) where R: RawRepresentable, R.RawValue == String, T: ExpressibleByNilLiteral { + self.init(key: key.rawValue, storageType: storageType) + } +} + +// MARK: - Storage + +protocol KeyedStorage { + associatedtype Value: Codable + + subscript(key: String) -> Value? { get set } +} + +/// An implementation of KeyedStorage on the UserDefaults. +/// +/// When used with a `Value` that conforms to `PlistRepresentable` the Codable encode/decode +/// phase is skipped, and values are stored natively in the plist. +final class UserDefaultsStorage: KeyedStorage { + private let userDefaults: UserDefaults + + init(userDefaults: UserDefaults) { + self.userDefaults = userDefaults + } + + subscript(key: String) -> Value? { + get { + let value: Value? + if Value.self is PlistRepresentable.Type { + value = decodePlistRepresentableValue(for: key) + } else { + value = decodeValue(for: key) + } + + return value + } + set { + if Value.self is PlistRepresentable.Type { + encodePlistRepresentable(value: newValue, for: key) + } else { + encode(value: newValue, for: key) + } + } + } + + private func decodeValue(for key: String) -> Value? { + userDefaults + .data(forKey: key) + .flatMap { + try? JSONDecoder().decode(Value.self, from: $0) + } + } + + private func decodePlistRepresentableValue(for key: String) -> Value? { + userDefaults.object(forKey: key) as? Value + } + + private func encode(value: Value?, for key: String) { + // Detects correctly double optionals like this: String?? = .some(nil) + if value.isNil { + userDefaults.removeObject(forKey: key) + } else { + let encodedValue = try? JSONEncoder().encode(value) + userDefaults.setValue(encodedValue, forKey: key) + } + } + + private func encodePlistRepresentable(value: Value?, for key: String) { + // Detects correctly double optionals like this: String?? = .some(nil) + if value.isNil { + userDefaults.removeObject(forKey: key) + } else { + userDefaults.set(value, forKey: key) + } + } +} + +private protocol Nullable { + var isNil: Bool { get } +} + +extension Optional: Nullable { + var isNil: Bool { + switch self { + case .none: + return true + case .some(let nullable as Nullable): + return nullable.isNil + case .some: + return false + } + } +} + +extension Dictionary: KeyedStorage where Key == String, Value: Codable { } + +// MARK: - PlistRepresentable + +/// A protocol to mark types as being plist compliant. +/// UserDefaultsStorage uses this protocol to avoid to encode/decode with Codable plist compliant values. +protocol PlistRepresentable { } + +extension Bool: PlistRepresentable { } +extension String: PlistRepresentable { } +extension Int: PlistRepresentable { } +extension Float: PlistRepresentable { } +extension Double: PlistRepresentable { } +extension Date: PlistRepresentable { } +extension Data: PlistRepresentable { } + +extension Array: PlistRepresentable where Element: PlistRepresentable { } +extension Dictionary: PlistRepresentable where Key == String, Value: PlistRepresentable { } +extension Optional: PlistRepresentable where Wrapped: PlistRepresentable { } diff --git a/ElementX/Sources/Other/UserSettingPropertyWrapper.swift b/ElementX/Sources/Other/UserSettingPropertyWrapper.swift deleted file mode 100644 index e6dba5765..000000000 --- a/ElementX/Sources/Other/UserSettingPropertyWrapper.swift +++ /dev/null @@ -1,139 +0,0 @@ -// -// Copyright 2022 New Vector Ltd -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. -// - -// Taken from https://www.swiftbysundell.com/articles/property-wrappers-in-swift/ - -import Combine -import Foundation - -/// Property wrapper that allows transparent access to user defaults and exposes -/// a combine publisher for listening to value changes -/// -/// Please use `UserSettingRawRepresentable` for storing RawRepresentable values -@propertyWrapper -struct UserSetting { - private let key: String - private var defaultValue: Value - private let storage: UserDefaults? - private let publisher: CurrentValueSubject - - init(key: String, defaultValue: Value, persistIn storage: UserDefaults?) { - self.key = key - self.defaultValue = defaultValue - self.storage = storage - - let value = storage?.value(forKey: key) as? Value ?? defaultValue - publisher = CurrentValueSubject(value) - } - - var wrappedValue: Value { - get { - let value = storage?.value(forKey: key) as? Value - return value ?? defaultValue - } - set { - guard let storage else { - defaultValue = newValue - publisher.send(defaultValue) - return - } - - if let optional = newValue as? AnyOptional, optional.isNil { - storage.removeObject(forKey: key) - publisher.send(defaultValue) - } else { - storage.setValue(newValue, forKey: key) - publisher.send(newValue) - } - } - } - - var projectedValue: AnyPublisher { - publisher.removeDuplicates(by: { $0 == $1 }).eraseToAnyPublisher() - } -} - -extension UserSetting where Value: ExpressibleByNilLiteral { - init(key: String, persistIn storage: UserDefaults?) { - self.init(key: key, defaultValue: nil, persistIn: storage) - } -} - -/// Property wrapper that allows transparent access to user defaults for RawRepresentable types -/// and exposes a combine publisher for listening to value changes -/// -/// Tried extending UserSetting with RawRepresentable conformance but in that case the non-restricted -/// method takes precedence. Decided to go with with the simple solution instead of fighting the system -@propertyWrapper -struct UserSettingRawRepresentable { - private let key: String - private var defaultValue: Value - private let storage: UserDefaults? - private let publisher: CurrentValueSubject - - init(key: String, defaultValue: Value, persistIn storage: UserDefaults?) { - self.key = key - self.defaultValue = defaultValue - self.storage = storage - - let value = (storage?.value(forKey: key) as? Value.RawValue).flatMap { Value(rawValue: $0) } ?? defaultValue - publisher = CurrentValueSubject(value) - } - - var wrappedValue: Value { - get { - guard let value = storage?.value(forKey: key) as? Value.RawValue else { - return defaultValue - } - - return Value(rawValue: value) ?? defaultValue - } - set { - guard let storage else { - defaultValue = newValue - publisher.send(defaultValue) - return - } - - if let optional = newValue as? AnyOptional, optional.isNil { - storage.removeObject(forKey: key) - publisher.send(newValue) - } else { - storage.setValue(newValue.rawValue, forKey: key) - publisher.send(newValue) - } - } - } - - var projectedValue: AnyPublisher { - publisher.removeDuplicates(by: { $0 == $1 }).eraseToAnyPublisher() - } -} - -extension UserSettingRawRepresentable where Value: ExpressibleByNilLiteral { - init(key: String, persistIn storage: UserDefaults?) { - self.init(key: key, defaultValue: nil, persistIn: storage) - } -} - -// Casting to AnyOptional will fail for any types that are not Optional (below) -private protocol AnyOptional { - var isNil: Bool { get } -} - -extension Optional: AnyOptional { - var isNil: Bool { self == nil } -} diff --git a/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineStyle.swift b/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineStyle.swift index 815314174..12175a3e4 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineStyle.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/Style/TimelineStyle.swift @@ -17,7 +17,7 @@ import Foundation import SwiftUI -enum TimelineStyle: String, CaseIterable { +enum TimelineStyle: String, CaseIterable, Codable { case plain case bubbles diff --git a/UnitTests/Sources/InvitesViewModelTests.swift b/UnitTests/Sources/InvitesViewModelTests.swift index 831dadc16..dc3d29dff 100644 --- a/UnitTests/Sources/InvitesViewModelTests.swift +++ b/UnitTests/Sources/InvitesViewModelTests.swift @@ -34,14 +34,14 @@ class InvitesViewModelTests: XCTestCase { func testEmptyState() async throws { setupViewModel() - _ = await context.nextViewState() + _ = await context.$viewState.values.first(where: { $0.invites != nil }) let invites = try XCTUnwrap(context.viewState.invites) XCTAssertTrue(invites.isEmpty) } func testListState() async throws { setupViewModel(roomSummaries: .mockInvites) - _ = await context.nextViewState() + _ = await context.$viewState.values.first(where: { $0.invites != nil }) let invites = try XCTUnwrap(context.viewState.invites) XCTAssertEqual(invites.count, 2) } diff --git a/UnitTests/Sources/RoomDetailsViewModelTests.swift b/UnitTests/Sources/RoomDetailsViewModelTests.swift index d7dd6e0bf..c2a1e2db3 100644 --- a/UnitTests/Sources/RoomDetailsViewModelTests.swift +++ b/UnitTests/Sources/RoomDetailsViewModelTests.swift @@ -111,7 +111,7 @@ class RoomDetailsScreenViewModelTests: XCTestCase { func testIgnoreFailure() async throws { let recipient = RoomMemberProxyMock.mockDan recipient.ignoreUserClosure = { - try? await Task.sleep(for: .milliseconds(10)) + try? await Task.sleep(for: .milliseconds(100)) return .failure(.ignoreUserFailed) } let mockedMembers: [RoomMemberProxyMock] = [.mockMe, recipient] @@ -132,7 +132,7 @@ class RoomDetailsScreenViewModelTests: XCTestCase { func testUnignoreSuccess() async throws { let recipient = RoomMemberProxyMock.mockIgnored recipient.unignoreUserClosure = { - try? await Task.sleep(for: .milliseconds(10)) + try? await Task.sleep(for: .milliseconds(100)) return .success(()) } let mockedMembers: [RoomMemberProxyMock] = [.mockMe, recipient] diff --git a/UnitTests/Sources/UserPreferenceTests.swift b/UnitTests/Sources/UserPreferenceTests.swift new file mode 100644 index 000000000..15599debb --- /dev/null +++ b/UnitTests/Sources/UserPreferenceTests.swift @@ -0,0 +1,168 @@ +// +// Copyright 2023 New Vector Ltd +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +@testable import ElementX +import Foundation +import XCTest + +final class UserPreferenceTests: XCTestCase { + override func setUpWithError() throws { + UserDefaults.testDefaults.removeVolatileDomain(forName: .userDefaultsSuiteName) + } + + func testStorePlistValue() throws { + let setPreference = { + let value = TestPreferences() + value.plist = "Hello" + } + + setPreference() + + let value = TestPreferences() + + XCTAssertEqual(value.plist, "Hello") + XCTAssertNotNil(UserDefaults.testDefaults.string(forKey: .key2), "Hello") + XCTAssertNil(UserDefaults.testDefaults.data(forKey: .key2), "Hello") + } + + func testStoreCodableValue() throws { + let storedType = CodableTestType(a: "some", b: [1, 2, 3]) + + let setPreference = { + let value = TestPreferences() + value.codable = storedType + } + + setPreference() + + let value = TestPreferences() + + XCTAssertEqual(value.codable, storedType) + XCTAssertNotNil(UserDefaults.testDefaults.data(forKey: .key3)) + } + + func testStorePlistValueOnVolatileStorage() throws { + let setPreference = { + let value = TestPreferences() + value.volatileVar = "Hello" + } + + setPreference() + + let value = TestPreferences() + + XCTAssertNil(value.volatileVar) + } + + func testStoreCodableValueOnVolatileStorage() throws { + let storedType = CodableTestType(a: "some", b: [1, 2, 3]) + + let setPreference = { + let value = TestPreferences() + value.volatileCodable = storedType + } + + setPreference() + + let value = TestPreferences() + + XCTAssertNil(value.volatileCodable) + XCTAssertNil(UserDefaults.testDefaults.data(forKey: .key4)) + } + + func testStorePlistArray() throws { + let setPreference = { + let value = TestPreferences() + value.plistArray = [1, 2, 3] + } + + setPreference() + + let value = TestPreferences() + + XCTAssertEqual(value.plistArray, [1, 2, 3]) + XCTAssertEqual(UserDefaults.testDefaults.array(forKey: .key5) as? [Int], [1, 2, 3]) + XCTAssertNil(UserDefaults.testDefaults.data(forKey: .key5), "Hello") + } + + func testAssignNilToPlistType() throws { + let setPreference = { + let value = TestPreferences() + value.plist = "Hello" + } + + setPreference() + + let value = TestPreferences() + value.plist = nil + + XCTAssertNil(value.plist) + XCTAssertNil(UserDefaults.testDefaults.string(forKey: .key2)) + } + + func testAssignNilToCodableType() throws { + let storedType = CodableTestType(a: "some", b: [1, 2, 3]) + + let setPreference = { + let value = TestPreferences() + value.codable = storedType + } + + setPreference() + + let value = TestPreferences() + value.codable = nil + + XCTAssertNil(value.codable) + XCTAssertNil(UserDefaults.testDefaults.data(forKey: .key3)) + } +} + +private struct TestPreferences { + @UserPreference(key: .key1, storageType: .volatile) + var volatileVar: String? + + @UserPreference(key: .key2, storageType: .userDefaults(.testDefaults)) + var plist: String? + + @UserPreference(key: .key3, storageType: .userDefaults(.testDefaults)) + var codable: CodableTestType? + + @UserPreference(key: .key4, storageType: .volatile) + var volatileCodable: CodableTestType? + + @UserPreference(key: .key5, storageType: .userDefaults(.testDefaults)) + var plistArray: [Int]? +} + +private struct CodableTestType: Equatable, Codable { + let a: String + let b: [Int] +} + +private extension String { + static let key1 = "foo.volatile" + static let key2 = "foo.plist" + static let key3 = "foo.codable" + static let key4 = "foo.volatile.codable" + static let key5 = "foo.plist.array" + static let userDefaultsSuiteName = "io.element.elementx.unitests" +} + +private extension UserDefaults { + // swiftlint:disable:next force_unwrapping + static let testDefaults = UserDefaults(suiteName: .userDefaultsSuiteName)! +}