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

@@ -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)

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 {

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<UUID?>, showHelpTab: @escaping (URL) -> Void) {
+
+  init(packages: [KeymanPackage], isSingleKeyboardPackage: Bool, expandedPackageID: Binding<UUID?>, idToDelete: Binding<UUID?>, 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

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

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
       }

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)
    */

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)
   }
 
   /**

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: "")
   }