Review incorrect behavior in preferences (#989)

- 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

Library/Sources/AppDataPreferences/Strategy/CDModulePreferencesRepositoryV3.swift

@@ -87,14 +87,22 @@ private final class CDModulePreferencesRepositoryV3: ModulePreferencesRepository
     }
 
     func save() throws {
-        guard context.hasChanges else {
+        try context.performAndWait {
-            return
+            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
         }
     }
 }

Library/Sources/AppDataPreferences/Strategy/CDProviderPreferencesRepositoryV3.swift

@@ -112,14 +112,16 @@ private final class CDProviderPreferencesRepositoryV3: ProviderPreferencesReposi
     }
 
     func save() throws {
-        guard context.hasChanges else {
+        try context.performAndWait {
-            return
+            guard context.hasChanges else {
-        }
+                return
-        do {
+            }
-            try context.save()
+            do {
-        } catch {
+                try context.save()
-            context.rollback()
+            } catch {
-            throw error
+                context.rollback()
+                throw error
+            }
         }
     }
 }

Library/Sources/AppUIMain/Views/Modules/OpenVPNView+Configuration.swift

@@ -37,7 +37,7 @@ extension OpenVPNView {
         let credentialsRoute: (any Hashable)?
 
         @ObservedObject
-        var allowedEndpoints: Blacklist<ExtendedEndpoint>
+        var excludedEndpoints: ObservableList<ExtendedEndpoint>
 
         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<ExtendedEndpoint>
 
     @ObservedObject
-    var allowedEndpoints: Blacklist<ExtendedEndpoint>
+    var excludedEndpoints: ObservableList<ExtendedEndpoint>
 
     var body: some View {
         Button {
-            if allowedEndpoints.isAllowed(remote) {
+            if excludedEndpoints.contains(remote) {
-                if remaining.count > 1 {
+                excludedEndpoints.remove(remote)
-                    allowedEndpoints.deny(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<ExtendedEndpoint> {
         all.filter {
-            allowedEndpoints.isAllowed($0)
+            !excludedEndpoints.contains($0)
         }
     }
 }
@@ -340,11 +340,11 @@ private extension OpenVPNView.ConfigurationView {
     struct Preview: View {
 
         @StateObject
-        private var allowedEndpoints = Blacklist<ExtendedEndpoint> { _ in
+        private var excludedEndpoints = ObservableList<ExtendedEndpoint> { _ 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()

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 {
+    var excludedEndpoints: ObservableList<ExtendedEndpoint> {
-        editor.preferences(forModuleWithId: module.id, manager: preferencesManager)
-    }
-
-    var allowedEndpoints: Blacklist<ExtendedEndpoint> {
         if draft.wrappedValue.providerSelection != nil {
-            return providerPreferences.allowedEndpoints()
+            return providerPreferences.excludedEndpoints()
         } else {
-            return preferences.allowedEndpoints()
+            return preferences.excludedEndpoints()
         }
     }
 

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(
+        let savedProfile = try await profileEditor.save(to: profileManager)
-            to: profileManager,
-            preferencesManager: preferencesManager
-        )
         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(
+        try await profileEditor.save(to: profileManager)
-            to: profileManager,
-            preferencesManager: preferencesManager
-        )
         onDismiss()
     }
 

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 {

Library/Sources/CommonLibrary/Domain/ModulePreferences.swift

@@ -34,17 +34,13 @@ public final class ModulePreferences: ObservableObject {
     public init() {
     }
 
-    public func allowedEndpoints() -> Blacklist<ExtendedEndpoint> {
+    public func excludedEndpoints() -> ObservableList<ExtendedEndpoint> {
-        Blacklist { [weak self] in
+        ObservableList { [weak self] in
-            self?.repository?.isExcludedEndpoint($0) != true
+            self?.repository?.isExcludedEndpoint($0) == true
-        } allow: { [weak self] in
+        } add: { [weak self] in
-            self?.repository?.removeExcludedEndpoint($0)
-        } deny: { [weak self] in
             self?.repository?.addExcludedEndpoint($0)
+        } remove: { [weak self] in
+            self?.repository?.removeExcludedEndpoint($0)
         }
     }
-
-    public func save() throws {
-        try repository?.save()
-    }
 }

Library/Sources/CommonLibrary/Domain/ProviderPreferences.swift

@@ -44,13 +44,13 @@ public final class ProviderPreferences: ObservableObject {
         }
     }
 
-    public func allowedEndpoints() -> Blacklist<ExtendedEndpoint> {
+    public func excludedEndpoints() -> ObservableList<ExtendedEndpoint> {
-        Blacklist { [weak self] in
+        ObservableList { [weak self] in
-            self?.repository?.isExcludedEndpoint($0) != true
+            self?.repository?.isExcludedEndpoint($0) == true
-        } allow: { [weak self] in
+        } add: { [weak self] in
-            self?.repository?.removeExcludedEndpoint($0)
-        } deny: { [weak self] in
             self?.repository?.addExcludedEndpoint($0)
+        } remove: { [weak self] in
+            self?.repository?.removeExcludedEndpoint($0)
         }
     }
 

Library/Sources/CommonLibrary/Strategy/ModulePreferencesRepository.swift

@@ -34,4 +34,6 @@ public protocol ModulePreferencesRepository {
     func removeExcludedEndpoint(_ endpoint: ExtendedEndpoint)
 
     func save() throws
+
+    func discard()
 }

Library/Sources/CommonUtils/Business/CoreDataPersistentStore.swift

@@ -114,7 +114,7 @@ extension CoreDataPersistentStore {
         container.viewContext
     }
 
-    public var backgroundContext: NSManagedObjectContext {
+    public func backgroundContext() -> NSManagedObjectContext {
         container.newBackgroundContext()
     }
 

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<T>: ObservableObject where T: Equatable {
+public final class ObservableList<T>: ObservableObject where T: Equatable {
-    private let isAllowed: (T) -> Bool
+    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,
+        contains: @escaping (T) -> Bool,
-        allow: @escaping (T) -> Void,
+        add: @escaping (T) -> Void,
-        deny: @escaping (T) -> Void
+        remove: @escaping (T) -> Void
     ) {
-        self.isAllowed = isAllowed
+        self.contains = contains
-        self.allow = allow
+        self.add = add
-        self.deny = deny
+        self.remove = remove
     }
 
-    public func isAllowed(_ value: T) -> Bool {
+    public func contains(_ value: T) -> Bool {
-        isAllowed(value)
+        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)
     }
 }

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)
+        profilesRepository = CDProfileRepositoryV2(context: store.backgroundContext())
-        tvProfilesRepository = CDProfileRepositoryV2(context: tvStore.backgroundContext)
+        tvProfilesRepository = CDProfileRepositoryV2(context: tvStore.backgroundContext())
     }
 }
 

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)
+            let repository = try trackedPreferences[moduleId] ?? manager.preferencesRepository(forModuleWithId: moduleId)
-            trackedPreferences[moduleId] = observable
+            preferences.repository = repository
-            return observable
+            trackedPreferences[moduleId] = repository // @Published
         } catch {
             pp_log(.App.profiles, .error, "Unable to track preferences for module \(moduleId): \(error)")
-            return ModulePreferences()
         }
     }
 
     @discardableResult
-    public func save(
+    public func save(to profileManager: ProfileManager) async throws -> Profile {
-        to profileManager: ProfileManager,
-        preferencesManager: PreferencesManager
-    ) 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()
     }
 }
 

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])
     }
 }

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)
         }()
 

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
+                )
             }
         )
     }