From 359c314cb745190be2d84dff43e8bdbca7361d94 Mon Sep 17 00:00:00 2001 From: Stefan Ceriu Date: Thu, 22 Dec 2022 10:59:12 +0200 Subject: [PATCH] Fixes #367 - Refactor AppSettings usage patterns --- ElementX.xcodeproj/project.pbxproj | 4 + .../Sources/Application/AppSettings.swift | 20 +-- .../Other/UserSettingPropertyWrapper.swift | 127 ++++++++++++++++++ .../Screens/RoomScreen/RoomScreenModels.swift | 2 + .../RoomScreen/RoomScreenViewModel.swift | 6 + .../Screens/RoomScreen/View/RoomScreen.swift | 3 +- .../Settings/SettingsScreenModels.swift | 3 +- .../Settings/SettingsScreenViewModel.swift | 19 ++- .../Settings/View/SettingsScreen.swift | 12 +- 9 files changed, 177 insertions(+), 19 deletions(-) create mode 100644 ElementX/Sources/Other/UserSettingPropertyWrapper.swift diff --git a/ElementX.xcodeproj/project.pbxproj b/ElementX.xcodeproj/project.pbxproj index 095af056f..dd8b49a18 100644 --- a/ElementX.xcodeproj/project.pbxproj +++ b/ElementX.xcodeproj/project.pbxproj @@ -443,6 +443,7 @@ D876EC0FED3B6D46C806912A /* AvatarSize.swift in Sources */ = {isa = PBXBuildFile; fileRef = E24B88AD3D1599E8CB1376E0 /* AvatarSize.swift */; }; D8CFF02C2730EE5BC4F17ABF /* ElementToggleStyle.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0960A7F5C1B0B6679BDF26F9 /* ElementToggleStyle.swift */; }; D9F80CE61BF8FF627FDB0543 /* LoadableImage.swift in Sources */ = {isa = PBXBuildFile; fileRef = C352359663A0E52BA20761EE /* LoadableImage.swift */; }; + DA4620936DA42CBE2524E1AE /* UserSettingPropertyWrapper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 68F8B8C529BF036E804B165E /* UserSettingPropertyWrapper.swift */; }; DBAA69CC2CE4D44BC8E20105 /* SettingsScreenModels.swift in Sources */ = {isa = PBXBuildFile; fileRef = 548E7D356609ACD33AE7643E /* SettingsScreenModels.swift */; }; DC68E866D6E664B0D2B06E74 /* MockImageCache.swift in Sources */ = {isa = PBXBuildFile; fileRef = AC1DA29A5A041CC0BACA7CB0 /* MockImageCache.swift */; }; DD9B70DE54B24E0694A35D8A /* Strings+Untranslated.swift in Sources */ = {isa = PBXBuildFile; fileRef = 1A18F6CE4D694D21E4EA9B25 /* Strings+Untranslated.swift */; }; @@ -777,6 +778,7 @@ 6654859746B0BE9611459391 /* cs */ = {isa = PBXFileReference; lastKnownFileType = text.plist.stringsdict; name = cs; path = cs.lproj/Localizable.stringsdict; sourceTree = ""; }; 667DD3A9D932D7D9EB380CAA /* sk */ = {isa = PBXFileReference; lastKnownFileType = text.plist.stringsdict; name = sk; path = sk.lproj/Localizable.stringsdict; sourceTree = ""; }; 66F2402D738694F98729A441 /* RoomTimelineProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RoomTimelineProvider.swift; sourceTree = ""; }; + 68F8B8C529BF036E804B165E /* UserSettingPropertyWrapper.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserSettingPropertyWrapper.swift; sourceTree = ""; }; 6920A4869821BF72FFC58842 /* MockMediaProvider.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockMediaProvider.swift; sourceTree = ""; }; 69219A908D7C22E6EE6689AE /* UserNotificationCenterSpy.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UserNotificationCenterSpy.swift; sourceTree = ""; }; 6A1AAC8EB2992918D01874AC /* rue */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = rue; path = rue.lproj/Localizable.strings; sourceTree = ""; }; @@ -2264,6 +2266,7 @@ 53482ECA4B6633961EC224F5 /* ScrollViewAdapter.swift */, BB3073CCD77D906B330BC1D6 /* Tests.swift */, 1F2529D434C750ED78ADF1ED /* UserAgentBuilder.swift */, + 68F8B8C529BF036E804B165E /* UserSettingPropertyWrapper.swift */, 44BBB96FAA2F0D53C507396B /* Extensions */, 8F9A844EB44B6AD7CA18FD96 /* HTMLParsing */, 06501F0E978B2D5C92771DC7 /* Logging */, @@ -3333,6 +3336,7 @@ 978BB24F2A5D31EE59EEC249 /* UserSessionProtocol.swift in Sources */, 7E91BAC17963ED41208F489B /* UserSessionStore.swift in Sources */, AC69B6DF15FC451AB2945036 /* UserSessionStoreProtocol.swift in Sources */, + DA4620936DA42CBE2524E1AE /* UserSettingPropertyWrapper.swift in Sources */, F07D88421A9BC4D03D4A5055 /* VideoRoomTimelineItem.swift in Sources */, 64F43D7390DA2A0AFD6BA911 /* VideoRoomTimelineView.swift in Sources */, 6FC10A00D268FCD48B631E37 /* ViewFrameReader.swift in Sources */, diff --git a/ElementX/Sources/Application/AppSettings.swift b/ElementX/Sources/Application/AppSettings.swift index 8c4fb3295..e0559f357 100644 --- a/ElementX/Sources/Application/AppSettings.swift +++ b/ElementX/Sources/Application/AppSettings.swift @@ -52,7 +52,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. - @AppStorage(UserDefaultsKeys.lastVersionLaunched.rawValue, store: store) + @UserSetting(key: UserDefaultsKeys.lastVersionLaunched.rawValue, defaultValue: nil, storage: store) var lastVersionLaunched: String? /// The default homeserver address used. This is intentionally a string without a scheme @@ -114,27 +114,27 @@ final class AppSettings: ObservableObject { } /// `true` when the user has opted in to send analytics. - @AppStorage(UserDefaultsKeys.enableAnalytics.rawValue, store: store) - var enableAnalytics = false + @UserSetting(key: UserDefaultsKeys.enableAnalytics.rawValue, defaultValue: false, storage: store) + var enableAnalytics /// Indicates if the device has already called identify for this session to PostHog. /// This is separate to `enableAnalytics` as logging out leaves analytics /// enabled, but requires the next account to be identified separately. - @AppStorage(UserDefaultsKeys.isIdentifiedForAnalytics.rawValue, store: store) - var isIdentifiedForAnalytics = false + @UserSetting(key: UserDefaultsKeys.isIdentifiedForAnalytics.rawValue, defaultValue: false, storage: store) + var isIdentifiedForAnalytics // MARK: - Room Screen - @AppStorage(UserDefaultsKeys.timelineStyle.rawValue, store: store) - var timelineStyle = TimelineStyle.bubbles + @UserSettingRawRepresentable(key: UserDefaultsKeys.timelineStyle.rawValue, defaultValue: TimelineStyle.bubbles, storage: store) + var timelineStyle // MARK: - Notifications - @AppStorage(UserDefaultsKeys.enableInAppNotifications.rawValue, store: store) - var enableInAppNotifications = true + @UserSetting(key: UserDefaultsKeys.timelineStyle.rawValue, defaultValue: true, storage: store) + var enableInAppNotifications /// Tag describing which set of device specific rules a pusher executes. - @AppStorage(UserDefaultsKeys.pusherProfileTag.rawValue, store: store) + @UserSetting(key: UserDefaultsKeys.pusherProfileTag.rawValue, defaultValue: nil, storage: store) var pusherProfileTag: String? // MARK: - Other diff --git a/ElementX/Sources/Other/UserSettingPropertyWrapper.swift b/ElementX/Sources/Other/UserSettingPropertyWrapper.swift new file mode 100644 index 000000000..d5483a786 --- /dev/null +++ b/ElementX/Sources/Other/UserSettingPropertyWrapper.swift @@ -0,0 +1,127 @@ +// +// 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 let defaultValue: Value + private let storage: UserDefaults + private let publisher: CurrentValueSubject + + init(key: String, defaultValue: Value, storage: UserDefaults = .standard) { + 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 { + 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, storage: UserDefaults = .standard) { + self.init(key: key, defaultValue: nil, storage: 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 let defaultValue: Value + private let storage: UserDefaults + private let publisher: CurrentValueSubject + + init(key: String, defaultValue: Value, storage: UserDefaults = .standard) { + 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 { + 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, storage: UserDefaults = .standard) { + self.init(key: key, defaultValue: nil, storage: 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/RoomScreenModels.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift index 39f198490..3f4e571b7 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenModels.swift @@ -64,6 +64,8 @@ struct RoomScreenViewState: BindableState { var canBackPaginate = true var isBackPaginating = false var showLoading = false + var timelineStyle: TimelineStyle + var bindings: RoomScreenViewStateBindings var contextMenuActionProvider: (@MainActor (_ itemId: String) -> TimelineItemContextMenuActions?)? diff --git a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift index ec18789c8..d14607a8f 100644 --- a/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift +++ b/ElementX/Sources/Screens/RoomScreen/RoomScreenViewModel.swift @@ -40,6 +40,7 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol super.init(initialViewState: RoomScreenViewState(roomId: timelineController.roomID, roomTitle: roomName ?? "Unknown room 💥", roomAvatarURL: roomAvatarUrl, + timelineStyle: ServiceLocator.shared.settings.timelineStyle, bindings: .init(composerText: "", composerFocused: false)), imageProvider: mediaProvider) @@ -78,6 +79,11 @@ class RoomScreenViewModel: RoomScreenViewModelType, RoomScreenViewModelProtocol return self.contextMenuActionsForItemId(itemId) } + ServiceLocator.shared.settings.$timelineStyle.sink { [weak self] timelineStyle in + self?.state.timelineStyle = timelineStyle + } + .store(in: &cancellables) + buildTimelineViews() } diff --git a/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift b/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift index c98a21d45..85d80a27c 100644 --- a/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift +++ b/ElementX/Sources/Screens/RoomScreen/View/RoomScreen.swift @@ -17,7 +17,6 @@ import SwiftUI struct RoomScreen: View { - @ObservedObject private var settings = ServiceLocator.shared.settings @ObservedObject var context: RoomScreenViewModel.Context @State private var showReactionsMenuForItemId = "" @@ -45,7 +44,7 @@ struct RoomScreen: View { TimelineView() .id(context.viewState.roomId) .environmentObject(context) - .timelineStyle(settings.timelineStyle) + .timelineStyle(context.viewState.timelineStyle) .overlay(alignment: .bottomTrailing) { scrollToBottomButton } } diff --git a/ElementX/Sources/Screens/Settings/SettingsScreenModels.swift b/ElementX/Sources/Screens/Settings/SettingsScreenModels.swift index 32d43c6f7..37cceab2d 100644 --- a/ElementX/Sources/Screens/Settings/SettingsScreenModels.swift +++ b/ElementX/Sources/Screens/Settings/SettingsScreenModels.swift @@ -35,7 +35,7 @@ struct SettingsScreenViewState: BindableState { } struct SettingsScreenViewStateBindings { - var enableAnalytics = ServiceLocator.shared.settings.enableAnalytics + var timelineStyle: TimelineStyle } enum SettingsScreenViewAction { @@ -44,4 +44,5 @@ enum SettingsScreenViewAction { case reportBug case sessionVerification case logout + case changedTimelineStyle } diff --git a/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift b/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift index 0302af43e..84dd26172 100644 --- a/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift +++ b/ElementX/Sources/Screens/Settings/SettingsScreenViewModel.swift @@ -14,6 +14,7 @@ // limitations under the License. // +import Combine import SwiftUI typealias SettingsScreenViewModelType = StateStoreViewModel @@ -25,13 +26,15 @@ class SettingsScreenViewModel: SettingsScreenViewModelType, SettingsScreenViewMo init(withUserSession userSession: UserSessionProtocol) { self.userSession = userSession - let bindings = SettingsScreenViewStateBindings() + let bindings = SettingsScreenViewStateBindings(timelineStyle: ServiceLocator.shared.settings.timelineStyle) super.init(initialViewState: .init(bindings: bindings, deviceID: userSession.deviceId, userID: userSession.userID, showSessionVerificationSection: !(userSession.sessionVerificationController?.isVerified ?? false)), imageProvider: userSession.mediaProvider) + listenToSettingsChange(publisher: ServiceLocator.shared.settings.$timelineStyle, keyPath: \.timelineStyle) + Task { if case let .success(userAvatarURL) = await userSession.clientProxy.loadUserAvatarURL() { state.userAvatarURL = userAvatarURL @@ -71,6 +74,20 @@ class SettingsScreenViewModel: SettingsScreenViewModelType, SettingsScreenViewMo callback?(.logout) case .sessionVerification: callback?(.sessionVerification) + case .changedTimelineStyle: + ServiceLocator.shared.settings.timelineStyle = state.bindings.timelineStyle } } + + private func listenToSettingsChange(publisher: AnyPublisher, + keyPath: WritableKeyPath) where Value: Equatable { + publisher.sink { [weak self] newValue in + guard newValue != self?.state.bindings[keyPath: keyPath] else { + return + } + + self?.state.bindings[keyPath: keyPath] = newValue + } + .store(in: &cancellables) + } } diff --git a/ElementX/Sources/Screens/Settings/View/SettingsScreen.swift b/ElementX/Sources/Screens/Settings/View/SettingsScreen.swift index bbf94a2e6..f43ec1d51 100644 --- a/ElementX/Sources/Screens/Settings/View/SettingsScreen.swift +++ b/ElementX/Sources/Screens/Settings/View/SettingsScreen.swift @@ -19,7 +19,6 @@ import SwiftUI struct SettingsScreen: View { @State private var showingLogoutConfirmation = false @Environment(\.colorScheme) private var colorScheme - @ObservedObject private var settings = ServiceLocator.shared.settings @ScaledMetric private var avatarSize = AvatarSize.user(on: .settings).value @ScaledMetric private var menuIconSize = 30.0 @@ -104,13 +103,16 @@ struct SettingsScreen: View { Section { SettingsPickerRow(title: ElementL10n.settingsTimelineStyle, image: Image(systemName: "rectangle.grid.1x2"), - selection: $settings.timelineStyle) { + selection: $context.timelineStyle) { ForEach(TimelineStyle.allCases, id: \.self) { style in - Text(style.description) + Text(style.name) .tag(style) } } .accessibilityIdentifier("timelineStylePicker") + .onChange(of: context.timelineStyle) { _ in + context.send(viewAction: .changedTimelineStyle) + } SettingsDefaultRow(title: ElementL10n.sendBugReport, image: Image(systemName: "questionmark.circle")) { @@ -166,8 +168,8 @@ struct SettingsScreen: View { } } -extension TimelineStyle: CustomStringConvertible { - var description: String { +private extension TimelineStyle { + var name: String { switch self { case .plain: return ElementL10n.roomTimelineStylePlainLongDescription