From f66193cf78fbc29197670098689c6b7ef141f56c Mon Sep 17 00:00:00 2001 From: Davide Date: Fri, 4 Oct 2024 20:58:11 +0200 Subject: [PATCH] Several fixes in ProfileManager (#685) - [x] Search not accounted for when reloading profiles - [x] Remote profile being saved twice - [x] Add some logging --- .../xcshareddata/swiftpm/Package.resolved | 2 +- .../xcschemes/Passepartout.xcscheme | 8 ++ Passepartout/Library/Package.swift | 2 +- .../AppLibrary/Business/ProfileManager.swift | 90 +++++++++++-------- .../AppUI/Business/ProfileEditor.swift | 3 +- .../Views/Settings/SettingsSectionGroup.swift | 2 +- fastlane/Fastfile | 14 +-- 7 files changed, 73 insertions(+), 48 deletions(-) diff --git a/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index 34bf07a7..a1b1332b 100644 --- a/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved +++ b/Passepartout.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved @@ -32,7 +32,7 @@ "kind" : "remoteSourceControl", "location" : "git@github.com:passepartoutvpn/passepartoutkit-source", "state" : { - "revision" : "779910e268e79f1004a95285ac2485255d88bb21" + "revision" : "f681f968f39ca514e29ac6c0abcf658c224e4c04" } }, { diff --git a/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme b/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme index 0a61ae41..68dce74f 100644 --- a/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme +++ b/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme @@ -59,6 +59,14 @@ argument = "-com.apple.CoreData.SQLDebug 1" isEnabled = "NO"> + + + + diff --git a/Passepartout/Library/Package.swift b/Passepartout/Library/Package.swift index 6618f1f6..eab5aed9 100644 --- a/Passepartout/Library/Package.swift +++ b/Passepartout/Library/Package.swift @@ -31,7 +31,7 @@ let package = Package( ], dependencies: [ // .package(url: "git@github.com:passepartoutvpn/passepartoutkit-source", from: "0.8.0"), - .package(url: "git@github.com:passepartoutvpn/passepartoutkit-source", revision: "779910e268e79f1004a95285ac2485255d88bb21"), + .package(url: "git@github.com:passepartoutvpn/passepartoutkit-source", revision: "f681f968f39ca514e29ac6c0abcf658c224e4c04"), // .package(path: "../../../passepartoutkit-source"), .package(url: "git@github.com:passepartoutvpn/passepartoutkit-source-openvpn-openssl", from: "0.8.0"), // .package(url: "git@github.com:passepartoutvpn/passepartoutkit-source-openvpn-openssl", revision: "031863a1cd683962a7dfe68e20b91fa820a1ecce"), diff --git a/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift b/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift index b798a014..7083208d 100644 --- a/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift +++ b/Passepartout/Library/Sources/AppLibrary/Business/ProfileManager.swift @@ -50,7 +50,7 @@ public final class ProfileManager: ObservableObject { private var allProfiles: [Profile.ID: Profile] { didSet { - reloadFilteredProfiles() + reloadFilteredProfiles(with: searchSubject.value) } } @@ -117,16 +117,36 @@ extension ProfileManager { } } - public func save(_ profile: Profile) async throws { + public func save(_ profile: Profile, isShared: Bool? = nil) async throws { + pp_log(.app, .notice, "Save profile \(profile.id)...") do { - try await beforeSave?(profile) - try await repository.saveEntities([profile]) - allProfiles[profile.id] = profile - didChange.send(.save(profile)) + if let existingProfile = allProfiles[profile.id], profile != existingProfile { + try await beforeSave?(profile) + try await repository.saveEntities([profile]) + + allProfiles[profile.id] = profile + didChange.send(.save(profile)) + } else { + pp_log(.app, .notice, "Profile \(profile.id) not modified, not saving") + } } catch { pp_log(.app, .fault, "Unable to save profile \(profile.id): \(error)") throw error } + do { + if let isShared, let remoteRepository { + if isShared { + pp_log(.app, .notice, "Enable remote sharing of profile \(profile.id)...") + try await remoteRepository.saveEntities([profile]) + } else { + pp_log(.app, .notice, "Disable remote sharing of profile \(profile.id)...") + try await remoteRepository.removeEntities(withIds: [profile.id]) + } + } + } catch { + pp_log(.app, .fault, "Unable to save/remove remote profile \(profile.id): \(error)") + throw error + } } public func remove(withId profileId: Profile.ID) async { @@ -134,14 +154,15 @@ extension ProfileManager { } public func remove(withIds profileIds: [Profile.ID]) async { + pp_log(.app, .notice, "Remove profiles \(profileIds)...") do { // remove local profiles var newAllProfiles = allProfiles try await repository.removeEntities(withIds: profileIds) + await afterRemove?(profileIds) profileIds.forEach { newAllProfiles.removeValue(forKey: $0) } - await afterRemove?(profileIds) // remove remote counterpart too try? await remoteRepository?.removeEntities(withIds: profileIds) @@ -169,22 +190,8 @@ extension ProfileManager { allRemoteProfiles.keys.contains(profileId) } - public func setRemotelyShared(_ shared: Bool, profileWithId profileId: Profile.ID) async throws { - guard let remoteRepository else { - pp_log(.app, .error, "Unable to share remotely when no remoteRepository is set") - return - } - guard let profile = allProfiles[profileId] else { - return - } - if shared { - try await remoteRepository.saveEntities([profile]) - } else { - try await remoteRepository.removeEntities(withIds: [profileId]) - } - } - - public func eraseRemoteProfiles() async throws { + public func eraseRemotelySharedProfiles() async throws { + pp_log(.app, .notice, "Erase remotely shared profiles...") try await remoteRepository?.removeEntities(withIds: Array(allRemoteProfiles.keys)) } } @@ -209,6 +216,7 @@ extension ProfileManager { var builder = profile.builder(withNewId: true) builder.name = firstUniqueName(from: profile.name) + pp_log(.app, .notice, "Duplicate profile [\(profileId), \(profile.name)] -> [\(builder.id), \(builder.name)]...") let copy = try builder.tryBuild() try await save(copy) @@ -239,7 +247,7 @@ extension ProfileManager { .first() .receive(on: DispatchQueue.main) .sink { [weak self] in - self?.notifyLocalEntities($0) + self?.reloadLocalProfiles($0) } .store(in: &subscriptions) @@ -247,7 +255,16 @@ extension ProfileManager { .entitiesPublisher .receive(on: DispatchQueue.main) .sink { [weak self] in - self?.notifyRemoteEntities($0) + self?.reloadRemoteProfiles($0) + } + .store(in: &subscriptions) + + remoteRepository? + .entitiesPublisher + .dropFirst() + .receive(on: DispatchQueue.main) + .sink { [weak self] in + self?.importRemoteProfiles($0) } .store(in: &subscriptions) @@ -261,26 +278,26 @@ extension ProfileManager { } private extension ProfileManager { - func notifyLocalEntities(_ result: EntitiesResult) { + func reloadLocalProfiles(_ result: EntitiesResult) { + pp_log(.app, .info, "Reload local profiles: \(result.entities.map(\.id))") allProfiles = result.entities.reduce(into: [:]) { $0[$1.id] = $1 } } - func notifyRemoteEntities(_ result: EntitiesResult) { - let isInitial = allRemoteProfiles.isEmpty + func reloadRemoteProfiles(_ result: EntitiesResult) { + pp_log(.app, .info, "Reload remote profiles: \(result.entities.map(\.id))") allRemoteProfiles = result.entities.reduce(into: [:]) { $0[$1.id] = $1 } objectWillChange.send() + } - // do not import on initial load - guard !isInitial else { - return - } + // pull remote updates into local profiles (best-effort) + func importRemoteProfiles(_ result: EntitiesResult) { + let profilesToImport = result.entities + pp_log(.app, .info, "Try to import remote profiles: \(result.entities.map(\.id))") - // pull remote updates into local profiles (best-effort) - let profilesToImport = allRemoteProfiles.values Task.detached { [weak self] in for remoteProfile in profilesToImport { do { @@ -294,14 +311,15 @@ private extension ProfileManager { } func performSearch(_ search: String) { + pp_log(.app, .notice, "Filter profiles with '\(search)'") reloadFilteredProfiles(with: search) } - func reloadFilteredProfiles(with search: String? = nil) { + func reloadFilteredProfiles(with search: String) { profiles = allProfiles .values .filter { - if let search, !search.isEmpty { + if !search.isEmpty { return $0.name.lowercased().contains(search.lowercased()) } return true diff --git a/Passepartout/Library/Sources/AppUI/Business/ProfileEditor.swift b/Passepartout/Library/Sources/AppUI/Business/ProfileEditor.swift index a7cf5822..5d669fbc 100644 --- a/Passepartout/Library/Sources/AppUI/Business/ProfileEditor.swift +++ b/Passepartout/Library/Sources/AppUI/Business/ProfileEditor.swift @@ -272,8 +272,7 @@ extension ProfileEditor { func save(to profileManager: ProfileManager) async throws { do { let newProfile = try build() - try await profileManager.save(newProfile) - try await profileManager.setRemotelyShared(isShared, profileWithId: newProfile.id) + try await profileManager.save(newProfile, isShared: isShared) } catch { pp_log(.app, .fault, "Unable to save edited profile: \(error)") throw error diff --git a/Passepartout/Library/Sources/AppUI/Views/Settings/SettingsSectionGroup.swift b/Passepartout/Library/Sources/AppUI/Views/Settings/SettingsSectionGroup.swift index a102b201..776912fc 100644 --- a/Passepartout/Library/Sources/AppUI/Views/Settings/SettingsSectionGroup.swift +++ b/Passepartout/Library/Sources/AppUI/Views/Settings/SettingsSectionGroup.swift @@ -86,7 +86,7 @@ private extension SettingsSectionGroup { Task { do { pp_log(.app, .info, "Erase CloudKit profiles...") - try await profileManager.eraseRemoteProfiles() + try await profileManager.eraseRemotelySharedProfiles() let containerId = BundleConfiguration.mainString(for: .cloudKitId) pp_log(.app, .info, "Erase CloudKit store with identifier \(containerId)...") diff --git a/fastlane/Fastfile b/fastlane/Fastfile index 54b185ba..efcf93b5 100644 --- a/fastlane/Fastfile +++ b/fastlane/Fastfile @@ -21,12 +21,6 @@ logname = "CHANGELOG.txt" desc "Bump version" lane :bump do |options| - version = options[:version] - build = options[:build] - increment_build_number(build_number: build) - unless version.nil? || version.empty? - increment_version_number_in_xcodeproj(version_number: version) - end unless options[:only] log = changelog_from_git_commits( pretty: "* %h %s", @@ -36,7 +30,13 @@ lane :bump do |options| File.open(path, "w") { |file| file.write(log) } - system("vim", path) + system("vim", path) or UI.user_error!("CHANGELOG editor cancelled") + end + version = options[:version] + build = options[:build] + increment_build_number(build_number: build) + unless version.nil? || version.empty? + increment_version_number_in_xcodeproj(version_number: version) end commit_version_bump( message: "Bump version",