From f006f0512d6f9d8558bd41aac4529a8dfd306500 Mon Sep 17 00:00:00 2001 From: Shawn Schantz Date: Tue, 25 Aug 2026 09:25:49 -0400 Subject: [PATCH] feat(mac): remove dependence on name of .kmp file promote delete package code to main view to avoid race condition --- mac/Config/Config/ConfigApp.swift | 3 +- mac/Config/Config/MainConfigView.swift | 57 +++++++++++-- mac/Config/Config/PackageRowView.swift | 52 +++++------- .../Sources/KeymanSettings/PackageRepo.swift | 2 +- .../KeymanSettings/SettingsContainer.swift | 5 +- .../Persistence/PackageInstallHelper.swift | 83 ++++++++++--------- .../Persistence/PackageRepository.swift | 5 +- .../Tests/KeymanSettingsTests/RepoStubs.swift | 2 +- 8 files changed, 131 insertions(+), 78 deletions(-) diff --git a/mac/Config/Config/ConfigApp.swift b/mac/Config/Config/ConfigApp.swift index dd6a872dd6..637e0ee693 100644 --- a/mac/Config/Config/ConfigApp.swift +++ b/mac/Config/Config/ConfigApp.swift @@ -18,8 +18,9 @@ struct ConfigApp: App { var body: some Scene { Window("Configuration", id: "main-config") { MainConfigView() +// .background(Color(.underPageBackgroundColor)) .frame( - minWidth: 600, maxWidth: 800, + minWidth: 600, maxWidth: 1000, minHeight: 400, maxHeight: .infinity ) .environmentObject(settings) diff --git a/mac/Config/Config/MainConfigView.swift b/mac/Config/Config/MainConfigView.swift index 956c903d56..2bec05dc8d 100644 --- a/mac/Config/Config/MainConfigView.swift +++ b/mac/Config/Config/MainConfigView.swift @@ -26,6 +26,23 @@ struct MainConfigView: View { @State private var isShowingDropKmpAlert = false @State private var alertMessage = "" @State private var isHovering = false + + // item being targeted for deletion + @State private var idToDelete: UUID? = nil + + private var packageNameToDelete: String { + guard let uuid = idToDelete else { return "this item" } + guard let package = settings.findInstalledPackage(with: uuid) else { return "this item" } + return package.packageName + } + + @Environment(\.colorScheme) var colorScheme + var canvasColor: Color { + colorScheme == .dark ? Color(white: 0.12) : Color(white: 0.94) + } + var cardColor: Color { + colorScheme == .dark ? Color(white: 0.20) : Color(.white) + } /** * Assigns packageSelectedForHelpUrl the url argument and changes the selected tab to the help tab @@ -34,7 +51,7 @@ struct MainConfigView: View { packageSelectedForHelpUrl = url selectedTab = 1 } - + var body: some View { TabView (selection: $selectedTab) { VStack { @@ -56,16 +73,44 @@ struct MainConfigView: View { .frame(width: 960, height: 390) } - Form { + List { // the view for single keyboard packages - PackageRowView(packages: settings.singleKeyboardPackages, isSingleKeyboardPackage: true, expandedPackageID: $expandedPackageID, showHelpTab: { url in + PackageRowView(packages: settings.singleKeyboardPackages, isSingleKeyboardPackage: true, expandedPackageID: $expandedPackageID, + idToDelete: $idToDelete, showHelpTab: { url in showHelpTab(for: url)}) // the view for multi keyboard packages - PackageRowView(packages: settings.multiKeyboardPackages, isSingleKeyboardPackage: false, expandedPackageID: $expandedPackageID, showHelpTab: { url in + PackageRowView(packages: settings.multiKeyboardPackages, isSingleKeyboardPackage: false, expandedPackageID: $expandedPackageID, + idToDelete: $idToDelete, showHelpTab: { url in showHelpTab(for: url) }) } - .formStyle(.grouped) + .listStyle(.inset) + .confirmationDialog( + "Are you sure you want to delete the Keyman package '\(packageNameToDelete)'?", + isPresented: Binding( + get: { idToDelete != nil }, + set: { if !$0 { idToDelete = nil } } + ), + titleVisibility: .visible + ) { + Button("Delete", role: .destructive) { + if let uuid = idToDelete { + print("deleting package.id: \(uuid)") + // use multiple expanded states? + //expandedStates.removeValue(forKey: uuid) + + withAnimation(.easeInOut(duration: 0.3)) { + expandedPackageID = nil + settings.removeInstalledPackage(with: uuid) + } + } + idToDelete = nil // dismiss safely + } + + Button("Cancel", role: .cancel) { + idToDelete = nil + } + } // highlight border with accent color when hovering over view .overlay(RoundedRectangle(cornerRadius: 10).stroke(Color.accentColor, lineWidth: 2).opacity(isHovering ? 1 : 0)) .animation(.easeInOut(duration: 0.2), value: isHovering) @@ -106,6 +151,8 @@ struct MainConfigView: View { do { try settings.installPackage() } catch { + self.alertMessage = error.localizedDescription + self.isShowingDropKmpAlert = true print("failed to install package: \(helper.packageName ?? "unknown package") with error: \(error.localizedDescription)") } } else { diff --git a/mac/Config/Config/PackageRowView.swift b/mac/Config/Config/PackageRowView.swift index b4705151d1..460ab0e747 100644 --- a/mac/Config/Config/PackageRowView.swift +++ b/mac/Config/Config/PackageRowView.swift @@ -15,35 +15,35 @@ import KeymanSettings public struct PackageRowView: View { @EnvironmentObject var settings: SettingsContainer - // visibilty state for the delete package alert - @State private var isShowingDeleteAlert = false // used to identify the selected KeymanPackage for the delete package alert - @State private var selectedPackage: KeymanPackage? = nil - - // settings.singleKeyboardPackages or settings.multiKeyboardPackages + + @Environment(\.colorScheme) var colorScheme + var canvasColor: Color { + colorScheme == .dark ? Color(white: 0.12) : Color(white: 0.94) + } + var cardColor: Color { + colorScheme == .dark ? Color(white: 0.20) : Color(.white) + } + + // could be settings.singleKeyboardPackages or settings.multiKeyboardPackages let packages: [KeymanPackage] // a boolean for whether or not a package contains multiple keyboards let isSingleKeyboardPackage: Bool // binded to the shared state variable in the parent view @Binding var expandedPackageID: UUID? + @Binding var idToDelete: UUID? // closure passed from the parent view let showHelpTab: (URL) -> Void - init(packages: [KeymanPackage], isSingleKeyboardPackage: Bool, expandedPackageID: Binding, showHelpTab: @escaping (URL) -> Void) { + + init(packages: [KeymanPackage], isSingleKeyboardPackage: Bool, expandedPackageID: Binding, idToDelete: Binding, showHelpTab: @escaping (URL) -> Void) { self.packages = packages self.isSingleKeyboardPackage = isSingleKeyboardPackage self._expandedPackageID = expandedPackageID + self._idToDelete = idToDelete self.showHelpTab = showHelpTab } - /** - * Sets isShowingDeleteAlert to true and assigns the state variable selectedPackage the KeymanPackage argument - */ - public func showDeleteAlert(for package: KeymanPackage) { - isShowingDeleteAlert = true - selectedPackage = package - } - public var body: some View { ForEach(packages, id: \.id) { package in ForEach(isSingleKeyboardPackage ? package.keyboards : package.keyboards.onlyFirst) { keyboard in @@ -51,11 +51,13 @@ public struct PackageRowView: View { // the package info view is shown inside each disclosure group if expandedPackageID == package.id { PackageInfoView(package: package, showAlertFunction: { package in - showDeleteAlert(for: package) + idToDelete = package.id + //showDeleteAlert(for: package) }) .transition(.move(edge: .top)) } - } label: { + } + label: { // a VStack is shown as the label for each disclosure group VStack (alignment: .leading, spacing: 0) { HStack { @@ -120,23 +122,15 @@ public struct PackageRowView: View { } } } + .listRowBackground( + Rectangle() + .fill(cardColor) // native Mac card color = Color(.controlBackgroundColor) + ) } } // animate changes in the package list .animation(.easeInOut, value: packages) - // binds the visibilty state to the alert builder - .alert("Are you sure you want to delete the keyboard \"\(selectedPackage?.packageName ?? "")\"?", - isPresented: $isShowingDeleteAlert, - presenting: selectedPackage) { package in - // cancel button - Button("Cancel", role: .cancel) { } - // delete button - Button("Delete", role: .destructive) { - settings.removeInstalledPackage(with: package.id) - } - } message: { package in - Text("You can't undo this action.") - } + .padding(.vertical, 8) } // the helper method to generate the custom binding for whether a package's disclosure group is expanded or not diff --git a/mac/KeymanSettings/Sources/KeymanSettings/PackageRepo.swift b/mac/KeymanSettings/Sources/KeymanSettings/PackageRepo.swift index 8b8a710b50..505fa16bf5 100644 --- a/mac/KeymanSettings/Sources/KeymanSettings/PackageRepo.swift +++ b/mac/KeymanSettings/Sources/KeymanSettings/PackageRepo.swift @@ -19,6 +19,6 @@ public protocol PackageRepo { func loadSinglePackage(packageUrl: URL) throws -> KeymanPackage func getDownloadUrl(for kmpFilename: String) -> URL func getUnzipDestinationUrl(for packageName: String) -> URL - func buildInstallationUrlForPackageName(packageName: String) -> URL + func buildInstallationUrlForPackageName(directoryName: String) -> URL func cleanupTempDirectory() } diff --git a/mac/KeymanSettings/Sources/KeymanSettings/SettingsContainer.swift b/mac/KeymanSettings/Sources/KeymanSettings/SettingsContainer.swift index 7ed7ac5808..438444729d 100644 --- a/mac/KeymanSettings/Sources/KeymanSettings/SettingsContainer.swift +++ b/mac/KeymanSettings/Sources/KeymanSettings/SettingsContainer.swift @@ -241,7 +241,7 @@ public class SettingsContainer : ObservableObject { self.packageRepository.deletePackage(package: package) // remove package from installed packages list - if let index = self.installedPackages.firstIndex(where: { $0.packageName == package.packageName }) { + if let index = self.installedPackages.firstIndex(where: { $0.id == package.id }) { self.installedPackages.remove(at: index) } @@ -434,6 +434,8 @@ public class SettingsContainer : ObservableObject { */ func replaceInstalledPackage() { if let package = self.packageInstall?.packageToInstall { + // find the existing package with the same name in the installedPackages array and replace it + // (we cannot use the id for this search, as the ids are unique) if let index = self.installedPackages.firstIndex(where: { $0.packageName == package.packageName }) { self.installedPackages[index] = package self.addEnabledKeyboards(for: package) @@ -480,6 +482,7 @@ public class SettingsContainer : ObservableObject { do { try install.installPackage() } catch { + self.packageInstall?.cleanupFailedInstallation() self.packageInstall = nil throw error } diff --git a/mac/KeymanSettings/Sources/Persistence/PackageInstallHelper.swift b/mac/KeymanSettings/Sources/Persistence/PackageInstallHelper.swift index 394789c78c..e28c1ddec4 100644 --- a/mac/KeymanSettings/Sources/Persistence/PackageInstallHelper.swift +++ b/mac/KeymanSettings/Sources/Persistence/PackageInstallHelper.swift @@ -56,9 +56,8 @@ public class PackageInstallHelper: Identifiable { self.packageRepository = packageRepo self.temporaryKmpFileLocation = self.packageRepository.getDownloadUrl(for: filename) - // filename minus .kmp extension - let directoryName = filename.replacingOccurrences(of: kmpFileExtension, with: "") - self.temporaryPackageLocation = self.packageRepository.getUnzipDestinationUrl(for: directoryName) + // unzip in a directory named the same as the kmp file minus .kmp extension + self.temporaryPackageLocation = self.packageRepository.getUnzipDestinationUrl(for: filename.replacingOccurrences(of: kmpFileExtension, with: "")) self.installedPackages = installedPackages self.isDownload = isDownload @@ -76,7 +75,8 @@ public class PackageInstallHelper: Identifiable { } /** - * Indicates that a package is ready to be unzipped and loaded + * Prepare for installation by unzipping and loading the package and determining where it should be installed. + * */ public func prepareToInstall(for kmpFileUrl: URL) throws { print ("prepareToInstall \(kmpFileUrl)") @@ -89,11 +89,18 @@ public class PackageInstallHelper: Identifiable { let package = try self.packageRepository.loadSinglePackage(packageUrl: self.temporaryPackageLocation) self.packageToInstall = package - // now that the package is loaded, we can build the installation directory from the packageName - self.installPackageLocation = self.packageRepository.buildInstallationUrlForPackageName(packageName: package.packageName) + // if there is an existing package of the same name, use its location as the place to install + if let existingPackage = findExistingPackage() { + self.packageToReplace = existingPackage + self.installPackageLocation = existingPackage.sourceDirectoryUrl + } else { + // if this is a new package, then use the same name as the temporary install directory + let directoryName = self.temporaryPackageLocation.lastPathComponent + self.installPackageLocation = self.packageRepository.buildInstallationUrlForPackageName(directoryName: directoryName) + } // now that we know what we are installing, determine the type of install - self.packageInstallationType = self.determinePackageInstallationType() + self.packageInstallationType = self.determinePackageInstallationType(newPackage: package) } catch { self.cleanupFailedInstallation() print ("package installation failed with error '\(error)' for \(kmpFileUrl)") @@ -127,37 +134,26 @@ public class PackageInstallHelper: Identifiable { * - an update of an existing package * - a downgrade of an existing package */ - func determinePackageInstallationType() -> PackageInstallationType { - let packageAlreadyInstalled = self.checkForExistingPackage() - var installationType: PackageInstallationType = .newPackage("unknown package") + func determinePackageInstallationType(newPackage: KeymanPackage) -> PackageInstallationType { + var installationType: PackageInstallationType = .newPackage(newPackage.packageName) + let packageAlreadyInstalled = self.packageToReplace != nil - // If there is no new package, return bogus value of .newPackage. - // Without a package, the installation will fail elsewhere and the - // type of installation is completely irrelevant. - guard let newPackage = self.packageToInstall else { - print("error: packageToInstall not set when determining package installation type") - return installationType - } - - if !packageAlreadyInstalled { - installationType = PackageInstallationType.newPackage(newPackage.packageName) - } else { - if let installedPackage = self.packageToReplace { - let newVersion = newPackage.packageVersion - let existingVersion = installedPackage.packageVersion - - let comparisonResult = newVersion.compare(existingVersion, options: .numeric) - - if comparisonResult == .orderedAscending { - print("package downgrade: new version is older than existing version") - installationType = PackageInstallationType.replaceNewerPackage(newPackage.packageName, existingVersion, newVersion) - } else if comparisonResult == .orderedDescending { - print("package upgrade: new version is newer than existing version") - installationType = PackageInstallationType.replaceOlderPackage(newPackage.packageName, existingVersion, newVersion) - } else { - print("new and existing package versions are identical") - installationType = PackageInstallationType.replaceSameVersionPackage(newPackage.packageName) - } + // if we are replacing an existing package, then determine what type of replacement this is + if let existingPackage = self.packageToReplace { + let newVersion = newPackage.packageVersion + let existingVersion = existingPackage.packageVersion + + let comparisonResult = newVersion.compare(existingVersion, options: .numeric) + + if comparisonResult == .orderedAscending { + print("package downgrade: new version is older than existing version") + installationType = PackageInstallationType.replaceNewerPackage(newPackage.packageName, existingVersion, newVersion) + } else if comparisonResult == .orderedDescending { + print("package upgrade: new version is newer than existing version") + installationType = PackageInstallationType.replaceOlderPackage(newPackage.packageName, existingVersion, newVersion) + } else { + print("new and existing package versions are identical") + installationType = PackageInstallationType.replaceSameVersionPackage(newPackage.packageName) } } @@ -293,12 +289,23 @@ public class PackageInstallHelper: Identifiable { var packageExists = false if let package = self.installedPackages.first(where: { $0.packageName == self.packageToInstall?.packageName }) { - self.packageToReplace = package packageExists = true } return packageExists } + /** + * If a package of the same name exists, return it. + */ + func findExistingPackage() -> KeymanPackage? { + var existingPackage: KeymanPackage? = nil + + if let package = self.installedPackages.first(where: { $0.packageName == self.packageToInstall?.packageName }) { + existingPackage = package + } + return existingPackage + } + /** * Install the newly downloaded package (no existing package to replace) */ diff --git a/mac/KeymanSettings/Sources/Persistence/PackageRepository.swift b/mac/KeymanSettings/Sources/Persistence/PackageRepository.swift index f507968044..02e4325135 100644 --- a/mac/KeymanSettings/Sources/Persistence/PackageRepository.swift +++ b/mac/KeymanSettings/Sources/Persistence/PackageRepository.swift @@ -160,11 +160,12 @@ public class PackageRepository: PackageRepo { public func getUnzipDestinationUrl(for packageName: String) -> URL { return self.pathUtil.keyman19TempDirectory.appendingPathComponent(packageName) } + /** * build the URL where the specified package will be installed */ - public func buildInstallationUrlForPackageName(packageName: String) -> URL { - return self.pathUtil.keyman19PackagesDirectory.appendingPathComponent(packageName) + public func buildInstallationUrlForPackageName(directoryName: String) -> URL { + return self.pathUtil.keyman19PackagesDirectory.appendingPathComponent(directoryName) } /** diff --git a/mac/KeymanSettings/Tests/KeymanSettingsTests/RepoStubs.swift b/mac/KeymanSettings/Tests/KeymanSettingsTests/RepoStubs.swift index 28ee3c4f01..3e90186a43 100644 --- a/mac/KeymanSettings/Tests/KeymanSettingsTests/RepoStubs.swift +++ b/mac/KeymanSettings/Tests/KeymanSettingsTests/RepoStubs.swift @@ -94,7 +94,7 @@ class PackageRepoStub: PackageRepo { return URL(fileURLWithPath: "") } - func buildInstallationUrlForPackageName(packageName: String) -> URL { + func buildInstallationUrlForPackageName(directoryName: String) -> URL { return URL(fileURLWithPath: "") }