From cffbe4ef21daa4be373b85e1547e49adb8cb9622 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Mon, 21 Sep 2026 10:05:23 +0200 Subject: [PATCH 1/4] fix: delete the entry chosen, never another one Deleting a file could silently remove a different entry: - Inside an archive opened within the archive, a delete recorded the inner archive's number for the entry against the outer archive: deleting x.txt from inner.zip removed the outer archive's a.txt. Deleting the row of an opened inner archive did the same for everything it held, and adding a file inside one wrote it into the outer archive, in a folder named like the inner one. - With XAD picked for zip in Settings, a delete handed XAD's numbers to 7-Zip's writer. XAD numbers entries its own way, since it folds Mac metadata sidecars into the files they describe: deleting helper removed the sidecar of icon.png. What an archive opened within the archive holds belongs to that archive. It can no longer be deleted or added to through the outer one, and the delete and add controls are disabled there. The inner archive's own row still goes, alone. An archive is editable only when the engine that opened it can write its format, as the catalog says. For zip that is 7-Zip, the default; a zip XAD opened is read-only. Settings marks the engines that can edit a format with a pencil, explained in the engine info, and its engine menus now share the column's width. An open archive keeps the engine that opened it, through the reload after a save too, and so does an archive opened within it: a change of engine in Settings applies to archives opened afterwards. Switched to 7-Zip while XAD's listing was showing, extraction took XAD's numbers as well. And a window no longer carries the editing of one archive over to the next one it opens. --- Config/products/macpacker.json | 23 +++ .../ArchiveContentToolbarView.swift | 4 +- .../ArchiveTableViewRepresentable.swift | 10 +- .../ArchiveContentViewer/ArchiveView.swift | 5 +- .../Settings/FormatSettingsView.swift | 28 ++- MacPacker/Localizable.xcstrings | 3 + Modules/Sources/Core/ArchiveState.swift | 107 ++++++++--- .../Core/Engine/AutomaticEngineSelector.swift | 6 +- Modules/Sources/Core/Formats/Catalog.swift | 5 + .../Tests/CoreTests/CoverageGapTests.swift | 10 + Modules/Tests/CoreTests/ZipWriteTests.swift | 173 ++++++++++++++++++ 11 files changed, 336 insertions(+), 38 deletions(-) diff --git a/Config/products/macpacker.json b/Config/products/macpacker.json index efd4888e..694164d8 100644 --- a/Config/products/macpacker.json +++ b/Config/products/macpacker.json @@ -381,6 +381,29 @@ "issues": [ "256" ] + }, + { + "type": "fix", + "title": { + "en": "Deleting a file could remove a different one", + "zh-Hans": "删除文件时可能误删另一个文件", + "fr": "Supprimer un fichier pouvait en retirer un autre", + "de": "Löschen einer Datei konnte eine andere entfernen", + "it": "Eliminare un file poteva rimuoverne un altro", + "ja": "ファイルを削除すると別のファイルが消えることがあった", + "fa": "حذف یک فایل ممکن بود فایل دیگری را پاک کند", + "pl": "Usunięcie pliku mogło usunąć inny plik", + "pt-BR": "Excluir um arquivo podia remover outro", + "ru": "Удаление файла могло стереть другой файл", + "uk": "Видалення файлу могло стерти інший файл", + "es-MX": "Eliminar un archivo podía quitar otro", + "ko": "파일을 삭제하면 다른 파일이 지워질 수 있었음", + "nl": "Een bestand verwijderen kon een ander wissen", + "tr": "Bir dosyayı silmek başka birini silebiliyordu" + }, + "issues": [ + "267" + ] } ] }, diff --git a/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift b/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift index 5928eca2..94bde50a 100644 --- a/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift +++ b/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift @@ -84,7 +84,7 @@ struct ArchiveContentToolbarView: ToolbarContent { } } .help("Add files or folders to the archive") - .disabled(!archiveState.canBeEdited || archiveState.isSaving) + .disabled(!archiveState.canAddHere) Button { archiveState.remove(items: archiveState.selectedItems) @@ -96,7 +96,7 @@ struct ArchiveContentToolbarView: ToolbarContent { } } .help("Delete the selected items from the archive") - .disabled(!archiveState.canBeEdited || archiveState.selectedItems.isEmpty || archiveState.isSaving) + .disabled(!archiveState.canRemove(archiveState.selectedItems)) Spacer() diff --git a/MacPacker/Features/ArchiveContentViewer/ArchiveTableViewRepresentable.swift b/MacPacker/Features/ArchiveContentViewer/ArchiveTableViewRepresentable.swift index 1ec2869e..aa791110 100644 --- a/MacPacker/Features/ArchiveContentViewer/ArchiveTableViewRepresentable.swift +++ b/MacPacker/Features/ArchiveContentViewer/ArchiveTableViewRepresentable.swift @@ -410,7 +410,7 @@ struct ArchiveTableViewRepresentable: NSViewRepresentable { delete.keyEquivalentModifierMask = [] delete.target = self delete.image = NSImage(systemSymbolName: "trash", accessibilityDescription: nil) - delete.isEnabled = hasSelection && state.canBeEdited && !state.isSaving + delete.isEnabled = state.canRemove(state.selectedItems) menu.addItem(delete) } @@ -446,7 +446,7 @@ struct ArchiveTableViewRepresentable: NSViewRepresentable { @objc func contextDelete(_ sender: Any?) { let state = parent.archiveState - guard state.canBeEdited, !state.selectedItems.isEmpty else { return } + guard state.canRemove(state.selectedItems) else { return } state.remove(items: state.selectedItems) } @@ -716,12 +716,12 @@ class ArchiveTableView: NSTableView, NSMenuItemValidation { } /// Enables Edit ▸ Delete only when the front archive is editable and has a - /// selection; every other item keeps the default "enabled if the responder - /// handles it" behavior. + /// selection that is its own, not inside an archive opened within it; every + /// other item keeps the default "enabled if the responder handles it" behavior. func validateMenuItem(_ menuItem: NSMenuItem) -> Bool { if menuItem.action == #selector(delete(_:)) { guard let state else { return false } - return state.canBeEdited && !state.selectedItems.isEmpty && !state.isSaving + return state.canRemove(state.selectedItems) } return true } diff --git a/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift b/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift index bbb18c9f..b198c510 100644 --- a/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift +++ b/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift @@ -43,9 +43,10 @@ struct ArchiveView: View { /// How far the archive window's drop cards sit inside the window. private static let dropInset: CGFloat = 12 - /// Adding needs an archive that the format can write, and no in-flight save. + /// Adding needs an archive that the format can write, no in-flight save, and + /// the window in that archive, not in one opened within it. private var canAdd: Bool { - state.hasArchive && state.canBeEdited && !state.isSaving + state.hasArchive && state.canAddHere } /// Dropping a non-archive doesn't open anything — it starts a new archive with diff --git a/MacPacker/Features/Settings/FormatSettingsView.swift b/MacPacker/Features/Settings/FormatSettingsView.swift index 132db5b5..3ed7c435 100644 --- a/MacPacker/Features/Settings/FormatSettingsView.swift +++ b/MacPacker/Features/Settings/FormatSettingsView.swift @@ -13,6 +13,9 @@ struct ArchiveFormatSettings: Identifiable { let name: String let extensions: String let engines: [ArchiveEngineType] + /// The engines that can also edit archives of this format, as the catalog + /// says: one that only reads opens them read-only. + let editors: Set var selectedEngine: ArchiveEngineType var defaultOpen: Bool = false } @@ -56,6 +59,7 @@ struct FormatSettingsView: View { appState.archiveEngineConfigStore.selectedEngine(for: formatId) else { continue } let engines = engineOptions.compactMap { ArchiveEngineType(configId: $0.id) } + let editors = Set(engineOptions.filter(\.canEdit).compactMap { ArchiveEngineType(configId: $0.id) }) let extString = type.extensions.joined(separator: ", ") // Default app detection: keep false for now (toggle is disabled anyway) @@ -69,6 +73,7 @@ struct FormatSettingsView: View { name: type.name, extensions: extString, engines: engines, + editors: editors, selectedEngine: selectedEngine, defaultOpen: isDefaultApp ) @@ -132,7 +137,8 @@ struct FormatSettingsView: View { TableColumn("File Format", value: \.name) TableColumn("Extensions", value: \.extensions) TableColumn("Engine") { - supportedPicker(identifier: $0.id, selectedEngine: $0.selectedEngine, supportedEngines: $0.engines) + supportedPicker(identifier: $0.id, selectedEngine: $0.selectedEngine, + supportedEngines: $0.engines, editors: $0.editors) } } .tableStyle(.bordered) @@ -184,6 +190,15 @@ struct FormatSettingsView: View { } } .font(.footnote) + + Divider() + + Label { + Text("Edits archives, not only opens them", comment: "Legend in the engine info popover: the pencil next to an engine in the format table means that engine can also change archives of that format, where the others only open and extract them") + } icon: { + Image(systemName: "pencil") + } + .font(.footnote) } .frame(width: 260) .padding() @@ -232,7 +247,8 @@ struct FormatSettingsView: View { func supportedPicker( identifier: String, selectedEngine: ArchiveEngineType, - supportedEngines: [ArchiveEngineType] + supportedEngines: [ArchiveEngineType], + editors: Set ) -> some View { let binding = Binding( get: { selectedEngine }, @@ -246,12 +262,18 @@ struct FormatSettingsView: View { Picker(String(""), selection: binding) { ForEach(supportedEngines, id: \.self) { engine in - Text(engine.rawValue).tag(engine) + if editors.contains(engine) { + Label(engine.rawValue, systemImage: "pencil").tag(engine) + } else { + Text(engine.rawValue).tag(engine) + } } } .labelsHidden() .pickerStyle(.menu) .controlSize(.small) + // as wide as the column, so every row's picker lines up + .frame(maxWidth: .infinity) // In automatic mode the column shows what MacPacker picked; editing it // would imply a choice that is not being honoured. .disabled(isAutomatic) diff --git a/MacPacker/Localizable.xcstrings b/MacPacker/Localizable.xcstrings index 951496b6..29b8afb0 100644 --- a/MacPacker/Localizable.xcstrings +++ b/MacPacker/Localizable.xcstrings @@ -3879,6 +3879,9 @@ } } }, + "Edits archives, not only opens them" : { + "comment" : "Legend in the engine info popover: the pencil next to an engine in the format table means that engine can also change archives of that format, where the others only open and extract them" + }, "Enabled" : { "comment" : "A label indicating that the file provider extension is enabled.", "isCommentAutoGenerated" : true, diff --git a/Modules/Sources/Core/ArchiveState.swift b/Modules/Sources/Core/ArchiveState.swift index 84fdc865..ae8547f6 100644 --- a/Modules/Sources/Core/ArchiveState.swift +++ b/Modules/Sources/Core/ArchiveState.swift @@ -108,10 +108,12 @@ public class ArchiveState: ObservableObject { private let catalog: ArchiveTypeCatalog private let archiveEngineSelector: ArchiveEngineSelectorProtocol - /// Formats whose configured engine could not open *this* archive, mapped to - /// the engine that could. Set by the loader's fallback and used for every - /// later lookup so extraction does not resolve back to the engine that - /// already failed. Cleared on reset with the rest of the archive state. + /// The engine that read this window's archives, by format — the archive and + /// any opened inside it. Every later lookup gets it, whatever Settings say + /// by then: entries are numbered by the engine that listed them, and 7-Zip + /// takes XAD's numbers for other entries. It is also where the loader's + /// fallback stays, when the configured engine could not open the archive. + /// Cleared on reset with the rest of the archive state. private var pinnedEngines: [String: ArchiveEngineType] = [:] /// The selector everything downstream should use. @@ -236,6 +238,7 @@ extension ArchiveState { /// Resets the state of the archive private func reset() { self.hasArchive = false + self.canBeEdited = false // A failed open sets its reason after this, so its alert still shows. self.openError = nil self.saveError = nil @@ -547,6 +550,12 @@ extension ArchiveState { return } guard let selectedItem else { return } + // A save writes only this archive. Added inside one opened within it, + // the file would land in this one, in a folder named like that archive. + guard !isWithinOpenedArchive(selectedItem) else { + log.notice("Ignoring add — inside an archive opened within this one", context: ["file": url.lastPathComponent]) + return + } let base = (selectedItem.virtualPath?.isEmpty == false && selectedItem.virtualPath != "/") ? selectedItem.virtualPath! + "/" : "" @@ -634,6 +643,10 @@ extension ArchiveState { log.notice("Ignoring delete — a save is in progress", context: ["items": "\(items.count)"]) return } + guard canRemove(items) else { + log.notice("Ignoring delete — nothing this archive can remove", context: ["items": "\(items.count)"]) + return + } let dropped = discard(items: items) @@ -685,25 +698,42 @@ extension ArchiveState { /// Depth-first: collects the source indices (real archive entries) and /// pending-addition paths of the item and all of its descendants, and - /// drops them from `entries`. + /// drops them from `entries`. An archive opened within this one goes as the + /// entry it is: what it holds leaves the tree with it, but belongs to that + /// archive, numbered as that one's entries. private func collectRemovals( _ item: ArchiveItem, indices: inout Set, - pendingPaths: inout Set + pendingPaths: inout Set, + inThisArchive: Bool = true ) { for childId in item.children ?? [] { if let child = entries[childId] { - collectRemovals(child, indices: &indices, pendingPaths: &pendingPaths) + collectRemovals(child, indices: &indices, pendingPaths: &pendingPaths, + inThisArchive: inThisArchive && item.archiveTypeId == nil) } } - if let index = item.index { - indices.insert(index) - } else if let path = item.virtualPath, path != "/" { - pendingPaths.insert(path) + if inThisArchive { + if let index = item.index { + indices.insert(index) + } else if let path = item.virtualPath, path != "/" { + pendingPaths.insert(path) + } } entries.removeValue(forKey: item.id) } + /// Whether `item` is an archive opened within this one, or inside one: what + /// such an archive holds is that archive's, and a save writes only this one. + private func isWithinOpenedArchive(_ item: ArchiveItem) -> Bool { + var current: ArchiveItem? = item + while let node = current, node.type != .root { + if node.archiveTypeId != nil { return true } + current = node.parent.flatMap { entries[$0] } + } + return false + } + /// Opens a dropped file in this window: a supported archive is opened directly, /// unless another window has it open already — that one comes forward instead; /// anything else becomes the first entry of a new archive. `open`/`create` reset @@ -721,6 +751,20 @@ extension ArchiveState { /// Whether there are unsaved changes (pending additions/removals). public var hasPendingChanges: Bool { !diff.isEmpty } + /// Whether `items` can be removed: none of them is inside an archive opened + /// within this one. The row of such an archive is this archive's own entry, + /// and can go. + public func canRemove(_ items: [ArchiveItem]) -> Bool { + canBeEdited && !isSaving && !items.isEmpty + && !items.contains { item in item.parent.flatMap { entries[$0] }.map(isWithinOpenedArchive) ?? false } + } + + /// Whether files can be added where the window is: into this archive, not + /// into one opened within it. + public var canAddHere: Bool { + canBeEdited && !isSaving && selectedItem.map { !isWithinOpenedArchive($0) } == true + } + /// Saves the pending changes. /// /// - For an archive loaded from disk, the changes are applied in place. @@ -786,6 +830,7 @@ extension ArchiveState { log.notice("Archive saved", context: ["target": target.lastPathComponent]) // reload from disk so entries and indices reflect the file + let engines = pinnedEngines open(url: saved.url) // Known already — reopening must not ask again. Set after open(), // which starts by forgetting every password, and before the first @@ -793,6 +838,9 @@ extension ArchiveState { if let password = saved.password { passwords[saved.url] = password } + // Likewise the engine: it is still the archive the window opened, + // whatever Settings have said since. + pinnedEngines = engines _ = try? await openTask?.value self.isSaving = false } catch { @@ -894,25 +942,31 @@ extension ArchiveState { if let firstVolume = loaderResult.firstVolumeURL { self.url = firstVolume } - if let type, - type.engines.contains(where: {$0.capabilities.contains(where: {$0 == "edit"})}) { + // Changed only by an engine that writes the format, since a + // change names entries by the numbers of the engine that listed + // them. XAD only reads, and 7-Zip's writer would take its numbers + // for other entries. + if let type, let used = loaderResult.engineType, + type.engines.contains(where: { $0.id == used.configId && $0.canEdit }) { canBeEdited = true } self.uncompressedSize = loaderResult.uncompressedSize self.isEncrypted = loaderResult.isEncrypted - // The loader may have fallen back to another engine because the - // configured one cannot read this archive. Pin it, or the first - // extraction would resolve the failing engine all over again. + // The archive stays with the engine that read it (see + // `pinnedEngines`). That may be a fallback, when the configured + // engine cannot read this archive: the first extraction would + // otherwise resolve the failing engine all over again. self.activeEngine = loaderResult.engineType - if let used = loaderResult.engineType, - used != archiveEngineSelector.engineType(for: loaderResult.type.id) { + if let used = loaderResult.engineType { pinnedEngines[loaderResult.type.id] = used - log.notice("Engine pinned for this archive", context: [ - "file": url.lastPathComponent, - "type": loaderResult.type.id, - "engine": used.configId - ]) + if used != archiveEngineSelector.engineType(for: loaderResult.type.id) { + log.notice("Engine pinned for this archive", context: [ + "file": url.lastPathComponent, + "type": loaderResult.type.id, + "engine": used.configId + ]) + } } updateStatusText(String(localized: "building tree...", bundle: .module, comment: "Archive operation status")) @@ -1177,6 +1231,13 @@ extension ArchiveState { updateStatusText(String(localized: "loading...", bundle: .module, comment: "Archive operation status")) let loaderResult = try await archiveLoader.loadEntries(url: url) + // Stays with the engine that read it, like the archive around it. + // One of that archive's format keeps its pin: the pin is per + // format, and moving it would hand the outer archive's entries to + // an engine that numbers them otherwise. + if let used = loaderResult.engineType, pinnedEngines[loaderResult.type.id] == nil { + pinnedEngines[loaderResult.type.id] = used + } if let tempDirectory = loaderResult.tempDirectory { tempDirectories.append(tempDirectory) diff --git a/Modules/Sources/Core/Engine/AutomaticEngineSelector.swift b/Modules/Sources/Core/Engine/AutomaticEngineSelector.swift index 9dd58c58..74b33399 100644 --- a/Modules/Sources/Core/Engine/AutomaticEngineSelector.swift +++ b/Modules/Sources/Core/Engine/AutomaticEngineSelector.swift @@ -15,9 +15,9 @@ import Foundation /// the header has to be decrypted during `XADArchive` init and XADArchive only /// takes a password afterwards. /// -/// In automatic mode `ArchiveLoader` may fall back to another engine to read an -/// archive; `ArchiveState` then pins that engine here for the rest of the -/// window. Without it the archive would list through the fallback and then fail +/// `ArchiveState` pins here the engine that read each archive, for the rest of +/// the window. In automatic mode that may be a fallback `ArchiveLoader` took; +/// without the pin the archive would list through the fallback and then fail /// on the first extraction, which resolves the engine from the format all over /// again. Fallback stays on so the pinned engine keeps its own alternatives. struct AutomaticEngineSelector: ArchiveEngineSelectorProtocol { diff --git a/Modules/Sources/Core/Formats/Catalog.swift b/Modules/Sources/Core/Formats/Catalog.swift index a8007ba4..f36b20e0 100644 --- a/Modules/Sources/Core/Formats/Catalog.swift +++ b/Modules/Sources/Core/Formats/Catalog.swift @@ -126,6 +126,11 @@ public struct EngineDto: Codable, Sendable { public let id: String // engine ID ("xad", "7zip", ...) public let capabilities: [String] // ["listContents", "extractFiles", "splitVolumes"] public let `default`: Bool? // optional, only present on one item + + /// Whether the engine writes archives of its format, so one it opened can be + /// changed. An engine that only reads numbers entries its own way, and the + /// writer would take those numbers for other entries. + public var canEdit: Bool { capabilities.contains("edit") } } // MARK: - Compounds diff --git a/Modules/Tests/CoreTests/CoverageGapTests.swift b/Modules/Tests/CoreTests/CoverageGapTests.swift index 94498786..faa8cb6b 100644 --- a/Modules/Tests/CoreTests/CoverageGapTests.swift +++ b/Modules/Tests/CoreTests/CoverageGapTests.swift @@ -462,6 +462,16 @@ extension AllCoreTests { let engine = catalog.defaultEngine(for: "zip") #expect(engine != nil) } + + /// What the window and the Settings pencil go by: only 7-Zip edits, and + /// only zip. A format gaining an editor shows up here first (#265). + @Test func onlySevenZipEditsAndOnlyZip() { + let catalog = ArchiveTypeCatalog() + let editors = catalog.allFormatIds().flatMap { format in + catalog.engineOptions(for: format).filter(\.canEdit).map { "\(format)/\($0.id)" } + } + #expect(editors == ["zip/7zip"]) + } } // MARK: - ArchiveEngineConfigStore: persistence diff --git a/Modules/Tests/CoreTests/ZipWriteTests.swift b/Modules/Tests/CoreTests/ZipWriteTests.swift index 3ee8addf..7bb05f38 100644 --- a/Modules/Tests/CoreTests/ZipWriteTests.swift +++ b/Modules/Tests/CoreTests/ZipWriteTests.swift @@ -615,6 +615,179 @@ extension AllCoreTests { #expect(try String(contentsOf: out.appendingPathComponent("folder/two.txt"), encoding: .utf8) == "two") } + // MARK: - The engine that read the archive + + /// A window with its Settings: the engine picked for zip, which can change + /// while the window is open. + private func stateWithSettings(zip engine: ArchiveEngineType) -> (ArchiveEngineConfigStore, ArchiveState) { + let catalog = ArchiveTypeCatalog() + let store = ArchiveEngineConfigStore(catalog: catalog, defaults: isolatedDefaults()) + store.isAutomatic = false + store.setSelectedEngine(engine, for: "zip") + let state = ArchiveState(catalog: catalog, engineSelector: ArchiveEngineSelector(catalog: catalog, configStore: store)) + state.folderAccessProvider = { _ in true } + return (store, state) + } + + /// Finder's `__MACOSX/._name` sidecars, which XAD folds into the files + /// they describe, so its entries are numbered unlike 7-Zip's: XAD's + /// number for `helper` is 7-Zip's for the sidecar of `icon.png`. + private func appleDoubleZip(in dir: URL) throws -> URL { + let zip = dir.appendingPathComponent("appledouble.zip") + try FileManager.default.copyItem( + at: Bundle.module.url(forResource: "zip", withExtension: nil)!.appendingPathComponent("appledouble.zip"), + to: zip) + return zip + } + + /// `helper` as 7-Zip finds it by its own path. + private func helperContents(in zip: URL, dir: URL) throws -> Data { + let archive = try SevenZipArchive(url: zip) + let entry = try #require(try archive.entries.first { $0.path == "payload/Contents/MacOS/helper" }) + let out = dir.appendingPathComponent(UUID().uuidString) + try FileManager.default.createDirectory(at: out, withIntermediateDirectories: true) + return try Data(contentsOf: #require(try archive.extract(index: entry.index, to: out)[entry.index])) + } + + /// Only an engine that writes a format can change an archive it opened, + /// and for zip that is 7-Zip. XAD numbers entries its own way — it folds + /// Mac metadata sidecars into the files they describe — and a delete made + /// through its listing handed those numbers to 7-Zip's writer: deleting + /// `helper` removed the sidecar of `icon.png`. A zip XAD opened is + /// read-only. + @Test func aZipXADOpenedCannotBeChanged() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (_, state) = stateWithSettings(zip: .xad) + state.open(url: try appleDoubleZip(in: dir)) + try await state.openTask?.value + #expect(state.activeEngine == .xad) + #expect(!state.canBeEdited) + + state.remove(items: [try #require(item("payload/Contents/MacOS/helper", in: state))]) + #expect(!state.hasPendingChanges) + } + + /// The archive a save reloads is still the one the window opened: it keeps + /// its engine, and a switch to XAD in Settings meanwhile does not turn it + /// read-only once it is saved. + @Test func aSaveKeepsTheEngineThatOpenedTheArchive() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (settings, state) = stateWithSettings(zip: .`7zip`) + state.open(url: try makeSystemZipFixture(in: dir)) + try await state.openTask?.value + + settings.setSelectedEngine(.xad, for: "zip") + state.remove(items: [try #require(item("folder/one.txt", in: state))]) + await state.save()?.value + #expect(state.saveError == nil, "\(state.saveError ?? "")") + #expect(item("folder/one.txt", in: state) == nil) + #expect(state.activeEngine == .`7zip`) + #expect(state.canBeEdited) + } + + /// A window holds one archive's editing at a time: opening one that cannot + /// be edited does not carry over the editing of the one before. + @Test func editingEndsWithTheArchiveItBelongsTo() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let sevenZ = dir.appendingPathComponent("plain.7z") + try SevenZipArchive.writeArchive( + destination: sevenZ, items: [.addData(archivePath: "a.txt", data: Data("a".utf8))], + options: .init(format: .sevenZ)) + let state = makeState() + state.open(url: try makeSystemZipFixture(in: dir)) + try await state.openTask?.value + #expect(state.canBeEdited) + + state.open(url: sevenZ) + try await state.openTask?.value + #expect(!state.canBeEdited) + } + + /// An archive opened inside the archive keeps its entries in an archive of + /// its own, and a save writes only the outer one: nothing inside can be + /// removed or added from here. Deleting `x.txt` inside `inner.zip` deleted + /// the outer archive's `a.txt`, since both were entry 0. The row of the + /// inner archive is the outer one's, and goes alone. + @Test func anArchiveOpenedInsideIsNotChangedThroughTheOuterOne() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let inner = dir.appendingPathComponent("inner.zip") + try SevenZipArchive.writeArchive( + destination: inner, + items: [.addData(archivePath: "x.txt", data: Data("x".utf8)), + .addData(archivePath: "y.txt", data: Data("y".utf8))], + options: .init(format: .zip)) + let outer = dir.appendingPathComponent("outer.zip") + try SevenZipArchive.writeArchive( + destination: outer, + items: [.addData(archivePath: "a.txt", data: Data("a".utf8)), + .addData(archivePath: "b.txt", data: Data("b".utf8)), + .addFile(archivePath: "inner.zip", diskPath: inner)], + options: .init(format: .zip)) + let state = makeState() + state.open(url: outer) + try await state.openTask?.value + let innerRow = try #require(item("inner.zip", in: state)) + try await state.openAsync(item: innerRow) + + let x = try #require(state.entries.values.first { $0.name == "x.txt" }) + #expect(!state.canRemove([x])) + state.remove(items: [x]) + #expect(!state.canAddHere) + state.add(url: inner) + #expect(!state.hasPendingChanges) + + state.openParent() + #expect(state.canAddHere) + #expect(state.canRemove([innerRow])) + state.remove(items: [innerRow]) + await state.save()?.value + #expect(state.saveError == nil, "\(state.saveError ?? "")") + #expect(try systemZipEntries(outer) == ["a.txt", "b.txt"]) + } + + /// A switch of engine in Settings applies to the archives opened after it. + /// The open one stays on the engine that read it and numbered its entries: + /// 7-Zip extracts by number, and would have taken XAD's number for `helper` + /// for the sidecar of `icon.png`. + @Test func anOpenArchiveKeepsTheEngineThatReadIt() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let zip = try appleDoubleZip(in: dir) + let (settings, state) = stateWithSettings(zip: .xad) + state.open(url: zip) + try await state.openTask?.value + let helper = try #require(item("payload/Contents/MacOS/helper", in: state)) + + settings.setSelectedEngine(.`7zip`, for: "zip") + let extracted = try await state.extractToTemp(item: helper) + #expect(try Data(contentsOf: extracted) == helperContents(in: zip, dir: dir)) + } + + /// So does an archive opened inside the archive, with the engine that read + /// it — not the outer one's, which is another format's. + @Test func anArchiveOpenedInsideKeepsTheEngineThatReadIt() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let zip = try appleDoubleZip(in: dir) + let outer = dir.appendingPathComponent("outer.7z") + try SevenZipArchive.writeArchive( + destination: outer, items: [.addFile(archivePath: "appledouble.zip", diskPath: zip)], + options: .init(format: .sevenZ)) + let (settings, state) = stateWithSettings(zip: .xad) + state.open(url: outer) + try await state.openTask?.value + try await state.openAsync(item: try #require(item("appledouble.zip", in: state))) + let helper = try #require(item("payload/Contents/MacOS/helper", in: state)) + + settings.setSelectedEngine(.`7zip`, for: "zip") + let extracted = try await state.extractToTemp(item: helper) + #expect(try Data(contentsOf: extracted) == helperContents(in: zip, dir: dir)) + } + // MARK: - macOS metadata (#191, #216) // A zip has nowhere to keep a resource fork or an extended attribute, so From 418538ac12025f985b4d431def3510ae81af8c61 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Mon, 21 Sep 2026 22:18:58 +0200 Subject: [PATCH 2/4] fix: line up the engine menus in Settings SwiftUI's menu picker keeps the width of its longest engine whatever frame it is given, so the rows did not line up. The engine menu is now AppKit's popup, which takes the column's width. --- .../Settings/FormatSettingsView.swift | 71 +++++++++++++++---- 1 file changed, 56 insertions(+), 15 deletions(-) diff --git a/MacPacker/Features/Settings/FormatSettingsView.swift b/MacPacker/Features/Settings/FormatSettingsView.swift index 3ed7c435..faf0c9a5 100644 --- a/MacPacker/Features/Settings/FormatSettingsView.swift +++ b/MacPacker/Features/Settings/FormatSettingsView.swift @@ -260,23 +260,64 @@ struct FormatSettingsView: View { } ) - Picker(String(""), selection: binding) { - ForEach(supportedEngines, id: \.self) { engine in - if editors.contains(engine) { - Label(engine.rawValue, systemImage: "pencil").tag(engine) - } else { - Text(engine.rawValue).tag(engine) - } + EnginePopUp(engines: supportedEngines, editors: editors, selection: binding) + // In automatic mode the column shows what MacPacker picked; editing it + // would imply a choice that is not being honoured. + .disabled(isAutomatic) + } +} + +/// A format's engine menu. AppKit's popup rather than SwiftUI's menu picker, +/// which keeps the width of its longest engine whatever frame it is given, so +/// the rows would not line up. An engine that can edit the format carries a +/// pencil, in the menu and on the button. +private struct EnginePopUp: NSViewRepresentable { + let engines: [ArchiveEngineType] + let editors: Set + @Binding var selection: ArchiveEngineType + + func makeCoordinator() -> Coordinator { Coordinator(self) } + + func makeNSView(context: Context) -> NSPopUpButton { + let button = NSPopUpButton(frame: .zero, pullsDown: false) + button.controlSize = .small + button.font = .systemFont(ofSize: NSFont.systemFontSize(for: .small)) + button.target = context.coordinator + button.action = #selector(Coordinator.picked(_:)) + return button + } + + func updateNSView(_ button: NSPopUpButton, context: Context) { + context.coordinator.parent = self + button.removeAllItems() + for engine in engines { + button.addItem(withTitle: engine.rawValue) + if editors.contains(engine) { + button.lastItem?.image = NSImage( + systemSymbolName: "pencil", + accessibilityDescription: String(localized: "Edits archives, not only opens them", comment: "Legend in the engine info popover: the pencil next to an engine in the format table means that engine can also change archives of that format, where the others only open and extract them")) } } - .labelsHidden() - .pickerStyle(.menu) - .controlSize(.small) - // as wide as the column, so every row's picker lines up - .frame(maxWidth: .infinity) - // In automatic mode the column shows what MacPacker picked; editing it - // would imply a choice that is not being honoured. - .disabled(isAutomatic) + button.selectItem(at: engines.firstIndex(of: selection) ?? -1) + button.isEnabled = context.environment.isEnabled + } + + /// As wide as the column. + func sizeThatFits(_ proposal: ProposedViewSize, nsView: NSPopUpButton, context: Context) -> CGSize? { + let fitting = nsView.intrinsicContentSize + let width = proposal.width.flatMap { $0.isFinite ? $0 : nil } ?? fitting.width + return CGSize(width: width, height: fitting.height) + } + + final class Coordinator: NSObject { + var parent: EnginePopUp + init(_ parent: EnginePopUp) { self.parent = parent } + + @objc func picked(_ sender: NSPopUpButton) { + let index = sender.indexOfSelectedItem + guard parent.engines.indices.contains(index) else { return } + parent.selection = parent.engines[index] + } } } From ad4e1052d898066c5580097559652d8ecd9afdef Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Mon, 21 Sep 2026 22:52:14 +0200 Subject: [PATCH 3/4] core: make the Finder drag UI tests drag again The three UI tests that drag a file from Finder failed on macOS 27 without the drag ever happening. XCTest plays press(forDuration:thenDragTo:) back as a touch gesture there ("Event is touch-exclusive"): testmanagerd waited out its five-second playback and the pointer never moved. They use click(forDuration:thenDragTo:) now, a mouse drag. The two drop-window tests also expected a zip, while Quick Compress opens on the format it was last used with, 7z on this Mac. They pass -dropWindowFormat zip. --- MacPackerUITests/MacPackerUITests.swift | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/MacPackerUITests/MacPackerUITests.swift b/MacPackerUITests/MacPackerUITests.swift index 0c087de8..6051a4a5 100644 --- a/MacPackerUITests/MacPackerUITests.swift +++ b/MacPackerUITests/MacPackerUITests.swift @@ -337,10 +337,12 @@ final class MacPackerUITests: XCTestCase { let source = finder.textFields.matching(NSPredicate(format: "value == %@", "dropped.txt")).firstMatch XCTAssertTrue(source.waitForExistence(timeout: 15), "Finder does not show the file to drag") - // upper half of the window = the "add" zone + // upper half of the window = the "add" zone. A click-drag, not a press: on + // macOS 27 XCTest plays `press` back as a touch gesture, and no mouse drag + // comes of it — the pointer never moves. let target = app.windows.firstMatch.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.35)) source.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) - .press(forDuration: 1, thenDragTo: target) + .click(forDuration: 1, thenDragTo: target) XCTAssertTrue(app.staticTexts["dropped.txt"].waitForExistence(timeout: 15), "the dropped file was not added to the archive") @@ -355,7 +357,7 @@ final class MacPackerUITests: XCTestCase { sleep(1) let openZone = app.windows.firstMatch.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.8)) source.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) - .press(forDuration: 1, thenDragTo: openZone) + .click(forDuration: 1, thenDragTo: openZone) XCTAssertTrue(app.staticTexts["New Archive"].waitForExistence(timeout: 15), "the open zone did not start a new archive for the plain file") XCTAssertTrue(app.staticTexts["Open in a new window"].waitForNonExistence(timeout: 5), @@ -375,7 +377,8 @@ final class MacPackerUITests: XCTestCase { defer { try? FileManager.default.removeItem(at: dir) } try "dropped".write(to: dir.appendingPathComponent("dropped.txt"), atomically: true, encoding: .utf8) - let app = launchApp(arguments: ["-DropWindow", "1"]) + // zip, whatever format Quick Compress was last used with on this Mac + let app = launchApp(arguments: ["-DropWindow", "1", "-dropWindowFormat", "zip"]) // by identifier, not by window title: the window is titled after the app, // exactly like the archive windows let dropArea = app.descendants(matching: .any)["quickCompress.dropArea"].firstMatch @@ -388,7 +391,7 @@ final class MacPackerUITests: XCTestCase { let target = dropArea.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) source.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) - .press(forDuration: 1, thenDragTo: target) + .click(forDuration: 1, thenDragTo: target) confirmAccessPanel(app, button: "Grant Access") @@ -442,7 +445,8 @@ final class MacPackerUITests: XCTestCase { defer { try? FileManager.default.removeItem(at: dir) } try "dropped".write(to: dir.appendingPathComponent("dropped.txt"), atomically: true, encoding: .utf8) - let app = launchApp(arguments: ["-DropWindow", "1", "-dropWindowOptionsExpanded", "YES"]) + let app = launchApp(arguments: ["-DropWindow", "1", "-dropWindowOptionsExpanded", "YES", + "-dropWindowFormat", "zip"]) let dropArea = app.descendants(matching: .any)["quickCompress.dropArea"].firstMatch XCTAssertTrue(dropArea.waitForExistence(timeout: 15), "the drop window did not open") let password = app.secureTextFields["saveOptions.password"].firstMatch @@ -459,7 +463,7 @@ final class MacPackerUITests: XCTestCase { XCTAssertTrue(source.waitForExistence(timeout: 15), "Finder does not show the file to drag") let drop = { source.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) - .press(forDuration: 1, thenDragTo: dropArea.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5))) + .click(forDuration: 1, thenDragTo: dropArea.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5))) } drop() From e7d7fb69667794d94c2e8ec78d8db657a8265909 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Mon, 21 Sep 2026 23:05:33 +0200 Subject: [PATCH 4/4] fix: line up the engine menus with SwiftUI's own picker A menu picker takes the width of its widest entry, so each row's menu was as wide as its own longest engine. Every entry now has the width of the longest engine name at the table's small size, with room for the pencil, computed from the names so a longer one cannot be cut off. That lines the menus up without the AppKit popup, which goes again. --- .../Settings/FormatSettingsView.swift | 85 ++++++------------- 1 file changed, 28 insertions(+), 57 deletions(-) diff --git a/MacPacker/Features/Settings/FormatSettingsView.swift b/MacPacker/Features/Settings/FormatSettingsView.swift index faf0c9a5..1b56f34c 100644 --- a/MacPacker/Features/Settings/FormatSettingsView.swift +++ b/MacPacker/Features/Settings/FormatSettingsView.swift @@ -243,6 +243,17 @@ struct FormatSettingsView: View { .disabled(true) } + /// A menu takes the width of its widest engine. Every row's menu is as wide + /// as the longest engine name at the table's small size, with room for the + /// pencil, so they all line up. + private static let engineMenuWidth: CGFloat = { + let font = NSFont.systemFont(ofSize: NSFont.systemFontSize(for: .small)) + let widest = ArchiveEngineType.allCases + .map { ($0.rawValue as NSString).size(withAttributes: [.font: font]).width } + .max() ?? 0 + return ceil(widest) + 17 // the pencil and its spacing + }() + @ViewBuilder func supportedPicker( identifier: String, @@ -260,64 +271,24 @@ struct FormatSettingsView: View { } ) - EnginePopUp(engines: supportedEngines, editors: editors, selection: binding) - // In automatic mode the column shows what MacPacker picked; editing it - // would imply a choice that is not being honoured. - .disabled(isAutomatic) - } -} - -/// A format's engine menu. AppKit's popup rather than SwiftUI's menu picker, -/// which keeps the width of its longest engine whatever frame it is given, so -/// the rows would not line up. An engine that can edit the format carries a -/// pencil, in the menu and on the button. -private struct EnginePopUp: NSViewRepresentable { - let engines: [ArchiveEngineType] - let editors: Set - @Binding var selection: ArchiveEngineType - - func makeCoordinator() -> Coordinator { Coordinator(self) } - - func makeNSView(context: Context) -> NSPopUpButton { - let button = NSPopUpButton(frame: .zero, pullsDown: false) - button.controlSize = .small - button.font = .systemFont(ofSize: NSFont.systemFontSize(for: .small)) - button.target = context.coordinator - button.action = #selector(Coordinator.picked(_:)) - return button - } - - func updateNSView(_ button: NSPopUpButton, context: Context) { - context.coordinator.parent = self - button.removeAllItems() - for engine in engines { - button.addItem(withTitle: engine.rawValue) - if editors.contains(engine) { - button.lastItem?.image = NSImage( - systemSymbolName: "pencil", - accessibilityDescription: String(localized: "Edits archives, not only opens them", comment: "Legend in the engine info popover: the pencil next to an engine in the format table means that engine can also change archives of that format, where the others only open and extract them")) + Picker(String(""), selection: binding) { + ForEach(supportedEngines, id: \.self) { engine in + Group { + if editors.contains(engine) { + Label(engine.rawValue, systemImage: "pencil") + } else { + Text(engine.rawValue) + } + } + .frame(width: Self.engineMenuWidth, alignment: .leading) + .tag(engine) } } - button.selectItem(at: engines.firstIndex(of: selection) ?? -1) - button.isEnabled = context.environment.isEnabled - } - - /// As wide as the column. - func sizeThatFits(_ proposal: ProposedViewSize, nsView: NSPopUpButton, context: Context) -> CGSize? { - let fitting = nsView.intrinsicContentSize - let width = proposal.width.flatMap { $0.isFinite ? $0 : nil } ?? fitting.width - return CGSize(width: width, height: fitting.height) - } - - final class Coordinator: NSObject { - var parent: EnginePopUp - init(_ parent: EnginePopUp) { self.parent = parent } - - @objc func picked(_ sender: NSPopUpButton) { - let index = sender.indexOfSelectedItem - guard parent.engines.indices.contains(index) else { return } - parent.selection = parent.engines[index] - } + .labelsHidden() + .pickerStyle(.menu) + .controlSize(.small) + // In automatic mode the column shows what MacPacker picked; editing it + // would imply a choice that is not being honoured. + .disabled(isAutomatic) } } -