From 9e5beff23ab0814d0f2c6a57c27faf45abf0b590 Mon Sep 17 00:00:00 2001 From: Davide Date: Sat, 16 Nov 2024 21:16:25 +0100 Subject: [PATCH] Perform migrate + import in one step (#882) - Drop the .importing / .imported steps - Animate rows re-sorting during process - Rephrase some strings better - Test fake migration with launch argument --- .../xcschemes/Passepartout.xcscheme | 5 +++ .../Views/Migration/MigrateButton.swift | 10 +---- .../Migration/MigrateContentView+List.swift | 12 ++++- .../Migration/MigrateContentView+Table.swift | 4 +- .../Views/Migration/MigrateContentView.swift | 13 ++++-- .../Views/Migration/MigrateView+Model.swift | 30 +++++-------- .../Views/Migration/MigrateView.swift | 45 +++++++------------ .../Views/Migration/MigrateViewStep.swift | 4 -- .../Business/MigrationManager.swift | 4 +- .../Domain/MigrationStatus.swift | 4 +- .../UILibrary/L10n/SwiftGen+Strings.swift | 10 ++--- .../Resources/en.lproj/Localizable.strings | 5 +-- Passepartout/Shared/AppContext+Shared.swift | 12 ++++- Passepartout/Shared/Shared.swift | 4 ++ 14 files changed, 77 insertions(+), 85 deletions(-) diff --git a/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme b/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme index 91e367c3..0e471e80 100644 --- a/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme +++ b/Passepartout.xcodeproj/xcshareddata/xcschemes/Passepartout.xcscheme @@ -111,6 +111,11 @@ value = "1" isEnabled = "NO"> + + diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateButton.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateButton.swift index a72851c9..8f37bf53 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateButton.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateButton.swift @@ -43,25 +43,19 @@ private extension MigrateButton { return Strings.Views.Migrate.Items.migrate case .migrating, .migrated: - return Strings.Views.Migrate.Items.import - - case .importing, .imported: return Strings.Global.done } } var isEnabled: Bool { switch step { - case .initial, .fetching, .migrating, .importing: + case .initial, .fetching, .migrating: return false case .fetched(let profiles): return !profiles.isEmpty - case .migrated(let profiles): - return !profiles.isEmpty - - case .imported: + case .migrated: return true } } diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+List.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+List.swift index c4de9d3f..04d000f3 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+List.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+List.swift @@ -141,7 +141,7 @@ private extension MigrateContentView.ListView { } } Spacer() - Button(isEditing ? Strings.Global.delete : Strings.Global.edit, role: isEditing ? .destructive : nil) { + Button(title, role: role) { if isEditing { if !selection.isEmpty { onDelete() @@ -157,6 +157,14 @@ private extension MigrateContentView.ListView { } .frame(height: 30) } + + var title: String { + isEditing ? Strings.Views.Migrate.Items.discard : Strings.Global.edit + } + + var role: ButtonRole? { + isEditing ? .destructive : nil + } } } @@ -269,7 +277,7 @@ private extension MigrateContentView.ListView { case .pending: ProgressView() - case .migrated, .imported: + case .done: ThemeImage(.marked) case .failed: diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+Table.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+Table.swift index 91dcb942..8ede4458 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+Table.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView+Table.swift @@ -82,7 +82,7 @@ private extension MigrateContentView.TableView { ) .environmentObject(theme) // TODO: #873, Table loses environment } - .width(20) + .width(30) TableColumn("") { profile in Button { onDelete([profile]) @@ -147,7 +147,7 @@ private extension MigrateContentView.TableView { case .pending: ThemeImage(.progress) - case .migrated, .imported: + case .done: ThemeImage(.marked) case .failed: diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView.swift index e3190904..f241ab09 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateContentView.swift @@ -70,7 +70,13 @@ struct MigrateContentView: View where PerformButton: View { extension Optional where Wrapped == MigrationStatus { var style: some ShapeStyle { - self != .excluded ? .primary : .secondary + switch self { + case .excluded, .failed: + return .secondary + + default: + return .primary + } } } @@ -101,9 +107,8 @@ extension Dictionary where Key == UUID, Value == MigrationStatus { initialStatuses: [ PrivatePreviews.profiles[0].id: .excluded, PrivatePreviews.profiles[1].id: .pending, - PrivatePreviews.profiles[2].id: .migrated, - PrivatePreviews.profiles[3].id: .imported, - PrivatePreviews.profiles[4].id: .failed + PrivatePreviews.profiles[2].id: .done, + PrivatePreviews.profiles[3].id: .failed ] ) .withMockEnvironment() diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView+Model.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView+Model.swift index 62e6d05e..92a8d10c 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView+Model.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView+Model.swift @@ -46,30 +46,14 @@ extension MigrateView { } extension MigrateView.Model { - - // XXX: filtering out the excluded rows may crash on macOS, because ThemeImage is - // momentarily removed from the hierarchy and loses access to the Theme - // .environmentObject(). this is certainly a SwiftUI bug - // - // https://github.com/passepartoutvpn/passepartout/pull/867#issuecomment-2476293204 - // var visibleProfiles: [MigratableProfile] { profiles -// .filter { -// switch step { -// case .initial, .fetching, .fetched: -// return true -// -// case .migrating, .migrated, .importing, .imported: -// return statuses[$0.id] != .excluded -// } -// } .sorted { switch step { case .initial, .fetching, .fetched: return $0.name.lowercased() < $1.name.lowercased() - case .migrating, .migrated, .importing, .imported: + case .migrating, .migrated: return (statuses[$0.id].rank, $0.name.lowercased()) < (statuses[$1.id].rank, $1.name.lowercased()) } } @@ -78,9 +62,15 @@ extension MigrateView.Model { private extension Optional where Wrapped == MigrationStatus { var rank: Int { - if self == .excluded { - return .max + switch self { + case .failed: + return 1 + + case .excluded: + return 2 + + default: + return .min } - return .min } } diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView.swift index 1d6aadb0..8bdc41f3 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateView.swift @@ -79,10 +79,10 @@ struct MigrateView: View { isEmpty: model.profiles.isEmpty, emptyMessage: Strings.Views.Migrate.noProfiles ) - .themeAnimation(on: model.step, category: .profiles) + .themeAnimation(on: model, category: .profiles) .themeConfirmation( isPresented: $isDeleting, - title: Strings.Views.Migrate.Alerts.Delete.title, + title: Strings.Views.Migrate.Items.discard, message: messageForDeletion, isDestructive: true, action: confirmPendingDeletion @@ -130,10 +130,7 @@ private extension MigrateView { case .fetched(let profiles): await migrate(profiles) - case .migrated(let profiles): - await save(profiles) - - case .imported: + case .migrated: dismiss() default: @@ -180,10 +177,20 @@ private extension MigrateView { do { pp_log(.App.migration, .notice, "Migrate \(profiles.count) profiles...") let profiles = try await migrationManager.migratedProfiles(profiles) { + guard $1 != .done else { + return + } model.statuses[$0] = $1 } - model.excludeFailed() - model.step = .migrated(profiles) + pp_log(.App.migration, .notice, "Mapped \(profiles.count) profiles to the new format, saving...") + await migrationManager.importProfiles(profiles, into: profileManager) { + model.statuses[$0] = $1 + } + let migrated = profiles.filter { + model.statuses[$0.id] == .done + } + pp_log(.App.migration, .notice, "Migrated \(migrated.count) profiles") + model.step = .migrated(migrated) } catch { pp_log(.App.migration, .error, "Unable to migrate profiles: \(error)") errorHandler.handle(error, title: title) @@ -191,28 +198,6 @@ private extension MigrateView { } } - func save(_ allProfiles: [Profile]) async { - guard case .migrated = model.step else { - assertionFailure("Must call migrate() and succeed, why is button enabled?") - return - } - - let profiles = allProfiles.filter { - model.statuses[$0.id] != .excluded - } - guard !profiles.isEmpty else { - assertionFailure("Nothing to import, why is button enabled?") - return - } - - model.step = .importing - pp_log(.App.migration, .notice, "Import \(profiles.count) migrated profiles...") - await migrationManager.importProfiles(profiles, into: profileManager) { - model.statuses[$0] = $1 - } - model.step = .imported - } - func confirmPendingDeletion() { guard let profilesPendingDeletion else { isEditing = false diff --git a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateViewStep.swift b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateViewStep.swift index c4d5d87c..da1d5048 100644 --- a/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateViewStep.swift +++ b/Passepartout/Library/Sources/AppUIMain/Views/Migration/MigrateViewStep.swift @@ -38,10 +38,6 @@ enum MigrateViewStep: Equatable { case migrated([Profile]) - case importing - - case imported - var canSelect: Bool { guard case .fetched = self else { return false diff --git a/Passepartout/Library/Sources/CommonLibrary/Business/MigrationManager.swift b/Passepartout/Library/Sources/CommonLibrary/Business/MigrationManager.swift index dd61e2da..d3220d24 100644 --- a/Passepartout/Library/Sources/CommonLibrary/Business/MigrationManager.swift +++ b/Passepartout/Library/Sources/CommonLibrary/Business/MigrationManager.swift @@ -82,7 +82,7 @@ extension MigrationManager { await onUpdate(migratable.id, .failed) return nil } - await onUpdate(migratable.id, .migrated) + await onUpdate(migratable.id, .done) return profile } catch { await onUpdate(migratable.id, .failed) @@ -115,7 +115,7 @@ extension MigrationManager { do { try await self.simulateBehavior() try await self.simulateSaveProfile(profile, manager: manager) - await onUpdate(profile.id, .imported) + await onUpdate(profile.id, .done) } catch { await onUpdate(profile.id, .failed) } diff --git a/Passepartout/Library/Sources/CommonLibrary/Domain/MigrationStatus.swift b/Passepartout/Library/Sources/CommonLibrary/Domain/MigrationStatus.swift index f445b3a7..43e4ef56 100644 --- a/Passepartout/Library/Sources/CommonLibrary/Domain/MigrationStatus.swift +++ b/Passepartout/Library/Sources/CommonLibrary/Domain/MigrationStatus.swift @@ -30,9 +30,7 @@ public enum MigrationStatus: Equatable { case pending - case migrated - - case imported + case done case failed } diff --git a/Passepartout/Library/Sources/UILibrary/L10n/SwiftGen+Strings.swift b/Passepartout/Library/Sources/UILibrary/L10n/SwiftGen+Strings.swift index 8d4df244..e74a4ad2 100644 --- a/Passepartout/Library/Sources/UILibrary/L10n/SwiftGen+Strings.swift +++ b/Passepartout/Library/Sources/UILibrary/L10n/SwiftGen+Strings.swift @@ -734,20 +734,18 @@ public enum Strings { public static func message(_ p1: Any) -> String { return Strings.tr("Localizable", "views.migrate.alerts.delete.message", String(describing: p1), fallback: "Do you want to discard these profiles? You will not be able to recover them later.\n\n%@") } - /// Discard - public static let title = Strings.tr("Localizable", "views.migrate.alerts.delete.title", fallback: "Discard") } } public enum Items { - /// Import - public static let `import` = Strings.tr("Localizable", "views.migrate.items.import", fallback: "Import") + /// Discard + public static let discard = Strings.tr("Localizable", "views.migrate.items.discard", fallback: "Discard") /// Proceed public static let migrate = Strings.tr("Localizable", "views.migrate.items.migrate", fallback: "Proceed") } public enum Sections { public enum Main { - /// Select below the profiles from old versions of Passepartout that you want to import. Profiles will disappear from this list once imported successfully. - public static let header = Strings.tr("Localizable", "views.migrate.sections.main.header", fallback: "Select below the profiles from old versions of Passepartout that you want to import. Profiles will disappear from this list once imported successfully.") + /// Select below the profiles from old versions of Passepartout that you want to import. They will disappear from this list once imported successfully. + public static let header = Strings.tr("Localizable", "views.migrate.sections.main.header", fallback: "Select below the profiles from old versions of Passepartout that you want to import. They will disappear from this list once imported successfully.") } } } diff --git a/Passepartout/Library/Sources/UILibrary/Resources/en.lproj/Localizable.strings b/Passepartout/Library/Sources/UILibrary/Resources/en.lproj/Localizable.strings index 7b139941..fd1d0df3 100644 --- a/Passepartout/Library/Sources/UILibrary/Resources/en.lproj/Localizable.strings +++ b/Passepartout/Library/Sources/UILibrary/Resources/en.lproj/Localizable.strings @@ -161,10 +161,9 @@ "views.migrate.title" = "Migrate"; "views.migrate.no_profiles" = "Nothing to migrate"; +"views.migrate.items.discard" = "Discard"; "views.migrate.items.migrate" = "Proceed"; -"views.migrate.items.import" = "Import"; -"views.migrate.sections.main.header" = "Select below the profiles from old versions of Passepartout that you want to import. Profiles will disappear from this list once imported successfully."; -"views.migrate.alerts.delete.title" = "Discard"; +"views.migrate.sections.main.header" = "Select below the profiles from old versions of Passepartout that you want to import. They will disappear from this list once imported successfully."; "views.migrate.alerts.delete.message" = "Do you want to discard these profiles? You will not be able to recover them later.\n\n%@"; "views.donate.title" = "Make a donation"; diff --git a/Passepartout/Shared/AppContext+Shared.swift b/Passepartout/Shared/AppContext+Shared.swift index 8f693149..e83cca85 100644 --- a/Passepartout/Shared/AppContext+Shared.swift +++ b/Passepartout/Shared/AppContext+Shared.swift @@ -101,7 +101,17 @@ extension AppContext { profilesContainerName: Constants.shared.containers.legacyV2, cloudKitIdentifier: BundleConfiguration.mainString(for: .legacyV2CloudKitId) ) - let migrationManager = MigrationManager(profileStrategy: profileStrategy) + let migrationSimulation: MigrationManager.Simulation? + if Configuration.Environment.isFakeMigration { + migrationSimulation = MigrationManager.Simulation( + fakeProfiles: true, + maxMigrationTime: 3.0, + randomFailures: true + ) + } else { + migrationSimulation = nil + } + let migrationManager = MigrationManager(profileStrategy: profileStrategy, simulation: migrationSimulation) return AppContext( iapManager: .sharedForApp, diff --git a/Passepartout/Shared/Shared.swift b/Passepartout/Shared/Shared.swift index 6045f9c8..b37334e7 100644 --- a/Passepartout/Shared/Shared.swift +++ b/Passepartout/Shared/Shared.swift @@ -106,6 +106,10 @@ extension Configuration.Environment { static var isFakeIAP: Bool { ProcessInfo.processInfo.environment["PP_FAKE_IAP"] == "1" } + + static var isFakeMigration: Bool { + ProcessInfo.processInfo.environment["PP_FAKE_MIGRATION"] == "1" + } } // MARK: ProfileManager