From 93a15cd7663310bf66f394b996864f18fa8e78c5 Mon Sep 17 00:00:00 2001 From: Davide Date: Mon, 9 Dec 2024 08:44:13 +0100 Subject: [PATCH] Review incorrect behavior in preferences (#989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Save/rollback was done outside of MOC - Use different contexts for module/provider preferences - Save providers → also saves modules - Discard modules → also discards providers - Use background context because it's not automatically merged (can rollback) - Expose ModulePreferences in OpenVPNView as StateObject - Rework Blacklist to a more reusable ObservableList - Reapply #988 --- .../CDModulePreferencesRepositoryV3.swift | 20 +++++++---- .../CDProviderPreferencesRepositoryV3.swift | 18 +++++----- .../Modules/OpenVPNView+Configuration.swift | 28 +++++++-------- .../AppUIMain/Views/Modules/OpenVPNView.swift | 24 ++++++++----- .../Views/Profile/ProfileCoordinator.swift | 10 ++---- .../Business/PreferencesManager.swift | 3 ++ .../Domain/ModulePreferences.swift | 16 ++++----- .../Domain/ProviderPreferences.swift | 12 +++---- .../ModulePreferencesRepository.swift | 2 ++ .../Business/CoreDataPersistentStore.swift | 2 +- .../{Blacklist.swift => ObservableList.swift} | 34 +++++++++---------- .../Strategy/ProfileV2MigrationStrategy.swift | 4 +-- .../UILibrary/Business/ProfileEditor.swift | 26 ++++++++------ .../UILibraryTests/ProfileEditorTests.swift | 2 +- .../App/Context/AppContext+Shared.swift | 2 +- .../Dependencies+PreferencesManager.swift | 10 ++++-- 16 files changed, 118 insertions(+), 95 deletions(-) rename Library/Sources/CommonUtils/Business/{Blacklist.swift => ObservableList.swift} (63%) diff --git a/Library/Sources/AppDataPreferences/Strategy/CDModulePreferencesRepositoryV3.swift b/Library/Sources/AppDataPreferences/Strategy/CDModulePreferencesRepositoryV3.swift index f14a681d..7c28c4c1 100644 --- a/Library/Sources/AppDataPreferences/Strategy/CDModulePreferencesRepositoryV3.swift +++ b/Library/Sources/AppDataPreferences/Strategy/CDModulePreferencesRepositoryV3.swift @@ -87,14 +87,22 @@ private final class CDModulePreferencesRepositoryV3: ModulePreferencesRepository } func save() throws { - guard context.hasChanges else { - return + try context.performAndWait { + guard context.hasChanges else { + return + } + do { + try context.save() + } catch { + context.rollback() + throw error + } } - do { - try context.save() - } catch { + } + + func discard() { + context.performAndWait { context.rollback() - throw error } } } diff --git a/Library/Sources/AppDataPreferences/Strategy/CDProviderPreferencesRepositoryV3.swift b/Library/Sources/AppDataPreferences/Strategy/CDProviderPreferencesRepositoryV3.swift index 992d53b4..65231bc6 100644 --- a/Library/Sources/AppDataPreferences/Strategy/CDProviderPreferencesRepositoryV3.swift +++ b/Library/Sources/AppDataPreferences/Strategy/CDProviderPreferencesRepositoryV3.swift @@ -112,14 +112,16 @@ private final class CDProviderPreferencesRepositoryV3: ProviderPreferencesReposi } func save() throws { - guard context.hasChanges else { - return - } - do { - try context.save() - } catch { - context.rollback() - throw error + try context.performAndWait { + guard context.hasChanges else { + return + } + do { + try context.save() + } catch { + context.rollback() + throw error + } } } } diff --git a/Library/Sources/AppUIMain/Views/Modules/OpenVPNView+Configuration.swift b/Library/Sources/AppUIMain/Views/Modules/OpenVPNView+Configuration.swift index d18f08cb..db3b47fd 100644 --- a/Library/Sources/AppUIMain/Views/Modules/OpenVPNView+Configuration.swift +++ b/Library/Sources/AppUIMain/Views/Modules/OpenVPNView+Configuration.swift @@ -37,7 +37,7 @@ extension OpenVPNView { let credentialsRoute: (any Hashable)? @ObservedObject - var allowedEndpoints: Blacklist + var excludedEndpoints: ObservableList var body: some View { moduleSection(for: accountRows, header: Strings.Global.Nouns.account) @@ -75,7 +75,7 @@ private extension OpenVPNView.ConfigurationView { SelectableRemoteButton( remote: remote, all: Set(remotes), - allowedEndpoints: allowedEndpoints + excludedEndpoints: excludedEndpoints ) } .themeSection(header: Strings.Modules.Openvpn.remotes) @@ -89,16 +89,16 @@ private struct SelectableRemoteButton: View { let all: Set @ObservedObject - var allowedEndpoints: Blacklist + var excludedEndpoints: ObservableList var body: some View { Button { - if allowedEndpoints.isAllowed(remote) { - if remaining.count > 1 { - allowedEndpoints.deny(remote) - } + if excludedEndpoints.contains(remote) { + excludedEndpoints.remove(remote) } else { - allowedEndpoints.allow(remote) + if remaining.count > 1 { + excludedEndpoints.add(remote) + } } } label: { HStack { @@ -112,7 +112,7 @@ private struct SelectableRemoteButton: View { } Spacer() ThemeImage(.marked) - .opaque(allowedEndpoints.isAllowed(remote)) + .opaque(!excludedEndpoints.contains(remote)) } .contentShape(.rect) } @@ -121,7 +121,7 @@ private struct SelectableRemoteButton: View { private var remaining: Set { all.filter { - allowedEndpoints.isAllowed($0) + !excludedEndpoints.contains($0) } } } @@ -340,11 +340,11 @@ private extension OpenVPNView.ConfigurationView { struct Preview: View { @StateObject - private var allowedEndpoints = Blacklist { _ in + private var excludedEndpoints = ObservableList { _ in true - } allow: { _ in + } add: { _ in // - } deny: { _ in + } remove: { _ in // } @@ -354,7 +354,7 @@ private extension OpenVPNView.ConfigurationView { isServerPushed: false, configuration: .forPreviews, credentialsRoute: nil, - allowedEndpoints: allowedEndpoints + excludedEndpoints: excludedEndpoints ) } .withMockEnvironment() diff --git a/Library/Sources/AppUIMain/Views/Modules/OpenVPNView.swift b/Library/Sources/AppUIMain/Views/Modules/OpenVPNView.swift index fd67724d..44b4af4c 100644 --- a/Library/Sources/AppUIMain/Views/Modules/OpenVPNView.swift +++ b/Library/Sources/AppUIMain/Views/Modules/OpenVPNView.swift @@ -51,6 +51,9 @@ struct OpenVPNView: View, ModuleDraftEditing { @State private var paywallReason: PaywallReason? + @StateObject + private var preferences = ModulePreferences() + @StateObject private var providerPreferences = ProviderPreferences() @@ -85,6 +88,13 @@ struct OpenVPNView: View, ModuleDraftEditing { .navigationDestination(for: Subroute.self, destination: destination) .themeAnimation(on: draft.wrappedValue.providerId, category: .modules) .withErrorHandler(errorHandler) + .onLoad { + editor.loadPreferences( + preferences, + from: preferencesManager, + forModuleWithId: module.id + ) + } } } @@ -99,7 +109,7 @@ private extension OpenVPNView { isServerPushed: isServerPushed, configuration: configuration, credentialsRoute: Subroute.credentials, - allowedEndpoints: allowedEndpoints + excludedEndpoints: excludedEndpoints ) } else { emptyConfigurationView @@ -175,7 +185,7 @@ private extension OpenVPNView { isServerPushed: false, configuration: configuration.builder(), credentialsRoute: nil, - allowedEndpoints: allowedEndpoints + excludedEndpoints: excludedEndpoints ) } .themeForm() @@ -199,15 +209,11 @@ private extension OpenVPNView { // MARK: - Logic private extension OpenVPNView { - var preferences: ModulePreferences { - editor.preferences(forModuleWithId: module.id, manager: preferencesManager) - } - - var allowedEndpoints: Blacklist { + var excludedEndpoints: ObservableList { if draft.wrappedValue.providerSelection != nil { - return providerPreferences.allowedEndpoints() + return providerPreferences.excludedEndpoints() } else { - return preferences.allowedEndpoints() + return preferences.excludedEndpoints() } } diff --git a/Library/Sources/AppUIMain/Views/Profile/ProfileCoordinator.swift b/Library/Sources/AppUIMain/Views/Profile/ProfileCoordinator.swift index 80a2c10c..912c2861 100644 --- a/Library/Sources/AppUIMain/Views/Profile/ProfileCoordinator.swift +++ b/Library/Sources/AppUIMain/Views/Profile/ProfileCoordinator.swift @@ -136,10 +136,7 @@ private extension ProfileCoordinator { // standard: always save, warn if purchase required func onCommitEditingStandard() async throws { - let savedProfile = try await profileEditor.save( - to: profileManager, - preferencesManager: preferencesManager - ) + let savedProfile = try await profileEditor.save(to: profileManager) do { try iapManager.verify(savedProfile) } catch AppError.ineligibleProfile(let requiredFeatures) { @@ -157,10 +154,7 @@ private extension ProfileCoordinator { paywallReason = .init(requiredFeatures) return } - try await profileEditor.save( - to: profileManager, - preferencesManager: preferencesManager - ) + try await profileEditor.save(to: profileManager) onDismiss() } diff --git a/Library/Sources/CommonLibrary/Business/PreferencesManager.swift b/Library/Sources/CommonLibrary/Business/PreferencesManager.swift index 18712a4f..53fcd228 100644 --- a/Library/Sources/CommonLibrary/Business/PreferencesManager.swift +++ b/Library/Sources/CommonLibrary/Business/PreferencesManager.swift @@ -85,6 +85,9 @@ private final class DummyModulePreferencesRepository: ModulePreferencesRepositor func save() throws { } + + func discard() { + } } private final class DummyProviderPreferencesRepository: ProviderPreferencesRepository { diff --git a/Library/Sources/CommonLibrary/Domain/ModulePreferences.swift b/Library/Sources/CommonLibrary/Domain/ModulePreferences.swift index a10b5b36..8186adc9 100644 --- a/Library/Sources/CommonLibrary/Domain/ModulePreferences.swift +++ b/Library/Sources/CommonLibrary/Domain/ModulePreferences.swift @@ -34,17 +34,13 @@ public final class ModulePreferences: ObservableObject { public init() { } - public func allowedEndpoints() -> Blacklist { - Blacklist { [weak self] in - self?.repository?.isExcludedEndpoint($0) != true - } allow: { [weak self] in - self?.repository?.removeExcludedEndpoint($0) - } deny: { [weak self] in + public func excludedEndpoints() -> ObservableList { + ObservableList { [weak self] in + self?.repository?.isExcludedEndpoint($0) == true + } add: { [weak self] in self?.repository?.addExcludedEndpoint($0) + } remove: { [weak self] in + self?.repository?.removeExcludedEndpoint($0) } } - - public func save() throws { - try repository?.save() - } } diff --git a/Library/Sources/CommonLibrary/Domain/ProviderPreferences.swift b/Library/Sources/CommonLibrary/Domain/ProviderPreferences.swift index b7f6aa6d..3afec295 100644 --- a/Library/Sources/CommonLibrary/Domain/ProviderPreferences.swift +++ b/Library/Sources/CommonLibrary/Domain/ProviderPreferences.swift @@ -44,13 +44,13 @@ public final class ProviderPreferences: ObservableObject { } } - public func allowedEndpoints() -> Blacklist { - Blacklist { [weak self] in - self?.repository?.isExcludedEndpoint($0) != true - } allow: { [weak self] in - self?.repository?.removeExcludedEndpoint($0) - } deny: { [weak self] in + public func excludedEndpoints() -> ObservableList { + ObservableList { [weak self] in + self?.repository?.isExcludedEndpoint($0) == true + } add: { [weak self] in self?.repository?.addExcludedEndpoint($0) + } remove: { [weak self] in + self?.repository?.removeExcludedEndpoint($0) } } diff --git a/Library/Sources/CommonLibrary/Strategy/ModulePreferencesRepository.swift b/Library/Sources/CommonLibrary/Strategy/ModulePreferencesRepository.swift index f6d0d1c2..be704164 100644 --- a/Library/Sources/CommonLibrary/Strategy/ModulePreferencesRepository.swift +++ b/Library/Sources/CommonLibrary/Strategy/ModulePreferencesRepository.swift @@ -34,4 +34,6 @@ public protocol ModulePreferencesRepository { func removeExcludedEndpoint(_ endpoint: ExtendedEndpoint) func save() throws + + func discard() } diff --git a/Library/Sources/CommonUtils/Business/CoreDataPersistentStore.swift b/Library/Sources/CommonUtils/Business/CoreDataPersistentStore.swift index ff74fd8a..c842897f 100644 --- a/Library/Sources/CommonUtils/Business/CoreDataPersistentStore.swift +++ b/Library/Sources/CommonUtils/Business/CoreDataPersistentStore.swift @@ -114,7 +114,7 @@ extension CoreDataPersistentStore { container.viewContext } - public var backgroundContext: NSManagedObjectContext { + public func backgroundContext() -> NSManagedObjectContext { container.newBackgroundContext() } diff --git a/Library/Sources/CommonUtils/Business/Blacklist.swift b/Library/Sources/CommonUtils/Business/ObservableList.swift similarity index 63% rename from Library/Sources/CommonUtils/Business/Blacklist.swift rename to Library/Sources/CommonUtils/Business/ObservableList.swift index 08485673..af47b681 100644 --- a/Library/Sources/CommonUtils/Business/Blacklist.swift +++ b/Library/Sources/CommonUtils/Business/ObservableList.swift @@ -1,5 +1,5 @@ // -// Blacklist.swift +// ObservableList.swift // Passepartout // // Created by Davide De Rosa on 12/8/24. @@ -26,34 +26,34 @@ import Foundation @MainActor -public final class Blacklist: ObservableObject where T: Equatable { - private let isAllowed: (T) -> Bool +public final class ObservableList: ObservableObject where T: Equatable { + private let contains: (T) -> Bool - private let allow: (T) -> Void + private let add: (T) -> Void - private let deny: (T) -> Void + private let remove: (T) -> Void public init( - isAllowed: @escaping (T) -> Bool, - allow: @escaping (T) -> Void, - deny: @escaping (T) -> Void + contains: @escaping (T) -> Bool, + add: @escaping (T) -> Void, + remove: @escaping (T) -> Void ) { - self.isAllowed = isAllowed - self.allow = allow - self.deny = deny + self.contains = contains + self.add = add + self.remove = remove } - public func isAllowed(_ value: T) -> Bool { - isAllowed(value) + public func contains(_ value: T) -> Bool { + contains(value) } - public func allow(_ value: T) { + public func add(_ value: T) { objectWillChange.send() - allow(value) + add(value) } - public func deny(_ value: T) { + public func remove(_ value: T) { objectWillChange.send() - deny(value) + remove(value) } } diff --git a/Library/Sources/LegacyV2/Strategy/ProfileV2MigrationStrategy.swift b/Library/Sources/LegacyV2/Strategy/ProfileV2MigrationStrategy.swift index d3a13804..2a67d5c1 100644 --- a/Library/Sources/LegacyV2/Strategy/ProfileV2MigrationStrategy.swift +++ b/Library/Sources/LegacyV2/Strategy/ProfileV2MigrationStrategy.swift @@ -66,8 +66,8 @@ public final class ProfileV2MigrationStrategy: ProfileMigrationStrategy, Sendabl cloudKitIdentifier: tvProfilesContainer.cloudKitIdentifier, author: nil ) - profilesRepository = CDProfileRepositoryV2(context: store.backgroundContext) - tvProfilesRepository = CDProfileRepositoryV2(context: tvStore.backgroundContext) + profilesRepository = CDProfileRepositoryV2(context: store.backgroundContext()) + tvProfilesRepository = CDProfileRepositoryV2(context: tvStore.backgroundContext()) } } diff --git a/Library/Sources/UILibrary/Business/ProfileEditor.swift b/Library/Sources/UILibrary/Business/ProfileEditor.swift index a2445fd1..30d5b52c 100644 --- a/Library/Sources/UILibrary/Business/ProfileEditor.swift +++ b/Library/Sources/UILibrary/Business/ProfileEditor.swift @@ -37,7 +37,8 @@ public final class ProfileEditor: ObservableObject { @Published public var isShared: Bool - private var trackedPreferences: [UUID: ModulePreferences] + @Published + private var trackedPreferences: [UUID: ModulePreferencesRepository] private(set) var removedModules: [UUID: any ModuleBuilder] @@ -207,23 +208,23 @@ extension ProfileEditor { removedModules = [:] } - public func preferences(forModuleWithId moduleId: UUID, manager: PreferencesManager) -> ModulePreferences { + public func loadPreferences( + _ preferences: ModulePreferences, + from manager: PreferencesManager, + forModuleWithId moduleId: UUID + ) { do { pp_log(.App.profiles, .debug, "Track preferences for module \(moduleId)") - let observable = try trackedPreferences[moduleId] ?? manager.preferences(forModuleWithId: moduleId) - trackedPreferences[moduleId] = observable - return observable + let repository = try trackedPreferences[moduleId] ?? manager.preferencesRepository(forModuleWithId: moduleId) + preferences.repository = repository + trackedPreferences[moduleId] = repository // @Published } catch { pp_log(.App.profiles, .error, "Unable to track preferences for module \(moduleId): \(error)") - return ModulePreferences() } } @discardableResult - public func save( - to profileManager: ProfileManager, - preferencesManager: PreferencesManager - ) async throws -> Profile { + public func save(to profileManager: ProfileManager) async throws -> Profile { do { let newProfile = try build() try await profileManager.save(newProfile, isLocal: true, remotelyShared: isShared) @@ -244,6 +245,11 @@ extension ProfileEditor { } public func discard() { + trackedPreferences.forEach { + pp_log(.App.profiles, .debug, "Discard tracked preferences for module \($0.key)") + $0.value.discard() + } + trackedPreferences.removeAll() } } diff --git a/Library/Tests/UILibraryTests/ProfileEditorTests.swift b/Library/Tests/UILibraryTests/ProfileEditorTests.swift index 15ce4182..cd7a6cc9 100644 --- a/Library/Tests/UILibraryTests/ProfileEditorTests.swift +++ b/Library/Tests/UILibraryTests/ProfileEditorTests.swift @@ -251,7 +251,7 @@ extension ProfileEditorTests { } .store(in: &subscriptions) - try await sut.save(to: manager, preferencesManager: PreferencesManager()) + try await sut.save(to: manager) await fulfillment(of: [exp]) } } diff --git a/Passepartout/App/Context/AppContext+Shared.swift b/Passepartout/App/Context/AppContext+Shared.swift index 05df53af..07ba7dcf 100644 --- a/Passepartout/App/Context/AppContext+Shared.swift +++ b/Passepartout/App/Context/AppContext+Shared.swift @@ -93,7 +93,7 @@ extension AppContext { cloudKitIdentifier: nil, author: nil ) - let repository = AppData.cdProviderRepositoryV3(context: store.backgroundContext) + let repository = AppData.cdProviderRepositoryV3(context: store.backgroundContext()) return ProviderManager(repository: repository) }() diff --git a/Passepartout/Shared/Dependencies+PreferencesManager.swift b/Passepartout/Shared/Dependencies+PreferencesManager.swift index 7ae05ab8..f64a3148 100644 --- a/Passepartout/Shared/Dependencies+PreferencesManager.swift +++ b/Passepartout/Shared/Dependencies+PreferencesManager.swift @@ -42,10 +42,16 @@ extension Dependencies { ) return PreferencesManager( modulesFactory: { - try AppData.cdModulePreferencesRepositoryV3(context: preferencesStore.context, moduleId: $0) + try AppData.cdModulePreferencesRepositoryV3( + context: preferencesStore.backgroundContext(), + moduleId: $0 + ) }, providersFactory: { - try AppData.cdProviderPreferencesRepositoryV3(context: preferencesStore.context, providerId: $0) + try AppData.cdProviderPreferencesRepositoryV3( + context: preferencesStore.backgroundContext(), + providerId: $0 + ) } ) }