From d3cbd7f04b1cab847816f57cfa3d47b920485306 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 4 Oct 2026 09:32:34 +0200 Subject: [PATCH 01/12] fix: compress large folders without freezing the app (#278) A folder with 20,000 files took 11 seconds before its compress started, with the app frozen, and then wrote for half a minute with nothing on screen. Three causes, all fixed here. The folder was read on the main actor. ArchiveState.add walked it synchronously, and most of the time went into bookkeeping rather than reading: `diff` and `entries` are published, and changed entry by entry every append copied the whole collection. ArchiveScanner, a new actor next to the loader, the extractor and the saver, now reads what is to be added off the main actor, one lstat per entry. ArchiveState only awaits it and puts the result into the archive in one change. Adds queue up in the order they were started, an add for an archive the window no longer shows is dropped, and a save is refused while one is pending. Removing a folder is one change as well. Compress never reported to the progress center, and Finder's compress has no window of its own. It registers a job now, before anything is read: the progress window comes up after its usual delay, can cancel, stays up with the reason when the compress fails, and the quit warning covers it. Quick Compress and the start page keep their own rows. A metadata sidecar was packed for every entry to find out that it says nothing. Only entries that carry an attribute worth keeping, or the hidden flag, are packed now. What goes into the archive is unchanged. --- MacPacker/AppDelegate.swift | 23 +- .../UrlHandling/AppUrlCompressHandler.swift | 14 +- .../ArchiveContentToolbarView.swift | 4 +- .../ArchiveWindow/ArchiveWindowManager.swift | 4 +- .../ExtractionProgressWindowController.swift | 10 +- .../LaunchParameters/LaunchParameters.swift | 6 +- MacPacker/Localizable.xcstrings | 36 ++ Modules/Sources/Core/ArchiveSaver.swift | 28 +- Modules/Sources/Core/ArchiveScanner.swift | 143 +++++ Modules/Sources/Core/ArchiveState.swift | 454 +++++++++++----- .../Core/Extensions/URL+Extensions.swift | 29 -- .../Extraction/ExtractionProgressCenter.swift | 47 +- Modules/Sources/Core/Models/ArchiveItem.swift | 24 - .../Sources/Swift7zip/SevenZipWriter.swift | 24 +- Modules/Tests/CoreTests/FolderAddTests.swift | 487 ++++++++++++++++++ .../CoreTests/SaveOptionsStateTests.swift | 20 +- Modules/Tests/CoreTests/ZipWriteTests.swift | 26 +- 17 files changed, 1123 insertions(+), 256 deletions(-) create mode 100644 Modules/Sources/Core/ArchiveScanner.swift delete mode 100644 Modules/Sources/Core/Extensions/URL+Extensions.swift create mode 100644 Modules/Tests/CoreTests/FolderAddTests.swift diff --git a/MacPacker/AppDelegate.swift b/MacPacker/AppDelegate.swift index c94ade51..408eb144 100644 --- a/MacPacker/AppDelegate.swift +++ b/MacPacker/AppDelegate.swift @@ -271,30 +271,39 @@ final class AppDelegate: NSObject, NSApplicationDelegate, ObservableObject { } /// Asked for in #119: don't let the app quit silently while an - /// extraction is running. + /// extraction is running. Nor while Finder's compress is: quitting leaves + /// the archive half written under the name it was going to have. public func applicationShouldTerminate(_ sender: NSApplication) -> NSApplication.TerminateReply { #if DEBUG // The extraction demo keeps a job "active" on purpose; don't let the // quit-guard alert block the relaunches that drive its screenshots. if ExtractionDemo.isRequested { return .terminateNow } #endif - guard ExtractionProgressCenter.shared.hasActiveJobs else { + guard let running = ExtractionProgressCenter.shared.runningKind else { return .terminateNow } - log.notice("Quit requested while extraction is running — asking user") + // "extraction" / "compression": the lines an extraction always logged, as they were + let what = running == .extraction ? "extraction" : "compression" + log.notice("Quit requested while \(what) is running — asking user") let alert = NSAlert() alert.alertStyle = .warning - alert.messageText = String(localized: .appExtractionInProgress) - alert.informativeText = String(localized: .appQuitDuringExtractionWarning) + switch running { + case .extraction: + alert.messageText = String(localized: .appExtractionInProgress) + alert.informativeText = String(localized: .appQuitDuringExtractionWarning) + case .compression: + alert.messageText = String(localized: .appCompressionInProgress) + alert.informativeText = String(localized: .appQuitDuringCompressionWarning) + } alert.addButton(withTitle: String(localized: .commonCancel)) alert.addButton(withTitle: String(localized: .appQuitAnyway)) if alert.runModal() == .alertFirstButtonReturn { - log.notice("Quit cancelled — extraction continues") + log.notice("Quit cancelled — \(what) continues") return .terminateCancel } - log.notice("Quit forced during extraction") + log.notice("Quit forced during \(what)") return .terminateNow } diff --git a/MacPacker/Core/UrlHandling/AppUrlCompressHandler.swift b/MacPacker/Core/UrlHandling/AppUrlCompressHandler.swift index 8421427f..bdb698bc 100644 --- a/MacPacker/Core/UrlHandling/AppUrlCompressHandler.swift +++ b/MacPacker/Core/UrlHandling/AppUrlCompressHandler.swift @@ -15,6 +15,10 @@ private let log = tb.Logger(subsystem: "app.MacPacker", category: "url") /// Writes `items` into a new archive at `destination` through the same /// add/save path the UI uses; the format follows the destination's extension. +/// +/// There is no window to show it in, so it goes to the progress window: that +/// comes up once the compress takes long enough, can cancel it, and stays up +/// to say why it failed. /// - Returns: whether the archive was written. @MainActor private func writeArchive( @@ -25,10 +29,12 @@ private func writeArchive( ) async -> Bool { log.notice("Compressing \(items.count) item(s) to \(destination.lastPathComponent)") let state = ArchiveState(catalog: catalog, engineSelector: engineSelector) - await state.compress(items, to: destination) - - if let error = state.error { - log.error("Compress failed", context: ["error": error]) + guard await state.compress(items, to: destination, showingProgress: true) else { + if let error = state.error { + log.error("Compress failed", context: ["error": error]) + } else { + log.notice("Compress cancelled", context: ["file": destination.lastPathComponent]) + } return false } log.notice("Compress done", context: ["file": destination.lastPathComponent]) diff --git a/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift b/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift index e75eb6a8..d3cc4eb9 100644 --- a/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift +++ b/MacPacker/Features/ArchiveContentViewer/ArchiveContentToolbarView.swift @@ -39,9 +39,7 @@ struct ArchiveContentToolbarView: ToolbarContent { panel.begin { response in guard response == .OK else { return } Task { @MainActor in - for url in panel.urls { - state.add(url: url) - } + state.add(urls: panel.urls) } } } diff --git a/MacPacker/Features/ArchiveWindow/ArchiveWindowManager.swift b/MacPacker/Features/ArchiveWindow/ArchiveWindowManager.swift index 03151417..93e67e55 100644 --- a/MacPacker/Features/ArchiveWindow/ArchiveWindowManager.swift +++ b/MacPacker/Features/ArchiveWindow/ArchiveWindowManager.swift @@ -138,8 +138,8 @@ class ArchiveWindowManager { func openCreateArchiveWindow(with files: [URL] = []) -> ArchiveState { let state = createAndShowArchiveWindow(nil) state.create(named: String(localized: .commonNewArchive)) - for file in files { - state.add(url: file) + if !files.isEmpty { + state.add(urls: files) } return state } diff --git a/MacPacker/Features/ExtractionProgress/ExtractionProgressWindowController.swift b/MacPacker/Features/ExtractionProgress/ExtractionProgressWindowController.swift index c4fd4ba8..b37279cc 100644 --- a/MacPacker/Features/ExtractionProgress/ExtractionProgressWindowController.swift +++ b/MacPacker/Features/ExtractionProgress/ExtractionProgressWindowController.swift @@ -13,10 +13,10 @@ import tb private let log = tb.Logger(subsystem: "app.MacPacker", category: "extraction") -/// Owns the extraction progress window. It observes the shared +/// Owns the progress window. It observes the shared /// `ExtractionProgressCenter` and /// - shows itself as soon as a job is running (also for window-less flows -/// like the Finder extension's "Extract here"), +/// like the Finder extension's "Extract here" and "Compress to …"), /// - closes itself shortly after all jobs finished successfully, /// - stays open when a job failed so the error remains visible. @MainActor @@ -103,6 +103,12 @@ final class ExtractionProgressWindowController: NSWindowController, NSWindowDele if hasActive { + // Named after what is running. The title bar shows no text; VoiceOver + // and Mission Control read it. + let title = String(localized: center.runningKind == .compression ? .commonCompressing : .commonExtracting) + if window?.title != title { + window?.title = title + } scheduledClose?.cancel() scheduledClose = nil scheduleShowIfNeeded() diff --git a/MacPacker/Features/LaunchParameters/LaunchParameters.swift b/MacPacker/Features/LaunchParameters/LaunchParameters.swift index 3c7f9d20..e27e64c3 100644 --- a/MacPacker/Features/LaunchParameters/LaunchParameters.swift +++ b/MacPacker/Features/LaunchParameters/LaunchParameters.swift @@ -239,6 +239,8 @@ enum LaunchParameters { _ state: ArchiveState, navigate: String?, add: [URL], search: String?, select: String? ) async { try? await state.openTask?.value + // what a new archive was opened with is read off the main actor + _ = await state.addTask?.value if let navigate, !navigate.isEmpty { for segment in navigate.split(separator: "/").map(String.init) { @@ -250,8 +252,8 @@ enum LaunchParameters { } } - for file in add { - state.add(url: file) + if !add.isEmpty { + _ = await state.add(urls: add).value } if let search, !search.isEmpty { diff --git a/MacPacker/Localizable.xcstrings b/MacPacker/Localizable.xcstrings index 453bc36d..050e903e 100644 --- a/MacPacker/Localizable.xcstrings +++ b/MacPacker/Localizable.xcstrings @@ -1,6 +1,18 @@ { "sourceLanguage": "en", "strings": { + "appCompressionInProgress": { + "comment": "Title of the alert shown when the user quits while Finder's \"Compress to …\" is still writing an archive.", + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "A compression is still in progress" + } + } + } + }, "appExtractionInProgress": { "comment": "Title of the alert shown when the user quits while an extraction is running.", "extractionState": "manual", @@ -217,6 +229,18 @@ } } }, + "appQuitDuringCompressionWarning": { + "comment": "Body of the alert shown when the user quits while Finder's \"Compress to …\" is still writing an archive.", + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Quitting now stops the compression and may leave an incomplete archive at the destination." + } + } + } + }, "appQuitDuringExtractionWarning": { "comment": "Body of the alert shown when the user quits while an extraction is running.", "extractionState": "manual", @@ -6585,6 +6609,18 @@ } } }, + "commonCompressing": { + "comment": "Name of the progress window while it shows an archive being written from Finder's \"Compress to …\". The title bar shows no text; VoiceOver and Mission Control read it.", + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Compressing" + } + } + } + }, "commonCompression": { "comment": "Label of the compression level picker in the archive options", "extractionState": "manual", diff --git a/Modules/Sources/Core/ArchiveSaver.swift b/Modules/Sources/Core/ArchiveSaver.swift index 4e0d84f1..1efca499 100644 --- a/Modules/Sources/Core/ArchiveSaver.swift +++ b/Modules/Sources/Core/ArchiveSaver.swift @@ -43,8 +43,11 @@ final actor ArchiveSaver { private let sourcePassword: String? private let passwordResolver: ArchivePasswordResolver private let folderAccessProvider: ArchiveFolderAccessUserProvider? - /// Percent written, on the main actor. - private let onProgress: @MainActor @Sendable (Int) -> Void + /// Bytes written, of how many, and when 7-Zip said so — on the main actor. + private let onProgress: @MainActor @Sendable (_ completed: UInt64, _ total: UInt64, _ at: Date) -> Void + /// Set to stop the write. 7-Zip is told at its next progress report, and + /// removes what it had written. + private let cancel: ExtractionCancelFlag? /// How often the source's password is asked for before the save gives up. static let passwordPrompts = 5 @@ -60,7 +63,8 @@ final actor ArchiveSaver { sourcePassword: String?, passwordResolver: @escaping ArchivePasswordResolver, folderAccessProvider: ArchiveFolderAccessUserProvider?, - onProgress: @escaping @MainActor @Sendable (Int) -> Void + onProgress: @escaping @MainActor @Sendable (_ completed: UInt64, _ total: UInt64, _ at: Date) -> Void, + cancel: ExtractionCancelFlag? = nil ) { self.source = source self.splitArchiveName = splitArchiveName @@ -73,6 +77,7 @@ final actor ArchiveSaver { self.passwordResolver = passwordResolver self.folderAccessProvider = folderAccessProvider self.onProgress = onProgress + self.cancel = cancel } func save() async throws -> Saved { @@ -178,20 +183,21 @@ final actor ArchiveSaver { } } - /// 7-Zip's byte progress as a percentage. The callback fires on the writing - /// (GCD) thread: throttled, then passed to the main actor. + /// 7-Zip's byte progress. The callback fires on the writing (GCD) thread: + /// throttled, then passed to the main actor with the time it was emitted — + /// reports reach the main queue in bursts, and speed is worked out from how + /// far apart they really were. What it returns is whether to go on. private func progressHandler() -> @Sendable (UInt64, UInt64) -> Bool { let throttle = ProgressThrottle() - let onProgress = onProgress + let (onProgress, cancel) = (onProgress, cancel) return { completed, total in - guard total > 0 else { return true } - let percent = Int((completed * 100) / total) - if throttle.shouldEmit(at: Date()) { + let now = Date() + if total > 0, throttle.shouldEmit(at: now) { DispatchQueue.main.async { - MainActor.assumeIsolated { onProgress(percent) } + MainActor.assumeIsolated { onProgress(completed, total, now) } } } - return true + return cancel?.isCancelled != true } } diff --git a/Modules/Sources/Core/ArchiveScanner.swift b/Modules/Sources/Core/ArchiveScanner.swift new file mode 100644 index 00000000..f2e18347 --- /dev/null +++ b/Modules/Sources/Core/ArchiveScanner.swift @@ -0,0 +1,143 @@ +// +// ArchiveScanner.swift +// Modules +// +// What is about to be added, read from disk: every file and folder below the +// ones picked, in the order the archive gets them. +// + +import Foundation +import Swift7zip +import tb + +private let log = tb.Logger(subsystem: "app.MacPacker", category: "archive") + +/// What a scan found. +struct ArchiveScan: Sendable { + struct Entry: Sendable { + /// What the window shows for it. Below the top it is already attached to + /// the folder it is in. + let item: ArchiveItem + /// What a save writes for it. + let update: ArchiveUpdateItem + /// Where in `entries` the folder it is in sits; `nil` for what was picked + /// itself. + let parent: Int? + } + + /// A folder comes before everything in it. + var entries: [Entry] = [] + /// Why a folder was left out, and with it all it holds; `nil` when + /// everything could be read. + var unreadable: String? +} + +/// Off the main actor, like the loader, the extractor and the saver: a folder +/// can hold any number of files, and each one is a trip to the disk. Read on the +/// main actor, a project of 20,000 files kept the app from drawing for 11 +/// seconds (#278). +final actor ArchiveScanner { + + /// - Parameters: + /// - urls: the files and folders picked + /// - base: where they go in the archive: empty for its top, else the + /// folder's path with a trailing slash + /// - cancel: looked at before each folder is read + /// - Throws: `CancellationError` once `cancel` is set. + func scan(_ urls: [URL], under base: String, cancel: ExtractionCancelFlag) async throws -> ArchiveScan { + try await runBlocking { + var scan = ArchiveScan() + for url in urls { + let status = Self.status(of: url) + let archivePath = base + url.lastPathComponent + if status?.isFolder == true { + // Reading a folder needs a stored grant's scope held: unlike + // a drop or a panel, a grant reused from storage gives no + // access outside it (#278). Only a folder: what there is to + // know about a file is readable without, and looking the + // grant up is a trip to another process for each one. + try Sandbox.accessSync(url: url) { + try Self.read(url, status, as: archivePath, in: nil, into: &scan, cancel: cancel) + } + } else { + try Self.read(url, status, as: archivePath, in: nil, into: &scan, cancel: cancel) + } + } + return scan + } + } + + /// All there is to know about `url`, in one call; `nil` when it can't be + /// read. `lstat`: a link is an entry of its own, whatever it points at. + private static func status(of url: URL) -> stat? { + var status = stat() + return lstat(url.path, &status) == 0 ? status : nil + } + + private static func read( + _ url: URL, + _ status: stat?, + as archivePath: String, + in parent: Int?, + into scan: inout ArchiveScan, + cancel: ExtractionCancelFlag + ) throws { + let isFolder = status?.isFolder == true + + var contents: [URL] = [] + if isFolder { + guard !cancel.isCancelled else { throw CancellationError() } + // Read before the folder goes in: one that can't be read must not be + // added empty, which would silently drop what it holds on save. It + // is left out, and the reason kept. + do { + contents = try FileManager.default.contentsOfDirectory(at: url, includingPropertiesForKeys: nil) + } catch { + log.error("Failed to read folder for add — skipping", context: [ + "path": url.path, + "error": String(describing: error) + ]) + scan.unreadable = error.localizedDescription + return + } + } + + let item = ArchiveItem( + name: url.lastPathComponent, + virtualPath: archivePath, + type: isFolder ? .directory : .file, + parent: parent.map { scan.entries[$0].item.id }, + uncompressedSize: status.map { Int($0.st_size) }, + // summed the way Foundation does it, so the date is the very one + // `attributesOfItem` gives + modificationDate: status.map { + Date(timeIntervalSinceReferenceDate: TimeInterval($0.st_mtimespec.tv_sec) + - Date.timeIntervalBetween1970AndReferenceDate + + 1.0e-9 * TimeInterval($0.st_mtimespec.tv_nsec)) + }, + posixPermissions: status.map { Int($0.st_mode & ~S_IFMT) }) + if let parent { + scan.entries[parent].item.addChild(item.id) + } + let position = scan.entries.count + scan.entries.append(.init( + item: item, + // The folder's own URL travels with its entry: a custom folder icon + // is a flag on the folder itself, not only the hidden file inside it. + update: isFolder + ? .addDirectory(archivePath: archivePath, diskPath: url) + : .addFile(archivePath: archivePath, diskPath: url), + parent: parent)) + + let named = contents.map { (name: $0.lastPathComponent, url: $0) }.sorted { $0.name < $1.name } + for child in named { + try read( + child.url, Self.status(of: child.url), + as: archivePath + "/" + child.name, in: position, into: &scan, cancel: cancel) + } + } +} + +private extension stat { + var isFolder: Bool { st_mode & S_IFMT == S_IFDIR } +} diff --git a/Modules/Sources/Core/ArchiveState.swift b/Modules/Sources/Core/ArchiveState.swift index 3716b2b4..8a8f828a 100644 --- a/Modules/Sources/Core/ArchiveState.swift +++ b/Modules/Sources/Core/ArchiveState.swift @@ -29,6 +29,16 @@ public enum ArchiveSortOrder: String { case posixPermissions } +private extension ArchiveUpdateItem { + /// Where an addition goes in the archive; `nil` for a removal or a move. + var addedPath: String? { + switch self { + case .addFile(let path, _, _, _), .addDirectory(let path, _, _, _), .addData(let path, _, _, _): path + case .remove, .move: nil + } + } +} + @MainActor public class ArchiveState: ObservableObject { @Published private(set) public var hasArchive: Bool = false @@ -157,6 +167,20 @@ public class ArchiveState: ObservableObject { private var openedFile: FileStamp? public private(set) var openTask: Task? + /// The add started last. What it adds is read off the main actor, so it is + /// there once this ends. + public private(set) var addTask: Task? + /// Adds started and not yet done. Nothing is saved before what they are + /// reading is in the archive. + private var pendingAdds = 0 + /// Stops the add that is reading right now. + private var addCancel: ExtractionCancelFlag? + /// Bumped by a cancel, so the adds waiting their turn go with the one that + /// was reading: several files dropped together are one add each. + private var addCancellations = 0 + /// Bumped whenever the archive in the window is replaced or closed, so an + /// add that was reading for the one before can tell. + private var contentGeneration = 0 private var archiveLoader: ArchiveLoader? public init(catalog: ArchiveTypeCatalog, engineSelector: ArchiveEngineSelectorProtocol) { @@ -237,6 +261,11 @@ extension ArchiveState { /// Resets the state of the archive private func reset() { + // whatever is still being read for an add was meant for what goes now + contentGeneration += 1 + addCancel?.cancel() + addCancel = nil + self.hasArchive = false self.canBeEdited = false // A failed open sets its reason after this, so its alert still shows. @@ -298,6 +327,14 @@ extension ArchiveState { /// Cancels the current operation which can be either loading the archive or extracting /// anything from the archive public func cancelCurrentOperation() { + // Files being read for an add: stop reading, and leave the archive as + // it is. Below is for a load, and ends in a reset. + if let addCancel { + addCancellations += 1 + addCancel.cancel() + return + } + openTask?.cancel() // Capture what is being cancelled before suspending. Awaiting the loader @@ -544,124 +581,226 @@ extension ArchiveState { loadChildren() } - /// - Returns: whether all of `url` was added. A folder that can't be read - /// is left out, with the reason in `error`, and so is one inside it. + /// Adds files and folders from disk where the window is; a folder goes in + /// with everything it holds. + /// + /// They are read off the main actor, by `ArchiveScanner`, so what was added + /// is there once the returned task ends. An add started meanwhile waits its + /// turn, and what it adds comes after. + /// + /// - Returns: the add. Its value is whether all of `urls` was added: a + /// folder that can't be read is left out, with the reason in `error`, and + /// so is one inside it. + @discardableResult + public func add(urls: [URL]) -> Task { + add(urls: urls, cancel: ExtractionCancelFlag()) + } + @discardableResult - public func add(url: URL) -> Bool { + public func add(url: URL) -> Task { + add(urls: [url]) + } + + private func add(urls: [URL], cancel: ExtractionCancelFlag) -> Task { + // Where the window is now is where it goes, wherever the window is by + // the time it has been read. + let target = selectedItem + let generation = contentGeneration + let cancellations = addCancellations + let previous = addTask + pendingAdds += 1 + let task = Task { + _ = await previous?.value + var added = false + // unless it was cancelled while it waited its turn + if cancellations == addCancellations { + added = await scanAndAdd(urls, under: target, of: generation, cancel: cancel) + } + pendingAdds -= 1 + return added + } + addTask = task + return task + } + + /// - Parameters: + /// - target: the folder shown when the add was asked for + /// - generation: the archive the window held then + private func scanAndAdd( + _ urls: [URL], + under target: ArchiveItem?, + of generation: Int, + cancel: ExtractionCancelFlag + ) async -> Bool { guard !isSaving else { - log.notice("Ignoring add — a save is in progress", context: ["file": url.lastPathComponent]) + log.notice("Ignoring add — a save is in progress", context: ["files": "\(urls.count)"]) return false } - guard let selectedItem else { return false } + // `create`, `open` or closing the window replaced the archive this was + // for: whatever the window shows now is not where it goes. + guard let target, generation == contentGeneration else { return false } // 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]) + guard !isWithinOpenedArchive(target) else { + log.notice("Ignoring add — inside an archive opened within this one", context: ["files": "\(urls.count)"]) return false } - let base = (selectedItem.virtualPath?.isEmpty == false && selectedItem.virtualPath != "/") - ? selectedItem.virtualPath! + "/" : "" - - let added: Bool - if url.isDirectory { - // Folders are added recursively so their contents end up in the - // archive. Listing one needs a stored grant's scope held: unlike a - // drop or a panel, a grant reused from storage gives no access - // outside it (#278). - added = Sandbox.accessSync(url: url) { - addFolder(url: url, archivePath: base + url.lastPathComponent, under: selectedItem) - } - } else { - addFile(url: url, archivePath: base + url.lastPathComponent, under: selectedItem) - added = true - } + let base = (target.virtualPath?.isEmpty == false && target.virtualPath != "/") + ? target.virtualPath! + "/" : "" + isBusy = true + addCancel = cancel + updateStatus(.processing) + updateStatusText(String(localized: "loading...", bundle: .module, comment: "Archive operation status")) + let scan = try? await ArchiveScanner().scan(urls, under: base, cancel: cancel) + + // replaced while it was read: the same, and the busy state is no longer + // this add's to clear + guard generation == contentGeneration else { return false } + isBusy = false + addCancel = nil + updateStatusText(nil) + updateStatus(.done) + // cancelled, or the folder it was going into was removed meanwhile + guard let scan, target === root || entries[target.id] != nil else { return false } + + put(scan, under: target) + if let unreadable = scan.unreadable { + self.error = unreadable + } self.isReloadNeeded = true loadChildren() - return added + return scan.unreadable == nil + } + + /// Puts what a scan read under `target`, in one change to `entries` and one + /// to `diff`. Both are published: changed entry by entry, every change + /// copies the whole collection, and with 20,000 files that alone took most + /// of 11 seconds (#278). + /// + /// A name the archive already holds is replaced, or the file the user meant + /// to replace would still be in there next to its replacement. Except a + /// folder over a folder: that one is merged into, so only the files that + /// collide inside it are replaced and the rest of what it holds stays. + private func put(_ scan: ArchiveScan, under target: ArchiveItem) { + var entries = self.entries + var replaced = Removal() + var additions: [ArchiveUpdateItem] = [] + additions.reserveCapacity(scan.entries.count) + // What each scanned entry ended up as: itself, or the folder already + // there that it was merged into. + var placed: [ArchiveItem] = [] + placed.reserveCapacity(scan.entries.count) + // The names taken in the folders that were there before this add. Only + // there can anything collide: below a folder the add brought itself, + // the disk has kept the names apart already. + var taken: [UUID: [String: ArchiveItem]] = [:] + + for entry in scan.entries { + let item = entry.item + let parent = entry.parent.map { placed[$0] } ?? target + let broughtWithItsFolder = entry.parent.map { placed[$0] === scan.entries[$0].item } ?? false + if !broughtWithItsFolder { + // taken out and put back, so that it is changed in place + var names = taken.removeValue(forKey: parent.id) ?? Dictionary( + (parent.children ?? []).compactMap { entries[$0] }.map { ($0.name, $0) }, + uniquingKeysWith: { first, _ in first }) + defer { taken[parent.id] = names } + + let existing = names[item.name] + if let existing, existing.isFolder, item.isFolder { + placed.append(existing) + continue + } + if let existing { + var gone = Removal() + take(existing, outOf: &entries, into: &gone) + parent.removeChild(existing.id) + // Brought by this very add, by an earlier one of its files + // with the same name: not in `diff` yet for `drop` to find. + if self.entries[existing.id] == nil { + additions.removeAll { $0.addedPath.map(gone.pendingPaths.contains) ?? false } + } + replaced.formUnion(gone) + } + names[item.name] = item + item.parent = parent.id + parent.addChild(item.id) + } + entries[item.id] = item + additions.append(entry.update) + placed.append(item) + } + + var diff = self.diff + drop(replaced, from: &diff) + diff.append(contentsOf: additions) + self.entries = entries + self.diff = diff + if !replaced.isEmpty { + // a replaced item must not stay selected — it is gone from `entries` + selectedItems = selectedItems.filter { entries[$0.id] != nil } + } + } + + /// How a save ended. + private enum SaveOutcome { + case written + case failed + case cancelled } /// Makes this fresh state a new archive at `destination` holding `items`, /// written in one go: Quick Compress and the Finder's compress entries. - /// When it fails, `error` says why. /// /// Writes nothing once an item can't be read in full. Left out, it would /// leave an archive short of what was asked for that still reports done — /// for a single folder, an empty one (#278). - public func compress(_ items: [URL], to destination: URL, options: CompressionOptions? = nil) async { + /// + /// - Parameter showingProgress: for a compress with no window of its own to + /// show its progress in — the Finder's entries. It is reported to the + /// progress center like an extraction: the progress window comes up once + /// it takes long enough, can cancel it, and stays up to say why it failed; + /// and the app asks before it quits in the middle of it. + /// - Returns: whether the archive was written. When it was not, `error` says + /// why — unless it was cancelled. + @discardableResult + public func compress( + _ items: [URL], + to destination: URL, + options: CompressionOptions? = nil, + showingProgress: Bool = false + ) async -> Bool { create() - for item in items { - guard add(url: item) else { return } - } - await save(to: destination, options: options)?.value - } - /// The item currently sitting under `name` in `parent` — a real archive - /// entry or a pending addition — if there is one. - private func child(named name: String, under parent: ArchiveItem) -> ArchiveItem? { - parent.children?.lazy.compactMap { self.entries[$0] }.first { $0.name == name } - } - - private func addFile(url: URL, archivePath: String, under parent: ArchiveItem) { - // Adding over a name the archive already holds replaces it. Without - // dropping the old entry the archive keeps both, so the file the user - // meant to replace is still in there next to its replacement. - if let existing = child(named: url.lastPathComponent, under: parent) { - discard(items: [existing]) - } - diff.append(.addFile(archivePath: archivePath, diskPath: url)) - let item = ArchiveItem(url: url, archivePath: archivePath) - item.parent = parent.id - parent.addChild(item.id) - entries[item.id] = item - } - - /// - Returns: whether `url` and every folder below it could be read. - private func addFolder(url: URL, archivePath: String, under parent: ArchiveItem) -> Bool { - // Read the contents first: if the folder can't be enumerated we must not - // add it as an empty directory (that would silently drop its real - // contents on save). Skip it and surface the error instead. - let children: [URL] - do { - children = try FileManager.default.contentsOfDirectory( - at: url, includingPropertiesForKeys: nil) - } catch { - log.error("Failed to read folder for add — skipping", context: [ - "path": url.path, - "error": String(describing: error) - ]) - self.error = error.localizedDescription - return false + let cancel = ExtractionCancelFlag() + // Before anything is read: reading a large folder is part of the wait. + let job = showingProgress ? progressCenter.begin( + kind: .compression, + archiveName: destination.lastPathComponent, + destination: destination.deletingLastPathComponent(), + itemCount: items.count, + totalBytes: nil + ) : nil + if let job { + progressCenter.setOnCancel(job) { cancel.cancel() } } - // A folder that is already there is merged into, not added a second - // time: only the files that collide inside it are replaced, the rest - // of what it holds stays. Anything else drops the whole folder. - let existing = child(named: url.lastPathComponent, under: parent) - let item: ArchiveItem - if let existing, existing.isFolder { - item = existing - } else { - if let existing { discard(items: [existing]) } - // The folder's own URL travels with the entry: a custom folder icon - // is a flag on the folder itself, not only the hidden file inside it. - diff.append(.addDirectory(archivePath: archivePath, diskPath: url)) - item = ArchiveItem(url: url, archivePath: archivePath) - item.parent = parent.id - parent.addChild(item.id) - entries[item.id] = item + var outcome = SaveOutcome.failed + if await add(urls: items, cancel: cancel).value { + outcome = await startSave(to: destination, options: options, job: job, cancel: cancel)?.value ?? .failed + } else if cancel.isCancelled { + outcome = .cancelled } - var complete = true - for child in children.sorted(by: { $0.lastPathComponent < $1.lastPathComponent }) { - if child.isDirectory { - complete = addFolder(url: child, archivePath: archivePath + "/" + child.lastPathComponent, under: item) - && complete - } else { - addFile(url: child, archivePath: archivePath + "/" + child.lastPathComponent, under: item) + if let job { + switch outcome { + case .written: progressCenter.finish(job, .done) + case .cancelled: progressCenter.finish(job, .cancelled) + case .failed: progressCenter.finish(job, .failed(error ?? "")) } } - return complete + return outcome == .written } /// Removes the given items (files or folders, including everything below @@ -678,12 +817,24 @@ extension ArchiveState { return } - let dropped = discard(items: items) + // One change to `entries` and one to `diff`, as for an add: see `put`. + var entries = self.entries + var removal = Removal() + for item in items { + take(item, outOf: &entries, into: &removal) + if let parentId = item.parent { + entries[parentId]?.removeChild(item.id) + } + } + var diff = self.diff + drop(removal, from: &diff) + self.entries = entries + self.diff = diff log.notice("Marked items for removal", context: [ "items": "\(items.count)", - "archiveEntries": "\(dropped.archiveEntries)", - "pendingAdds": "\(dropped.pendingAdds)" + "archiveEntries": "\(removal.indices.count)", + "pendingAdds": "\(removal.pendingPaths.count)" ]) selectedItems = [] @@ -691,68 +842,57 @@ extension ArchiveState { loadChildren() } - /// Takes the given items (and everything below them) out of the tree and - /// records that in the diff: entries that exist in the archive on disk - /// become removals, pending (unsaved) additions are dropped from the diff - /// again. Used both by delete and by an add that replaces an existing item. - @discardableResult - private func discard(items: [ArchiveItem]) -> (archiveEntries: Int, pendingAdds: Int) { - var removedIndices: Set = [] - var droppedAddPaths: Set = [] - for item in items { - collectRemovals(item, indices: &removedIndices, pendingPaths: &droppedAddPaths) - if let parentId = item.parent { - entries[parentId]?.removeChild(item.id) - } - } + /// What taking items out of the tree comes to in the diff. + private struct Removal { + /// Entries the archive on disk has: a save removes them. + var indices: Set = [] + /// Additions not saved yet: dropped from the diff again. + var pendingPaths: Set = [] - // pending (unsaved) additions are simply dropped from the diff - if !droppedAddPaths.isEmpty { - diff.removeAll { entry in - switch entry { - case .addFile(let p, _, _, _), .addDirectory(let p, _, _, _), .addData(let p, _, _, _): - return droppedAddPaths.contains(p) - default: - return false - } - } - } - // entries that exist in the archive on disk are removed on save - diff.append(contentsOf: removedIndices.sorted().map { .remove(sourceIndex: $0) }) - - // a discarded item must not stay selected — it is gone from `entries` - selectedItems = selectedItems.filter { entries[$0.id] != nil } + var isEmpty: Bool { indices.isEmpty && pendingPaths.isEmpty } - return (removedIndices.count, droppedAddPaths.count) + mutating func formUnion(_ other: Removal) { + indices.formUnion(other.indices) + pendingPaths.formUnion(other.pendingPaths) + } } - /// 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`. 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( + /// Depth-first: takes `item` and all of its descendants out of `entries`, + /// and notes what that comes to in the diff. 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 take( _ item: ArchiveItem, - indices: inout Set, - pendingPaths: inout Set, + outOf entries: inout [UUID: ArchiveItem], + into removal: inout Removal, inThisArchive: Bool = true ) { for childId in item.children ?? [] { if let child = entries[childId] { - collectRemovals(child, indices: &indices, pendingPaths: &pendingPaths, - inThisArchive: inThisArchive && item.archiveTypeId == nil) + take(child, outOf: &entries, into: &removal, + inThisArchive: inThisArchive && item.archiveTypeId == nil) } } if inThisArchive { if let index = item.index { - indices.insert(index) + removal.indices.insert(index) } else if let path = item.virtualPath, path != "/" { - pendingPaths.insert(path) + removal.pendingPaths.insert(path) } } entries.removeValue(forKey: item.id) } + /// Records in `diff` that what `removal` names is gone. + private func drop(_ removal: Removal, from diff: inout [ArchiveUpdateItem]) { + // pending (unsaved) additions are simply dropped from the diff + if !removal.pendingPaths.isEmpty { + diff.removeAll { $0.addedPath.map(removal.pendingPaths.contains) ?? false } + } + // entries that exist in the archive on disk are removed on save + diff.append(contentsOf: removal.indices.sorted().map { .remove(sourceIndex: $0) }) + } + /// 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 { @@ -811,10 +951,29 @@ extension ArchiveState { to destination: URL? = nil, options: CompressionOptions? = nil ) -> Task? { + guard let saving = startSave(to: destination, options: options, job: nil, cancel: nil) else { return nil } + return Task { _ = await saving.value } + } + + /// - Parameters: + /// - job: the progress center's job the write reports its bytes to, if it has one + /// - cancel: set to stop the write; what it had written by then is removed + private func startSave( + to destination: URL?, + options: CompressionOptions?, + job: UUID?, + cancel: ExtractionCancelFlag? + ) -> Task? { guard !isSaving else { log.notice("Ignoring save — a save is already in progress") return nil } + // What is still being read is not in `diff` yet: saved now, the archive + // would be written without it. + guard pendingAdds == 0 else { + log.notice("Ignoring save — files are still being added") + return nil + } guard let target = destination ?? url else { return nil } // Save with nothing pending is a no-op. A Save As is not, even of a clean // archive onto its own file: its options have to reach every entry. @@ -839,7 +998,14 @@ extension ArchiveState { sourcePassword: url.flatMap { passwords[$0] }, passwordResolver: makePasswordResolver(), folderAccessProvider: folderAccessProvider, - onProgress: { [weak self] percent in self?.progress = percent }) + onProgress: { [weak self, progressCenter] completed, total, date in + self?.progress = Int((completed * 100) / total) + if let job { + progressCenter.reportEngineProgress( + job, completed: Int64(completed), total: Int64(total), at: date) + } + }, + cancel: cancel) isSaving = true isBusy = true @@ -873,6 +1039,17 @@ extension ArchiveState { pinnedEngines = engines _ = try? await openTask?.value self.isSaving = false + return .written + } catch SevenZipError.cancelled { + // Stopped on request, and nothing half-written is left behind: + // not a failure, and what was pending still is. + log.notice("Archive save cancelled", context: ["target": target.lastPathComponent]) + self.isBusy = false + self.isSaving = false + self.progress = nil + updateStatusText(nil) + updateStatus(.done) + return .cancelled } catch { log.error("Archive save failed", context: [ "target": target.lastPathComponent, @@ -885,6 +1062,7 @@ extension ArchiveState { self.progress = nil updateStatusText(nil) updateStatus(.done) + return .failed } } } diff --git a/Modules/Sources/Core/Extensions/URL+Extensions.swift b/Modules/Sources/Core/Extensions/URL+Extensions.swift deleted file mode 100644 index ced9d49d..00000000 --- a/Modules/Sources/Core/Extensions/URL+Extensions.swift +++ /dev/null @@ -1,29 +0,0 @@ -// -// Url+Extensions.swift -// MacPacker -// -// Created by Stephan Arenswald on 11.10.23. -// - -import Foundation - -extension URL { - var isDirectory: Bool { - (try? resourceValues(forKeys: [.isDirectoryKey]))?.isDirectory == true - } - - var fileSize: Int? { - let attrs = try? FileManager.default.attributesOfItem(atPath: path) - return (attrs?[.size] as? NSNumber)?.intValue - } - - var permissions: Int? { - let attrs = try? FileManager.default.attributesOfItem(atPath: path) - return (attrs?[.posixPermissions] as? NSNumber)?.intValue - } - - var modificationDate: Date? { - let attrs = try? FileManager.default.attributesOfItem(atPath: path) - return (attrs?[.modificationDate] as? Date) - } -} diff --git a/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift b/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift index cddf5861..cf8dcbed 100644 --- a/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift +++ b/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift @@ -21,7 +21,8 @@ public struct ExtractionSpeedSample: Equatable, Sendable { public let bytesPerSecond: Double } -/// One user-visible extraction run (e.g. "extract selected items to folder X"). +/// One user-visible run: an extraction (e.g. "extract selected items to folder +/// X"), or a compress that has no window of its own to show its progress in. public struct ExtractionJob: Identifiable, Equatable, Sendable { public enum State: Equatable, Sendable { case running @@ -30,7 +31,22 @@ public struct ExtractionJob: Identifiable, Equatable, Sendable { case cancelled } + /// What the job does. The window shows both alike; what is said around it + /// differs — the quit warning, the name of the window. + public enum Kind: Equatable, Sendable { + case extraction + case compression + + fileprivate var label: String { + switch self { + case .extraction: "Extraction" + case .compression: "Compression" + } + } + } + public let id: UUID + public let kind: Kind public let archiveName: String public let destination: URL? public let itemCount: Int @@ -148,9 +164,9 @@ public struct ExtractionJob: Identifiable, Equatable, Sendable { } } -/// Global registry of running extractions. All archive windows (and the -/// window-less Finder-extension flows) report here so one progress window -/// can show everything that is being extracted right now. +/// Global registry of running jobs. All archive windows (and the window-less +/// Finder-extension flows) report here so one progress window can show +/// everything that is being extracted, or compressed from Finder, right now. @MainActor public final class ExtractionProgressCenter: ObservableObject { public static let shared = ExtractionProgressCenter() @@ -165,9 +181,19 @@ public final class ExtractionProgressCenter: ObservableObject { jobs.contains { !$0.isFinished } } + /// What the running jobs are doing, for the wording around the window: + /// extraction while one is running, compression when those are all that is + /// left, `nil` when nothing is. + public var runningKind: ExtractionJob.Kind? { + let running = jobs.filter { !$0.isFinished } + if running.contains(where: { $0.kind == .extraction }) { return .extraction } + return running.isEmpty ? nil : .compression + } + /// Registers a new running job and returns its id. @discardableResult public func begin( + kind: ExtractionJob.Kind = .extraction, archiveName: String, destination: URL?, itemCount: Int, @@ -175,6 +201,7 @@ public final class ExtractionProgressCenter: ObservableObject { ) -> UUID { let job = ExtractionJob( id: UUID(), + kind: kind, archiveName: archiveName, destination: destination, itemCount: itemCount, @@ -182,7 +209,7 @@ public final class ExtractionProgressCenter: ObservableObject { startedAt: Date() ) jobs.append(job) - log.notice("Extraction job started", context: [ + log.notice("\(kind.label) job started", context: [ "job": job.id.uuidString, "archive": archiveName, "destination": destination?.path ?? "(temp)", @@ -203,7 +230,8 @@ public final class ExtractionProgressCenter: ObservableObject { /// running until the extraction task acknowledges with `finish(_, .cancelled)`. public func requestCancel(_ id: UUID) { guard let handler = cancelHandlers.removeValue(forKey: id) else { return } - log.notice("Extraction cancel requested", context: ["job": id.uuidString]) + let label = jobs.first { $0.id == id }?.kind.label ?? ExtractionJob.Kind.extraction.label + log.notice("\(label) cancel requested", context: ["job": id.uuidString]) handler() } @@ -245,21 +273,22 @@ public final class ExtractionProgressCenter: ObservableObject { cancelHandlers[id] = nil let duration = Date().timeIntervalSince(jobs[index].startedAt) + let label = jobs[index].kind.label switch state { case .done: - log.notice("Extraction job done", context: [ + log.notice("\(label) job done", context: [ "job": id.uuidString, "archive": jobs[index].archiveName, "seconds": String(format: "%.2f", duration) ]) case .failed(let message): - log.error("Extraction job failed", context: [ + log.error("\(label) job failed", context: [ "job": id.uuidString, "archive": jobs[index].archiveName, "error": message ]) case .cancelled: - log.notice("Extraction job cancelled", context: [ + log.notice("\(label) job cancelled", context: [ "job": id.uuidString, "archive": jobs[index].archiveName ]) diff --git a/Modules/Sources/Core/Models/ArchiveItem.swift b/Modules/Sources/Core/Models/ArchiveItem.swift index af4bf3ef..2872b02d 100644 --- a/Modules/Sources/Core/Models/ArchiveItem.swift +++ b/Modules/Sources/Core/Models/ArchiveItem.swift @@ -50,30 +50,6 @@ public class ArchiveItem: Identifiable, Hashable, @unchecked Sendable { public private(set) var url: URL? = nil public private(set) var archiveTypeId: String? = nil - /// Creates an archive item from an existing real file (used for create/edit mode) - /// - Parameters: - /// - url: real file / folder - /// - archivePath: path this item will have inside the archive — kept so a - /// pending (unsaved) addition can be matched to its diff entry again - public init(url: URL, archivePath: String? = nil) { - self.name = url.lastPathComponent - self.type = url.isDirectory ? .directory : .file - self.virtualPath = archivePath - self.compressedSize = -1 - self.uncompressedSize = url.fileSize ?? -1 - self.modificationDate = url.modificationDate - self.posixPermissions = url.permissions - self.index = nil - self.ext = "" - - if type != .directory { - self.ext = getExtension(name: name) - } - if type == .directory { - self.children = [] - } - } - public init( index: UInt32? = nil, name: String, diff --git a/Modules/Sources/Swift7zip/SevenZipWriter.swift b/Modules/Sources/Swift7zip/SevenZipWriter.swift index 91ed1fb4..fc31820b 100644 --- a/Modules/Sources/Swift7zip/SevenZipWriter.swift +++ b/Modules/Sources/Swift7zip/SevenZipWriter.swift @@ -475,6 +475,19 @@ extension SevenZipArchive { return buffer.split(separator: 0).map { String(cString: Array($0) + [0]) } } + /// Whether `source` has anything `packMetadata` would put on the stand-in it + /// packs: an extended attribute worth keeping, or the hidden flag. With + /// neither, that stand-in is an empty file, and packing it gives the sample + /// `EmptyMetadata` compares against — a sidecar that is left out. + static func carriesMetadata(_ source: URL) throws -> Bool { + if try attributeNames(of: source).contains(where: { !systemOwnedAttributes.contains($0) }) { + return true + } + var status = stat() + guard lstat(source.path, &status) == 0 else { throw metadataError("lstat", source) } + return status.st_flags & UInt32(UF_HIDDEN) != 0 + } + /// Serializes what macOS keeps outside a file's contents — resource fork, /// extended attributes, and the FinderInfo that carries a custom-icon or /// invisible flag — into AppleDouble. Returns the file written. @@ -612,12 +625,19 @@ extension SevenZipArchive { continue } + // Nothing a sidecar would carry, so none is packed to find that out. + // That is nearly every file, and packing costs two scratch files and + // a read each: 8 seconds for 20,000 of them, before a single byte of + // the archive was written (#278). + guard try carriesMetadata(source) else { continue } + let values = try source.resourceValues( forKeys: [.isSymbolicLinkKey, .contentModificationDateKey]) if values.isSymbolicLink == true { continue } - // The only reason to leave a sidecar out: it was packed, and what came - // back says nothing. A failure above is a failure, not an absence. + // Otherwise the only reason to leave a sidecar out: it was packed, and + // what came back says nothing. A failure above is a failure, not an + // absence. let packed = try packMetadata(of: source, into: scratch) let bytes = try Data(contentsOf: packed) if empty.describesNothing(bytes) { continue } diff --git a/Modules/Tests/CoreTests/FolderAddTests.swift b/Modules/Tests/CoreTests/FolderAddTests.swift new file mode 100644 index 00000000..9770ae68 --- /dev/null +++ b/Modules/Tests/CoreTests/FolderAddTests.swift @@ -0,0 +1,487 @@ +// +// FolderAddTests.swift +// Modules +// +// Adding folders from disk: read off the main actor, put into the archive in +// one go, and — for a compress with no window of its own — reported to the +// progress center. All of it from #278, a project of 20,000 files that took +// 11 seconds to start compressing, with the app frozen and nothing to show. +// + +import Combine +import Testing +import Foundation +@testable import Swift7zip +@testable import Core + +/// A project folder: `files` small files, eight to a folder, the folders nested +/// the way a build leaves them. +func makeProject(named name: String = "FileApex", in dir: URL, files: Int) throws -> URL { + let project = dir.appendingPathComponent(name) + for index in 0.. [String] { + state.diff.compactMap { item in + switch item { + case .addFile(let path, _, _, _), .addDirectory(let path, _, _, _), .addData(let path, _, _, _): path + default: nil + } + } +} + +extension AllCoreTests { + + @MainActor struct FolderAddTests { + + private func makeState() -> ArchiveState { + ArchiveState(catalog: ArchiveTypeCatalog(), engineSelector: ArchiveEngineSelector7zip()) + } + + /// The turns something else gets on the main actor. + @MainActor private final class Turns { + var count = 0 + } + + /// The main actor keeps running other work while a folder is read. It + /// used to be read on it, file by file, with nothing else getting a + /// turn until the last one: no drawing, no progress, no cancel. + @Test func addingAFolderLeavesTheMainActorFree() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 2_000) + let state = makeState() + state.create() + + // Something else that wants the main actor, the way the UI does. + let turns = Turns() + let other = Task { + while !Task.isCancelled { + turns.count += 1 + await Task.yield() + } + } + defer { other.cancel() } + + let adding = state.add(url: project) + let before = turns.count + #expect(await adding.value) + let during = turns.count - before + + #expect(pendingPaths(state).contains("FileApex/module0/build/out0/Class0.kt")) + // Read on the main actor, the add leaves it a turn before it starts + // and one after it ends, and none in between. + #expect(during > 100, "the main actor got \(during) turns while the folder was read") + } + + /// What was read goes into the archive in one go. `diff` and `entries` + /// are published, and changed entry by entry each change copied all of + /// them: for 20,000 files most of the 11 seconds. + @Test func addingAFolderChangesTheArchiveOnce() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 400) + let onDisk = (FileManager.default.subpaths(atPath: project.path) ?? []).count + 1 + let state = makeState() + state.create() + + var diffChanges = 0 + var entriesChanges = 0 + let watching = [ + state.$diff.dropFirst().sink { _ in diffChanges += 1 }, + state.$entries.dropFirst().sink { _ in entriesChanges += 1 }, + ] + defer { watching.forEach { $0.cancel() } } + + #expect(await state.add(url: project).value) + + #expect(state.diff.count == onDisk) + #expect(state.itemCount == onDisk) + #expect(diffChanges == 1, "diff changed \(diffChanges) times") + #expect(entriesChanges == 1, "entries changed \(entriesChanges) times") + } + + /// Taking a folder out again is one change as well: it used to drop its + /// entries one by one, the same copy for each. + @Test func removingAFolderChangesTheArchiveOnce() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 400) + let state = makeState() + state.create() + #expect(await state.add(url: project).value) + let folder = try #require(state.childItems?.first) + + var diffChanges = 0 + var entriesChanges = 0 + let watching = [ + state.$diff.dropFirst().sink { _ in diffChanges += 1 }, + state.$entries.dropFirst().sink { _ in entriesChanges += 1 }, + ] + defer { watching.forEach { $0.cancel() } } + + state.remove(items: [folder]) + + #expect(state.diff.isEmpty, "\(state.diff.count) additions are still pending") + #expect(state.itemCount == 0) + #expect(diffChanges == 1, "diff changed \(diffChanges) times") + #expect(entriesChanges == 1, "entries changed \(entriesChanges) times") + } + + /// What the window shows for a pending entry is what the disk says about + /// it, and the entries come in the order they always did: a folder, then + /// what it holds, by name. A link is an entry of its own, not followed. + @Test func whatIsAddedShowsAsItIsOnDisk() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let fm = FileManager.default + let project = dir.appendingPathComponent("Project") + let build = project.appendingPathComponent("build") + try fm.createDirectory(at: build, withIntermediateDirectories: true) + try "fun main() {}".write(to: project.appendingPathComponent("Main.kt"), atomically: true, encoding: .utf8) + try "SECRET=1".write(to: project.appendingPathComponent(".env"), atomically: true, encoding: .utf8) + try "#!/bin/sh".write(to: project.appendingPathComponent("run.sh"), atomically: true, encoding: .utf8) + try Data(count: 4_096).write(to: build.appendingPathComponent("app.jar")) + try fm.createSymbolicLink(at: project.appendingPathComponent("link"), withDestinationURL: build) + try fm.setAttributes([.posixPermissions: 0o640], ofItemAtPath: project.appendingPathComponent("Main.kt").path) + try fm.setAttributes([.posixPermissions: 0o755], ofItemAtPath: project.appendingPathComponent("run.sh").path) + try fm.setAttributes( + [.modificationDate: Date(timeIntervalSince1970: 1_700_000_000)], + ofItemAtPath: build.appendingPathComponent("app.jar").path) + + let state = makeState() + state.create() + #expect(await state.add(url: project).value) + + #expect(pendingPaths(state) == [ + "Project", "Project/.env", "Project/Main.kt", "Project/build", "Project/build/app.jar", + "Project/link", "Project/run.sh", + ]) + for path in pendingPaths(state) { + let item = try #require(state.entries.values.first { $0.virtualPath == path }, "\(path) is not shown") + let onDisk = dir.appendingPathComponent(path) + let attributes = try fm.attributesOfItem(atPath: onDisk.path) + let isFolder = attributes[.type] as? FileAttributeType == .typeDirectory + #expect(item.type == (isFolder ? .directory : .file), "\(path)") + #expect(item.uncompressedSize == (attributes[.size] as? NSNumber)?.intValue, "\(path)") + #expect(item.modificationDate == attributes[.modificationDate] as? Date, "\(path)") + #expect(item.posixPermissions == (attributes[.posixPermissions] as? NSNumber)?.intValue, "\(path)") + #expect(item.name == onDisk.lastPathComponent) + } + let link = try #require(state.entries.values.first { $0.virtualPath == "Project/link" }) + #expect(link.children == nil, "the link was followed") + // and the tree is the one on disk + let shown = try #require(state.entries.values.first { $0.virtualPath == "Project" }) + let names = (shown.children ?? []).compactMap { state.entries[$0]?.name } + #expect(names == [".env", "Main.kt", "build", "link", "run.sh"]) + } + + /// Adds take their turn: a folder still being read does not get passed + /// by a single file dropped after it. + @Test func addsLandInTheOrderTheyWereStarted() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 400) + let single = dir.appendingPathComponent("notes.txt") + try "notes".write(to: single, atomically: true, encoding: .utf8) + let state = makeState() + state.create() + + state.add(url: project) + #expect(await state.add(url: single).value) + + #expect(pendingPaths(state).first == "FileApex") + #expect(pendingPaths(state).last == "notes.txt") + } + + /// Two things of one name can come in one add — picked from different + /// folders. They meet as they would have one after the other: the later + /// file replaces what was there, a folder is merged into a folder. + @Test func aNameTwiceInOneAddIsInTheArchiveOnce() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let fm = FileManager.default + for (place, file) in [("first", "a.txt"), ("second", "b.txt")] { + let src = dir.appendingPathComponent("\(place)/src") + try fm.createDirectory(at: src, withIntermediateDirectories: true) + try place.write(to: src.appendingPathComponent(file), atomically: true, encoding: .utf8) + try place.write(to: dir.appendingPathComponent("\(place)/notes.txt"), atomically: true, encoding: .utf8) + } + // a folder in one place, a file of the same name in the other + try fm.createDirectory(at: dir.appendingPathComponent("first/build"), withIntermediateDirectories: true) + try "jar".write(to: dir.appendingPathComponent("first/build/app.jar"), atomically: true, encoding: .utf8) + try "not a folder".write(to: dir.appendingPathComponent("second/build"), atomically: true, encoding: .utf8) + let picked = ["first/notes.txt", "second/notes.txt", "first/src", "second/src", "first/build", "second/build"] + .map { dir.appendingPathComponent($0) } + + let state = makeState() + state.create() + #expect(await state.add(urls: picked).value) + + #expect(pendingPaths(state) == ["notes.txt", "src", "src/a.txt", "src/b.txt", "build"]) + #expect(state.itemCount == 5) + let dest = dir.appendingPathComponent("out.zip") + await state.save(to: dest)?.value + #expect(state.error == nil, "\(state.error ?? "")") + #expect(try systemZipEntries(dest).sorted() == ["build", "notes.txt", "src/", "src/a.txt", "src/b.txt"]) + let extracted = dir.appendingPathComponent("extracted") + try run("/usr/bin/unzip", ["-q", dest.path, "-d", extracted.path]) + #expect(try String(contentsOf: extracted.appendingPathComponent("notes.txt"), encoding: .utf8) == "second") + #expect(try String(contentsOf: extracted.appendingPathComponent("build"), encoding: .utf8) == "not a folder") + } + + /// The status bar's cancel stops what is being read, and what was waiting + /// behind it, and leaves the archive as it was. For a load the same + /// button ends in an empty window; an add is not a load. + @Test func cancellingAnAddLeavesTheArchiveAsItWas() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 2_000) + let first = dir.appendingPathComponent("first.txt") + let later = dir.appendingPathComponent("later.txt") + try "first".write(to: first, atomically: true, encoding: .utf8) + try "later".write(to: later, atomically: true, encoding: .utf8) + let state = makeState() + state.create() + #expect(await state.add(url: first).value) + + let adding = state.add(url: project) + let waiting = state.add(url: later) + // busy from the moment the folder is being read + for _ in 0..<1_000 where !state.isBusy { await Task.yield() } + #expect(state.isBusy) + state.cancelCurrentOperation() + + #expect(await !adding.value) + #expect(await !waiting.value) + #expect(state.hasArchive, "the cancel closed the archive") + #expect(pendingPaths(state) == ["first.txt"]) + #expect(!state.isBusy) + // and it takes files again + #expect(await state.add(url: later).value) + #expect(pendingPaths(state) == ["first.txt", "later.txt"]) + } + + /// What an add was reading for is gone by the time it is read: the + /// window shows another archive now, and that one does not get it. + @Test func anAddIsDroppedOnceItsArchiveIsGone() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 64) + let state = makeState() + state.create() + + let adding = state.add(url: project) + state.create(named: "Another") + + #expect(await !adding.value) + #expect(state.diff.isEmpty, "\(state.diff.count) entries went into the wrong archive") + #expect(state.itemCount == 0) + } + + /// A save does not go ahead of files still being read: it would write the + /// archive without them. + @Test func nothingIsSavedWhileFilesAreStillBeingAdded() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 64) + let dest = dir.appendingPathComponent("out.zip") + let state = makeState() + state.create() + + let adding = state.add(url: project) + #expect(state.save(to: dest) == nil, "saved before the folder was read") + #expect(await adding.value) + #expect(!FileManager.default.fileExists(atPath: dest.path)) + + await state.save(to: dest)?.value + #expect(state.error == nil, "\(state.error ?? "")") + #expect(try systemZipList(dest).contains("FileApex/module0/build/out0/Class0.kt")) + } + } + + // MARK: - Metadata sidecars + + struct SidecarSelectionTests { + + /// A sidecar is packed only for what has something to carry. Packing one + /// to find out it says nothing cost two scratch files and a read per + /// entry: 8 seconds for 20,000 ordinary files, before anything was + /// written. What goes into the archive is unchanged — the tests around + /// `createdArchiveKeepsExtendedAttributes` say what that is. + @Test func onlyWhatHasMetadataIsPackedForASidecar() throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let fm = FileManager.default + func file(_ name: String) throws -> URL { + let url = dir.appendingPathComponent(name) + try "contents".write(to: url, atomically: true, encoding: .utf8) + return url + } + func tag(_ url: URL, _ name: String) { + let value = Array("value".utf8) + #expect(setxattr(url.path, name, value, value.count, 0, 0) == 0, "could not set \(name)") + } + + let plain = try file("plain.txt") + #expect(try !SevenZipArchive.carriesMetadata(plain)) + + let commented = try file("commented.txt") + tag(commented, "com.apple.metadata:kMDItemComment") + #expect(try SevenZipArchive.carriesMetadata(commented)) + + // describes this Mac, not the file: never packed, so nothing to carry + let downloaded = try file("downloaded.txt") + tag(downloaded, "com.apple.quarantine") + #expect(try !SevenZipArchive.carriesMetadata(downloaded)) + + // a filesystem flag no attribute mentions, carried as the FinderInfo one + let hidden = try file("hidden.txt") + #expect(chflags(hidden.path, UInt32(UF_HIDDEN)) == 0) + #expect(try SevenZipArchive.carriesMetadata(hidden)) + + let folder = dir.appendingPathComponent("folder") + try fm.createDirectory(at: folder, withIntermediateDirectories: true) + #expect(try !SevenZipArchive.carriesMetadata(folder)) + tag(folder, "com.apple.FinderInfo.custom") + #expect(try SevenZipArchive.carriesMetadata(folder)) + + // the link itself is asked, not what it points at + let link = dir.appendingPathComponent("link") + try fm.createSymbolicLink(at: link, withDestinationURL: commented) + #expect(try !SevenZipArchive.carriesMetadata(link)) + + // not being able to tell is a failure, not "nothing" + #expect(throws: (any Error).self) { + _ = try SevenZipArchive.carriesMetadata(dir.appendingPathComponent("gone.txt")) + } + } + } + + // MARK: - Compress without a window + + @MainActor struct CompressProgressTests { + + private func makeState(reportingTo center: ExtractionProgressCenter) -> ArchiveState { + let state = ArchiveState(catalog: ArchiveTypeCatalog(), engineSelector: ArchiveEngineSelector7zip()) + state.progressCenter = center + return state + } + + /// Finder's compress entries have no window to show progress in, so they + /// go to the progress center: the window that comes up for a long + /// extraction comes up for them as well. + @Test func aCompressWithNoWindowShowsInTheProgressCenter() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 64) + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + + #expect(await state.compress([project], to: dest, showingProgress: true)) + + #expect(center.jobs.count == 1) + let job = try #require(center.jobs.first) + #expect(job.kind == .compression) + #expect(job.archiveName == "FileApex.zip") + #expect(job.destination?.path == dir.path) + #expect(job.state == .done, "\(job.state)") + #expect(job.hasEngineProgress, "the writer's progress never reached the job") + #expect(try systemZipList(dest).contains("FileApex/module0/build/out0/Class0.kt")) + } + + /// Quick Compress and the start page show a row of their own for each + /// archive, and stay out of the progress window. + @Test func aCompressWithItsOwnRowStaysOutOfTheProgressCenter() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 16) + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + + #expect(await state.compress([project], to: dest)) + + #expect(center.jobs.isEmpty) + #expect(FileManager.default.fileExists(atPath: dest.path)) + } + + /// Cancel in the progress window stops the compress, and no archive is + /// left behind under the name it was going to have. + @Test func aCancelledCompressWritesNothing() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 2_000) + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + + let compressing = Task { await state.compress([project], to: dest, showingProgress: true) } + // the job is there as soon as the compress starts + for _ in 0..<1_000 where center.jobs.isEmpty { await Task.yield() } + let job = try #require(center.jobs.first, "the compress never showed up in the progress center") + center.requestCancel(job.id) + + #expect(await !compressing.value) + #expect(center.jobs.first?.state == .cancelled, "\(String(describing: center.jobs.first?.state))") + #expect(!FileManager.default.fileExists(atPath: dest.path), "a cancelled compress left an archive") + #expect(state.error == nil, "cancelling is not an error: \(state.error ?? "")") + } + + /// A compress from Finder that fails has nowhere else to say so: the job + /// fails with the reason, which keeps the progress window up. + @Test func aCompressThatFailsSaysWhy() async throws { + let dir = try makeTempDir() + let project = try makeProject(in: dir, files: 16) + let locked = project.appendingPathComponent("module0/build") + // Restored before the directory is removed, or the cleanup cannot + // descend into it either. + defer { + chmod(locked.path, 0o755) + try? FileManager.default.removeItem(at: dir) + } + #expect(chmod(locked.path, 0o000) == 0, "could not make the folder unreadable") + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + + #expect(await !state.compress([project], to: dest, showingProgress: true)) + + guard case .failed(let reason) = center.jobs.first?.state else { + Issue.record("the job did not fail: \(String(describing: center.jobs.first?.state))") + return + } + #expect(reason.contains("build"), "\(reason)") + #expect(!FileManager.default.fileExists(atPath: dest.path)) + } + + /// The quit warning and the window's name follow what is running: + /// extraction while there is one, compression when that is all there is. + @Test func whatIsRunningDecidesTheWording() { + let center = ExtractionProgressCenter() + #expect(center.runningKind == nil) + + let compress = center.begin( + kind: .compression, archiveName: "a.zip", destination: nil, itemCount: 1, totalBytes: nil) + #expect(center.runningKind == .compression) + + let extract = center.begin(archiveName: "b.zip", destination: nil, itemCount: 1, totalBytes: nil) + #expect(center.jobs.last?.kind == .extraction) + #expect(center.runningKind == .extraction) + + center.finish(extract, .done) + #expect(center.runningKind == .compression) + center.finish(compress, .cancelled) + #expect(center.runningKind == nil) + } + } +} diff --git a/Modules/Tests/CoreTests/SaveOptionsStateTests.swift b/Modules/Tests/CoreTests/SaveOptionsStateTests.swift index a02c8c1e..f4b0fe25 100644 --- a/Modules/Tests/CoreTests/SaveOptionsStateTests.swift +++ b/Modules/Tests/CoreTests/SaveOptionsStateTests.swift @@ -477,7 +477,7 @@ extension AllCoreTests { let state = ArchiveState(catalog: ArchiveTypeCatalog(), engineSelector: ArchiveEngineSelector7zip()) state.create() - state.add(url: folder) + #expect(await state.add(url: folder).value) let target = dir.appendingPathComponent("out.\(format.rawValue)") await state.save(to: target, options: .init(format: format, excludeDSStore: exclude))?.value #expect(state.error == nil, "\(state.error ?? "")") @@ -518,7 +518,7 @@ extension AllCoreTests { let state = ArchiveState(catalog: ArchiveTypeCatalog(), engineSelector: ArchiveEngineSelector7zip()) state.open(url: zip) try await state.openTask?.value - state.add(url: folder) + #expect(await state.add(url: folder).value) await state.save(options: .init(format: .zip, excludeDSStore: true))?.value #expect(state.error == nil, "\(state.error ?? "")") @@ -704,7 +704,7 @@ extension AllCoreTests { #expect(state.activeEngine == (engine == .xad ? .xad : .`7zip`)) let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) let format: CompressionOptions.Format = source.pathExtension == "7z" ? .sevenZ : .zip let saved = asACopy ? dir.appendingPathComponent("copy.\(format.rawValue)") : source @@ -752,7 +752,7 @@ extension AllCoreTests { try await state.openTask?.value let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) await state.save()?.value #expect(state.saveError != nil) @@ -783,7 +783,7 @@ extension AllCoreTests { try await state.openTask?.value let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) await state.save()?.value #expect(state.saveError?.contains("ASCII") == true, "\(state.saveError ?? "no reason")") @@ -821,7 +821,7 @@ extension AllCoreTests { try noise(bytes: 200_000).write(to: file) let state = makeState(Prompts([])) state.create() - state.add(url: file) + #expect(await state.add(url: file).value) await state.save(to: dir.appendingPathComponent("set.\(format.rawValue)"), options: .init(format: format, volumeSize: 64 << 10))?.value return state @@ -858,7 +858,7 @@ extension AllCoreTests { try await state.openTask?.value let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) let nested = try #require(state.entries.values.first { $0.name == "inner.zip" }) try await state.openAsync(item: nested) @@ -906,7 +906,7 @@ extension AllCoreTests { let before = try names.map { try Data(contentsOf: dir.appendingPathComponent($0)) } let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) await state.save()?.value #expect(state.saveError?.contains("Save As") == true, "\(state.saveError ?? "no reason")") @@ -932,7 +932,7 @@ extension AllCoreTests { let prompts = Prompts([nil]) let state = makeState(prompts) state.create() - state.add(url: file) + #expect(await state.add(url: file).value) let target = dir.appendingPathComponent("hidden.7z") await state.save(to: target, options: .init(format: .sevenZ, password: "pw", encryptFileNames: true))?.value #expect(state.error == nil, "\(state.error ?? "")") @@ -1003,7 +1003,7 @@ extension AllCoreTests { if withAnAddition { let added = dir.appendingPathComponent("added.txt") try "added".write(to: added, atomically: true, encoding: .utf8) - state.add(url: added) + #expect(await state.add(url: added).value) } await state.save(to: archive, options: .init(format: format, password: "fresh", diff --git a/Modules/Tests/CoreTests/ZipWriteTests.swift b/Modules/Tests/CoreTests/ZipWriteTests.swift index 7bb05f38..9823de7b 100644 --- a/Modules/Tests/CoreTests/ZipWriteTests.swift +++ b/Modules/Tests/CoreTests/ZipWriteTests.swift @@ -342,8 +342,8 @@ extension AllCoreTests { let state = makeState() state.create() #expect(state.canBeEdited) - state.add(url: fileA) - state.add(url: folder) + #expect(await state.add(url: fileA).value) + #expect(await state.add(url: folder).value) #expect(state.hasPendingChanges) let dest = dir.appendingPathComponent("created.zip") @@ -445,7 +445,7 @@ extension AllCoreTests { for name in ["a.txt", "b.txt"] { let file = dir.appendingPathComponent(name) try name.write(to: file, atomically: true, encoding: .utf8) - state.add(url: file) + #expect(await state.add(url: file).value) await state.save()?.value #expect(state.saveError == nil, "\(state.saveError ?? "")") } @@ -460,7 +460,7 @@ extension AllCoreTests { case .saveAsOverIt: let other = makeState() other.create() - other.add(url: dir.appendingPathComponent("src/other")) + #expect(await other.add(url: dir.appendingPathComponent("src/other")).value) await other.save(to: zip)?.value case .anotherApp: try run("/usr/bin/zip", ["-d", zip.path, "root.txt"]) @@ -491,7 +491,7 @@ extension AllCoreTests { let state = makeState() state.create() - state.add(url: fileA) + #expect(await state.add(url: fileA).value) let pendingBefore = state.diff.count // save() flips isSaving synchronously before handing back its Task, @@ -501,7 +501,7 @@ extension AllCoreTests { #expect(state.isSaving) // every mutating entry point must refuse while a save is running - state.add(url: extra) + #expect(await !state.add(url: extra).value) #expect(state.diff.count == pendingBefore, "add slipped in during save") if let victim = state.entries.values.first(where: { $0.name == "a.txt" }) { @@ -531,7 +531,7 @@ extension AllCoreTests { try await state.openTask?.value // add a file, then remove it again before saving — no net change - state.add(url: extra) + #expect(await state.add(url: extra).value) let pending = try #require(item("extra.txt", in: state)) state.remove(items: [pending]) #expect(!state.hasPendingChanges) @@ -556,7 +556,7 @@ extension AllCoreTests { try await state.openTask?.value #expect(state.canBeEdited) - state.add(url: replacement) + #expect(await state.add(url: replacement).value) let saveTask = try #require(state.save()) await saveTask.value #expect(state.error == nil) @@ -594,7 +594,7 @@ extension AllCoreTests { state.open(url: zip) try await state.openTask?.value - state.add(url: newFolder) + #expect(await state.add(url: newFolder).value) let saveTask = try #require(state.save()) await saveTask.value #expect(state.error == nil) @@ -737,7 +737,7 @@ extension AllCoreTests { #expect(!state.canRemove([x])) state.remove(items: [x]) #expect(!state.canAddHere) - state.add(url: inner) + #expect(await !state.add(url: inner).value) #expect(!state.hasPendingChanges) state.openParent() @@ -870,7 +870,7 @@ extension AllCoreTests { let state = makeState() state.create() - state.add(url: folder) + #expect(await state.add(url: folder).value) let dest = dir.appendingPathComponent("created.zip") let saveTask = try #require(state.save(to: dest)) @@ -1167,8 +1167,8 @@ extension AllCoreTests { let state = makeState() state.create() - state.add(url: file) - state.add(url: folder) + #expect(await state.add(url: file).value) + #expect(await state.add(url: folder).value) let dest = dir.appendingPathComponent("created.zip") let saveTask = try #require(state.save(to: dest)) From 8557a21271b18126af164186326b7e9cc62701c0 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 17:51:42 +0200 Subject: [PATCH 02/12] refactor: rename ArchiveScanner to FileScanner (#278) It reads files and folders from disk before they go into an archive and never opens one. ArchiveLoader is what reads archives, and the old name put the two side by side as if they did the same thing. --- Modules/Sources/Core/ArchiveState.swift | 6 +++--- .../Core/{ArchiveScanner.swift => FileScanner.swift} | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) rename Modules/Sources/Core/{ArchiveScanner.swift => FileScanner.swift} (96%) diff --git a/Modules/Sources/Core/ArchiveState.swift b/Modules/Sources/Core/ArchiveState.swift index 8a8f828a..28481a93 100644 --- a/Modules/Sources/Core/ArchiveState.swift +++ b/Modules/Sources/Core/ArchiveState.swift @@ -584,7 +584,7 @@ extension ArchiveState { /// Adds files and folders from disk where the window is; a folder goes in /// with everything it holds. /// - /// They are read off the main actor, by `ArchiveScanner`, so what was added + /// They are read off the main actor, by `FileScanner`, so what was added /// is there once the returned task ends. An add started meanwhile waits its /// turn, and what it adds comes after. /// @@ -652,7 +652,7 @@ extension ArchiveState { addCancel = cancel updateStatus(.processing) updateStatusText(String(localized: "loading...", bundle: .module, comment: "Archive operation status")) - let scan = try? await ArchiveScanner().scan(urls, under: base, cancel: cancel) + let scan = try? await FileScanner().scan(urls, under: base, cancel: cancel) // replaced while it was read: the same, and the busy state is no longer // this add's to clear @@ -682,7 +682,7 @@ extension ArchiveState { /// to replace would still be in there next to its replacement. Except a /// folder over a folder: that one is merged into, so only the files that /// collide inside it are replaced and the rest of what it holds stays. - private func put(_ scan: ArchiveScan, under target: ArchiveItem) { + private func put(_ scan: FileScan, under target: ArchiveItem) { var entries = self.entries var replaced = Removal() var additions: [ArchiveUpdateItem] = [] diff --git a/Modules/Sources/Core/ArchiveScanner.swift b/Modules/Sources/Core/FileScanner.swift similarity index 96% rename from Modules/Sources/Core/ArchiveScanner.swift rename to Modules/Sources/Core/FileScanner.swift index f2e18347..d2644393 100644 --- a/Modules/Sources/Core/ArchiveScanner.swift +++ b/Modules/Sources/Core/FileScanner.swift @@ -1,5 +1,5 @@ // -// ArchiveScanner.swift +// FileScanner.swift // Modules // // What is about to be added, read from disk: every file and folder below the @@ -13,7 +13,7 @@ import tb private let log = tb.Logger(subsystem: "app.MacPacker", category: "archive") /// What a scan found. -struct ArchiveScan: Sendable { +struct FileScan: Sendable { struct Entry: Sendable { /// What the window shows for it. Below the top it is already attached to /// the folder it is in. @@ -36,7 +36,7 @@ struct ArchiveScan: Sendable { /// can hold any number of files, and each one is a trip to the disk. Read on the /// main actor, a project of 20,000 files kept the app from drawing for 11 /// seconds (#278). -final actor ArchiveScanner { +final actor FileScanner { /// - Parameters: /// - urls: the files and folders picked @@ -44,9 +44,9 @@ final actor ArchiveScanner { /// folder's path with a trailing slash /// - cancel: looked at before each folder is read /// - Throws: `CancellationError` once `cancel` is set. - func scan(_ urls: [URL], under base: String, cancel: ExtractionCancelFlag) async throws -> ArchiveScan { + func scan(_ urls: [URL], under base: String, cancel: ExtractionCancelFlag) async throws -> FileScan { try await runBlocking { - var scan = ArchiveScan() + var scan = FileScan() for url in urls { let status = Self.status(of: url) let archivePath = base + url.lastPathComponent @@ -79,7 +79,7 @@ final actor ArchiveScanner { _ status: stat?, as archivePath: String, in parent: Int?, - into scan: inout ArchiveScan, + into scan: inout FileScan, cancel: ExtractionCancelFlag ) throws { let isFolder = status?.isFolder == true From 84f4b984b0859191df550e664ce4a93212276b54 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 18:05:03 +0200 Subject: [PATCH 03/12] fix: add a drop in one go and say why a read failed (#278) A drop onto an archive window added its files one by one: a thousand files were a thousand reads, a thousand changes to the archive and a thousand reloads of the list, 3.3 seconds where one add takes 0.015. The drop now hands everything it holds to one add, through the collector the start page already uses. A read that fails for another reason than being cancelled was swallowed with the cancel. The add came back empty-handed, the compress job failed without a reason, and Finder's handler logged it as cancelled. Nothing raises such an error today; when something does, it is reported. New test with a clock, next to the ones that watch for the two causes found: 20,000 files are in the archive within two seconds, best of three tries. It fails when the old three attribute reads per file come back, which the other tests let through. --- .../ArchiveContentViewer/ArchiveView.swift | 19 +++-- .../DropWindow/CompressSettings.swift | 3 +- Modules/Sources/Core/ArchiveState.swift | 23 +++++- Modules/Tests/CoreTests/FolderAddTests.swift | 80 +++++++++++++++++++ 4 files changed, 116 insertions(+), 9 deletions(-) diff --git a/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift b/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift index 9dfaeacc..23229694 100644 --- a/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift +++ b/MacPacker/Features/ArchiveContentViewer/ArchiveView.swift @@ -130,6 +130,17 @@ struct ArchiveView: View { "zone": zone.name ]) + // One drop is one add, however many files it holds. Added file by file, + // each was read, put into the archive and shown on its own: a thousand + // files, a thousand times over. + if zone == .add { + loadDroppedFileURLs(from: providers) { urls in + log.notice("Drop → add to current archive", context: ["files": "\(urls.count)"]) + state.add(urls: urls) + } + return + } + for provider in providers { provider.loadItem(forTypeIdentifier: UTType.fileURL.identifier, options: nil) { data, _ in guard let data = data as? Data, @@ -138,15 +149,11 @@ struct ArchiveView: View { return } Task { @MainActor in - switch zone { - case .add: - log.notice("Drop → add to current archive", context: ["file": url.lastPathComponent]) - state.add(url: url) - case .open where state.hasArchive: + if state.hasArchive { // This window is taken (and may hold unsaved edits). log.notice("Drop → open in a new window", context: ["file": url.lastPathComponent]) openInNewWindowAction(url) - case .open: + } else { log.notice("Drop → open in this window", context: ["file": url.lastPathComponent]) if state.isSupportedArchive(url: url) { RecentArchives.note(url) } state.openDropped(url: url) diff --git a/MacPacker/Features/DropWindow/CompressSettings.swift b/MacPacker/Features/DropWindow/CompressSettings.swift index d8c09f9a..d9016db9 100644 --- a/MacPacker/Features/DropWindow/CompressSettings.swift +++ b/MacPacker/Features/DropWindow/CompressSettings.swift @@ -76,7 +76,8 @@ struct CompressPinButton: View { } } -/// The urls of one drop, handed over together — one drop is one archive. +/// The urls of one drop, handed over together — one drop is one archive, or one +/// add to the archive a window shows. /// /// `loadItem` must be *started* inside the drop callback: the providers belong to /// the drag session and are unreliable once it ends. The answers arrive later, out diff --git a/Modules/Sources/Core/ArchiveState.swift b/Modules/Sources/Core/ArchiveState.swift index 28481a93..36895d68 100644 --- a/Modules/Sources/Core/ArchiveState.swift +++ b/Modules/Sources/Core/ArchiveState.swift @@ -146,6 +146,11 @@ public class ArchiveState: ObservableObject { /// and says whether there was one. The app wires this to its window manager; /// the default finds none, so Core and the unit tests open in place. public var focusWindowHolding: (URL) -> Bool = { _ in false } + /// Reads what is to be added from disk: `FileScanner`, off the main actor. + /// A property so that a test can hand in one that fails. + var scanFiles: @Sendable ([URL], String, ExtractionCancelFlag) async throws -> FileScan = { urls, base, cancel in + try await FileScanner().scan(urls, under: base, cancel: cancel) + } /// Where user-triggered extractions report their progress. Defaults to /// the app-wide center that feeds the extraction progress window; /// tests inject their own instance. @@ -652,7 +657,18 @@ extension ArchiveState { addCancel = cancel updateStatus(.processing) updateStatusText(String(localized: "loading...", bundle: .module, comment: "Archive operation status")) - let scan = try? await FileScanner().scan(urls, under: base, cancel: cancel) + var scan: FileScan? + var failure: String? + do { + scan = try await scanFiles(urls, base, cancel) + } catch is CancellationError { + // stopped on request: nothing was added, and nothing went wrong + } catch { + // Not swallowed with the cancel: the add would come back empty-handed + // without a word, and a compress fail for no reason it could name. + log.error("Failed to read what was to be added", context: ["error": String(describing: error)]) + failure = error.localizedDescription + } // replaced while it was read: the same, and the busy state is no longer // this add's to clear @@ -661,7 +677,10 @@ extension ArchiveState { addCancel = nil updateStatusText(nil) updateStatus(.done) - // cancelled, or the folder it was going into was removed meanwhile + if let failure { + self.error = failure + } + // cancelled or failed, or the folder it was going into was removed meanwhile guard let scan, target === root || entries[target.id] != nil else { return false } put(scan, under: target) diff --git a/Modules/Tests/CoreTests/FolderAddTests.swift b/Modules/Tests/CoreTests/FolderAddTests.swift index 9770ae68..75d5c56f 100644 --- a/Modules/Tests/CoreTests/FolderAddTests.swift +++ b/Modules/Tests/CoreTests/FolderAddTests.swift @@ -109,6 +109,63 @@ extension AllCoreTests { #expect(entriesChanges == 1, "entries changed \(entriesChanges) times") } + /// However many files come in one add, the archive changes once. A drop + /// onto a window hands everything it holds to one add for that reason: + /// added file by file, a thousand files were a thousand reads, a + /// thousand changes and a thousand reloads of the list. + @Test func manyFilesInOneAddChangeTheArchiveOnce() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let files = try (0..<300).map { index -> URL in + let file = dir.appendingPathComponent("file\(index).txt") + try Data("file \(index)".utf8).write(to: file) + return file + } + let state = makeState() + state.create() + + var diffChanges = 0 + var entriesChanges = 0 + let watching = [ + state.$diff.dropFirst().sink { _ in diffChanges += 1 }, + state.$entries.dropFirst().sink { _ in entriesChanges += 1 }, + ] + defer { watching.forEach { $0.cancel() } } + + #expect(await state.add(urls: files).value) + + // in the order they were handed over + #expect(pendingPaths(state) == files.map(\.lastPathComponent)) + #expect(diffChanges == 1, "diff changed \(diffChanges) times") + #expect(entriesChanges == 1, "entries changed \(entriesChanges) times") + } + + /// A time limit, where the tests above watch for the two causes that + /// were found: 20,000 files are in the archive within two seconds of + /// being added. As it is read now that takes 0.3 seconds on the machine + /// this was written on, and the CI runner is about 1.6 times slower; the + /// walk on the main actor took 13. Up to three tries, and the best one + /// counts: a busy machine only ever makes a run slower. + @Test func twentyThousandFilesAreAddedWithinTwoSeconds() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 20_000) + let limit = Duration.seconds(2) + + var times: [Duration] = [] + for _ in 0..<3 where (times.min() ?? limit) >= limit { + let state = makeState() + state.create() + let start = ContinuousClock.now + #expect(await state.add(url: project).value) + times.append(ContinuousClock.now - start) + #expect(state.diff.count > 20_000) + } + + let best = try #require(times.min()) + #expect(best < limit, "adding 20,000 files took \(times)") + } + /// Taking a folder out again is one change as well: it used to drop its /// entries one by one, the same copy for each. @Test func removingAFolderChangesTheArchiveOnce() async throws { @@ -464,6 +521,29 @@ extension AllCoreTests { #expect(!FileManager.default.fileExists(atPath: dest.path)) } + /// A read that fails for another reason than being cancelled says why: in + /// `error`, and in the job of a compress that has no window. Swallowed, the + /// add came back empty-handed and the job failed without a word, which + /// the Finder's handler then took for a cancel. + @Test func aReadThatFailsSaysWhy() async throws { + struct DiskGone: LocalizedError { + var errorDescription: String? { "The disk went away." } + } + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 16) + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + state.scanFiles = { _, _, _ in throw DiskGone() } + + #expect(await !state.compress([project], to: dest, showingProgress: true)) + + #expect(state.error == "The disk went away.") + #expect(center.jobs.first?.state == .failed("The disk went away."), "\(String(describing: center.jobs.first?.state))") + #expect(!FileManager.default.fileExists(atPath: dest.path)) + } + /// The quit warning and the window's name follow what is running: /// extraction while there is one, compression when that is all there is. @Test func whatIsRunningDecidesTheWording() { From cdc5c41b8e2ff1065a6774e34bdad63c8be0f790 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 18:05:03 +0200 Subject: [PATCH 04/12] core: changelog for 1.1.0 (#278) The block in progress becomes 1.1.0: there will be no 1.0.1. Two entries for the Finder compress work, next to the one for the empty zip. --- Config/products/macpacker.json | 50 +++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/Config/products/macpacker.json b/Config/products/macpacker.json index a81e610c..1bdd8c40 100644 --- a/Config/products/macpacker.json +++ b/Config/products/macpacker.json @@ -36,8 +36,32 @@ }, "versions": [ { - "version": "1.0.1", + "version": "1.1.0", "items": [ + { + "type": "feat", + "title": { + "en": "Progress window when compressing from Finder", + "zh-Hans": "从 Finder 压缩时显示进度窗口", + "fr": "Fenêtre de progression pour la compression depuis le Finder", + "de": "Fortschrittsfenster beim Komprimieren im Finder", + "it": "Finestra di avanzamento per la compressione dal Finder", + "ja": "Finder からの圧縮に進行状況ウィンドウを表示", + "fa": "پنجره پیشرفت هنگام فشرده‌سازی از Finder", + "pl": "Okno postępu przy kompresowaniu z Findera", + "pt-BR": "Janela de progresso ao comprimir pelo Finder", + "ru": "Окно хода сжатия из Finder", + "uk": "Вікно перебігу стиснення з Finder", + "es-MX": "Ventana de progreso al comprimir desde el Finder", + "ko": "Finder에서 압축할 때 진행 창 표시", + "nl": "Voortgangsvenster bij comprimeren in de Finder", + "tr": "Finder’dan sıkıştırırken ilerleme penceresi", + "vi": "Cửa sổ tiến trình khi nén từ Finder" + }, + "issues": [ + "278" + ] + }, { "type": "fix", "highlight": true, @@ -62,6 +86,30 @@ "issues": [ "278" ] + }, + { + "type": "fix", + "title": { + "en": "Compressing a large folder freezes the app", + "zh-Hans": "压缩大型文件夹会导致应用程序冻结", + "fr": "Compresser un gros dossier bloque l’application", + "de": "Großen Ordner komprimieren lässt die App einfrieren", + "it": "Comprimere una cartella grande blocca l’applicazione", + "ja": "大きなフォルダを圧縮するとアプリがフリーズする", + "fa": "فشرده‌سازی پوشه بزرگ باعث توقف برنامه می‌شود", + "pl": "Kompresowanie dużego folderu zawiesza aplikację", + "pt-BR": "Comprimir uma pasta grande faz o aplicativo travar", + "ru": "Сжатие большой папки приводит к зависанию приложения", + "uk": "Стиснення великої папки призводить до зависання застосунку", + "es-MX": "Comprimir una carpeta grande congela la aplicación", + "ko": "큰 폴더를 압축하면 앱이 멈춤", + "nl": "Een grote map comprimeren laat de app vastlopen", + "tr": "Büyük bir klasörü sıkıştırmak uygulamayı donduruyor", + "vi": "Nén thư mục lớn làm ứng dụng bị treo" + }, + "issues": [ + "278" + ] } ] }, From f0b43939760e023fc7b110f499217b59f84b6b32 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 18:13:54 +0200 Subject: [PATCH 05/12] core: print how long adding 20,000 files took (#278) In the log of every run, so the headroom under the two-second limit shows for the machine it ran on. --- Modules/Tests/CoreTests/FolderAddTests.swift | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/Tests/CoreTests/FolderAddTests.swift b/Modules/Tests/CoreTests/FolderAddTests.swift index 75d5c56f..4b9424e7 100644 --- a/Modules/Tests/CoreTests/FolderAddTests.swift +++ b/Modules/Tests/CoreTests/FolderAddTests.swift @@ -163,6 +163,8 @@ extension AllCoreTests { } let best = try #require(times.min()) + // in the log of every run, so the headroom on a given machine shows + print("Adding 20,000 files took \(times), the limit is \(limit)") #expect(best < limit, "adding 20,000 files took \(times)") } From 3e44d284f3a7454391806c9017aa84bb76881194 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 18:16:46 +0200 Subject: [PATCH 06/12] core: test the sidecars against Apple's own tool and a plain unzip (#278) The sidecars are AppleDouble under __MACOSX/, the form Finder's Compress writes. Until now only our own reader was asked to put them back. ditto, which runs when a zip is double-clicked, now is too: the metadata is on the file again and nothing of the sidecar tree is left. And unzip, which does not put it back, still gets every file whole, with the sidecar left where a zip from Finder leaves it. --- Modules/Tests/CoreTests/ZipWriteTests.swift | 62 +++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/Modules/Tests/CoreTests/ZipWriteTests.swift b/Modules/Tests/CoreTests/ZipWriteTests.swift index 9823de7b..458169fa 100644 --- a/Modules/Tests/CoreTests/ZipWriteTests.swift +++ b/Modules/Tests/CoreTests/ZipWriteTests.swift @@ -1179,6 +1179,68 @@ extension AllCoreTests { #expect(listed.contains { $0.hasPrefix("__MACOSX") } == false, "nothing here has metadata worth storing: \(listed)") } + + /// A folder with one file that has a Finder comment and one that has + /// nothing, written to a zip through the window's own path. + private func makeCommentedArchive(in dir: URL) async throws -> (zip: URL, comment: Data) { + let folder = dir.appendingPathComponent("Photos") + try FileManager.default.createDirectory(at: folder, withIntermediateDirectories: true) + let tagged = folder.appendingPathComponent("tagged.txt") + try "contents".write(to: tagged, atomically: true, encoding: .utf8) + try "plain".write(to: folder.appendingPathComponent("plain.txt"), atomically: true, encoding: .utf8) + let comment = Data("a comment".utf8) + setExtendedAttribute("com.apple.metadata:kMDItemFinderComment", comment, at: tagged) + + let state = makeState() + state.create() + #expect(await state.add(url: folder).value) + let dest = dir.appendingPathComponent("created.zip") + await state.save(to: dest)?.value + #expect(state.error == nil, "\(state.error ?? "")") + return (dest, comment) + } + + // The sidecars are not a format of our own. AppleDouble under `__MACOSX/` + // is what Finder's "Compress" writes, so Apple's own tool has to take an + // archive of ours the way it takes one of its own: the metadata back on + // the file, and nothing of the sidecar tree left over. `ditto` is what + // runs when a zip is double-clicked in Finder. + @Test func createdArchiveGivesItsMetadataBackToApplesOwnTool() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (zip, comment) = try await makeCommentedArchive(in: dir) + + let out = dir.appendingPathComponent("out") + try run("/usr/bin/ditto", ["-x", "-k", zip.path, out.path]) + + let extracted = out.appendingPathComponent("Photos/tagged.txt") + #expect(extendedAttribute("com.apple.metadata:kMDItemFinderComment", at: extracted) == comment, + "ditto did not put the comment back") + #expect(try String(contentsOf: extracted, encoding: .utf8) == "contents") + #expect(try String(contentsOf: out.appendingPathComponent("Photos/plain.txt"), encoding: .utf8) == "plain") + #expect(!FileManager.default.fileExists(atPath: out.appendingPathComponent("__MACOSX").path), + "ditto took the sidecars for ordinary files") + } + + // A tool that does not put the metadata back still gets every file, whole. + // What it cannot use stays out of the way: in `__MACOSX`, beside the + // folder and not inside it, where a zip made by Finder leaves it too. + @Test func createdArchiveExtractsWithAToolThatDoesNotKnowSidecars() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (zip, _) = try await makeCommentedArchive(in: dir) + + let out = dir.appendingPathComponent("out") + try run("/usr/bin/unzip", ["-q", zip.path, "-d", out.path]) + + let photos = out.appendingPathComponent("Photos") + #expect(try String(contentsOf: photos.appendingPathComponent("tagged.txt"), encoding: .utf8) == "contents") + #expect(try String(contentsOf: photos.appendingPathComponent("plain.txt"), encoding: .utf8) == "plain") + #expect(try FileManager.default.contentsOfDirectory(atPath: photos.path).sorted() == ["plain.txt", "tagged.txt"], + "a sidecar landed among the files") + #expect(FileManager.default.fileExists(atPath: out.appendingPathComponent("__MACOSX/Photos/._tagged.txt").path), + "the sidecar is not where a zip from Finder has it") + } } } From c2af4aeb1126c692ba8743478fc1c949aabc6c9a Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Fri, 9 Oct 2026 18:29:18 +0200 Subject: [PATCH 07/12] core: measure the add against a walk, not in seconds (#278) The two-second limit was set from this machine: 0.3 seconds here, and a guess of 0.45 for the CI runner. The runner's first report was 1.45, its disk being that much slower, which leaves no room for a busy day. Any number of seconds is either too tight there or too loose here. The add is now compared with a plain walk of the same 20,000 files, made in the same test: it may cost six times that. Here it costs 1.6 times. With the old three attribute reads per file it is 17 times and fails. --- Modules/Tests/CoreTests/FolderAddTests.swift | 55 ++++++++++++++------ 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/Modules/Tests/CoreTests/FolderAddTests.swift b/Modules/Tests/CoreTests/FolderAddTests.swift index 4b9424e7..727989db 100644 --- a/Modules/Tests/CoreTests/FolderAddTests.swift +++ b/Modules/Tests/CoreTests/FolderAddTests.swift @@ -140,32 +140,55 @@ extension AllCoreTests { #expect(entriesChanges == 1, "entries changed \(entriesChanges) times") } - /// A time limit, where the tests above watch for the two causes that - /// were found: 20,000 files are in the archive within two seconds of - /// being added. As it is read now that takes 0.3 seconds on the machine - /// this was written on, and the CI runner is about 1.6 times slower; the - /// walk on the main actor took 13. Up to three tries, and the best one - /// counts: a busy machine only ever makes a run slower. - @Test func twentyThousandFilesAreAddedWithinTwoSeconds() async throws { + /// A plain walk of `folder`: every folder listed, every entry looked at + /// once. The least that reading it can take, on this machine, right now. + private func walk(_ folder: URL) throws -> Int { + var seen = 0 + for entry in try FileManager.default.contentsOfDirectory(at: folder, includingPropertiesForKeys: nil) { + var status = stat() + #expect(lstat(entry.path, &status) == 0) + seen += 1 + if status.st_mode & S_IFMT == S_IFDIR { + seen += try walk(entry) + } + } + return seen + } + + /// A limit on time, where the tests above watch for the two causes that + /// were found: adding 20,000 files costs no more than a few times a + /// plain walk of them. Measured against the walk, not in seconds: the + /// same add takes 0.3 seconds on the machine this was written on and + /// 1.4 on the CI runner, whose disk is that much slower, so a number of + /// seconds is either too tight there or too loose here. The walk on the + /// main actor took 13 seconds here, a hundred times its own walk. + /// + /// Up to three tries, and one good one is enough: a busy machine only + /// ever makes a run slower. + @Test func addingTwentyThousandFilesCostsLittleMoreThanWalkingThem() async throws { let dir = try makeTempDir() defer { try? FileManager.default.removeItem(at: dir) } let project = try makeProject(in: dir, files: 20_000) - let limit = Duration.seconds(2) + let allowed = 6.0 + + var tries: [(add: Duration, walk: Duration)] = [] + for _ in 0..<3 where !tries.contains(where: { $0.add < $0.walk * allowed }) { + var start = ContinuousClock.now + let onDisk = try walk(project) + 1 + let walking = ContinuousClock.now - start - var times: [Duration] = [] - for _ in 0..<3 where (times.min() ?? limit) >= limit { let state = makeState() state.create() - let start = ContinuousClock.now + start = ContinuousClock.now #expect(await state.add(url: project).value) - times.append(ContinuousClock.now - start) - #expect(state.diff.count > 20_000) + tries.append((ContinuousClock.now - start, walking)) + #expect(state.diff.count == onDisk) } - let best = try #require(times.min()) // in the log of every run, so the headroom on a given machine shows - print("Adding 20,000 files took \(times), the limit is \(limit)") - #expect(best < limit, "adding 20,000 files took \(times)") + print("Adding 20,000 files, and a plain walk of them: \(tries)") + #expect(tries.contains { $0.add < $0.walk * allowed }, + "adding 20,000 files took more than \(allowed) times a walk of them: \(tries)") } /// Taking a folder out again is one change as well: it used to drop its From 53c78ecc01270eba20cf9cf07ef1f895fe44a257 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 11 Oct 2026 08:26:12 +0200 Subject: [PATCH 08/12] fix: a bookmark that left with its file no longer hides the folder's grant (#278) Compressing a folder from Finder failed with "Cannot create output file" in a folder the user had given access to, when an archive of that name had been there before, been opened once, and been deleted. Opening an archive stores a bookmark for it, for the list of recent archives. A bookmark follows its file: moved to the Trash it points into the Trash, and once the Trash is emptied it cannot be resolved. The lookup for a path's grant took the first bookmark found on the way up, which for the new archive's path was that one. Access was started on a file in the Trash, or the search ended with the error, and the folder's own grant one level up was never reached. A bookmark now counts for a path only while it still points at what is there, and one that cannot be resolved is passed over instead of ending the search. --- Modules/Sources/Core/Sandbox/Sandbox.swift | 111 +++++++++++++----- .../Tests/CoreTests/SandboxGrantTests.swift | 91 ++++++++++++++ 2 files changed, 170 insertions(+), 32 deletions(-) create mode 100644 Modules/Tests/CoreTests/SandboxGrantTests.swift diff --git a/Modules/Sources/Core/Sandbox/Sandbox.swift b/Modules/Sources/Core/Sandbox/Sandbox.swift index f5f8d653..223204c0 100644 --- a/Modules/Sources/Core/Sandbox/Sandbox.swift +++ b/Modules/Sources/Core/Sandbox/Sandbox.swift @@ -28,6 +28,11 @@ public nonisolated final class Sandbox: Sendable { /// Persist a security-scoped bookmark for `url`. Call right after the user has /// granted access (e.g. from an open panel) while the grant is still active. public static func storeBookmark(url: URL) { + storeBookmark(url: url, in: store) + } + + /// `store` is where the bookmarks are kept: the app's defaults, or a test's own. + static func storeBookmark(url: URL, in store: UserDefaults) { do { let bookmarkData = try url.bookmarkData( options: .withSecurityScope, @@ -82,49 +87,91 @@ public nonisolated final class Sandbox: Sendable { sandbox.getSecurityUrl(url: url) } - private func getSecurityUrl(url: URL) -> URL? { + static func securityScopedURL(for url: URL, in store: UserDefaults) -> URL? { + sandbox.getSecurityUrl(url: url, in: store) + } + + private func getSecurityUrl(url: URL, in store: UserDefaults = Sandbox.store) -> URL? { var securityUrl: URL? = nil autoreleasepool { - var isStale = false var subUrl: URL = url - do { - while subUrl.path().count > 1 { - let key = getBookmarkKey(url: subUrl) - - if let bookmarkData = Sandbox.store.data(forKey: key) { - let resolved = try URL( - resolvingBookmarkData: bookmarkData, - options: [.withSecurityScope, .withoutMounting, .withoutUI], - bookmarkDataIsStale: &isStale - ) - securityUrl = resolved - - // A stale-but-resolvable bookmark still grants access right - // now; refresh it from the resolved URL so it keeps working. - // Network bookmarks go stale whenever the volume is - // unmounted/remounted. Creating the new bookmark requires the - // scope to be held. - if isStale, resolved.startAccessingSecurityScopedResource() { - defer { resolved.stopAccessingSecurityScopedResource() } - if let refreshed = try? resolved.bookmarkData( - options: .withSecurityScope, - includingResourceValuesForKeys: nil, - relativeTo: nil) { - Sandbox.store.set(refreshed, forKey: key) - } + while subUrl.path().count > 1 { + let key = getBookmarkKey(url: subUrl) + var isStale = false + + if let bookmarkData = store.data(forKey: key), + let resolved = grant(in: bookmarkData, storedFor: subUrl, isStale: &isStale) { + securityUrl = resolved + + // A stale-but-resolvable bookmark still grants access right + // now; refresh it from the resolved URL so it keeps working. + // Network bookmarks go stale whenever the volume is + // unmounted/remounted. Creating the new bookmark requires the + // scope to be held. + if isStale, resolved.startAccessingSecurityScopedResource() { + defer { resolved.stopAccessingSecurityScopedResource() } + if let refreshed = try? resolved.bookmarkData( + options: .withSecurityScope, + includingResourceValuesForKeys: nil, + relativeTo: nil) { + store.set(refreshed, forKey: key) } - - return } - subUrl = subUrl.deletingLastPathComponent() + return } - } catch { - log.error("Could not resolve bookmark data: \(error.localizedDescription)") + + subUrl = subUrl.deletingLastPathComponent() } } return securityUrl } + + /// What the bookmark stored for `path` gives access to — if that is still + /// what is at `path`. Otherwise `nil`, and the search goes on further up. + /// + /// A bookmark follows its file. An archive that was opened has one, for the + /// list of recent archives: moved to the Trash, it points into the Trash, + /// and once the Trash is emptied it cannot be resolved at all. Neither is a + /// grant for `path` any longer. Taken for one, it hid the folder's grant + /// above it — being the first found on the way up, or by ending the search + /// with its error — and the next archive written under that name could not + /// be created, in a folder the user had given access to. + private func grant(in bookmarkData: Data, storedFor path: URL, isStale: inout Bool) -> URL? { + let resolved: URL + do { + resolved = try URL( + resolvingBookmarkData: bookmarkData, + options: [.withSecurityScope, .withoutMounting, .withoutUI], + bookmarkDataIsStale: &isStale + ) + } catch { + log.error("Could not resolve bookmark data: \(error.localizedDescription)") + return nil + } + guard isSameItem(resolved, path) else { + log.notice("Bookmark has left the path it was stored for", context: ["name": path.lastPathComponent]) + return nil + } + return resolved + } + + /// Whether two urls name the same file or folder. By what the file system + /// says they are, where it says; by path where one of them is not there to + /// ask about — which is the case this is for. + private func isSameItem(_ one: URL, _ other: URL) -> Bool { + func identity(_ url: URL) -> (any NSObjectProtocol)? { + // asked afresh: a url keeps what it was last told about its file, + // and that was before the file moved + var url = url + url.removeAllCachedResourceValues() + return (try? url.resourceValues(forKeys: [.fileResourceIdentifierKey]))?.fileResourceIdentifier + } + if let first = identity(one), let second = identity(other) { + return first.isEqual(second) + } + return one.resolvingSymlinksInPath().path == other.resolvingSymlinksInPath().path + } } diff --git a/Modules/Tests/CoreTests/SandboxGrantTests.swift b/Modules/Tests/CoreTests/SandboxGrantTests.swift new file mode 100644 index 00000000..aefecf6a --- /dev/null +++ b/Modules/Tests/CoreTests/SandboxGrantTests.swift @@ -0,0 +1,91 @@ +// +// SandboxGrantTests.swift +// Modules +// +// Which stored grant covers a path. Bookmarks are real ones, kept in a store +// of the test's own; whether access is then given is the sandbox's business +// and not tested here. +// + +import Testing +import Foundation +@testable import Core + +extension AllCoreTests { + + struct SandboxGrantTests { + + /// Bookmarks of this test only, gone again with `discard`. + private func makeStore() throws -> (store: UserDefaults, discard: () -> Void) { + let suite = "SandboxGrantTests-\(UUID().uuidString)" + let store = try #require(UserDefaults(suiteName: suite)) + return (store, { store.removePersistentDomain(forName: suite) }) + } + + /// Where a lookup ended up, comparable with a path the test made. + private func place(_ url: URL?) -> String? { + url?.resolvingSymlinksInPath().path + } + + /// A bookmark follows its file. An archive that was opened once has one, + /// for the list of recent archives; moved to the Trash, that bookmark + /// points into the Trash, and once the Trash is emptied at nothing. Its + /// old path is then covered by the folder's grant and by nothing else. + /// + /// It used to be taken for the path's grant all the same, being the first + /// one found on the way up: access was started on a file in the Trash, or + /// on nothing, and the archive that was to be written under the old name + /// could not be created — "Cannot create output file", in a folder the + /// user had given access to. + @Test func aBookmarkThatLeftWithItsFileDoesNotStandInForTheFoldersGrant() throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (store, discard) = try makeStore() + defer { discard() } + let fm = FileManager.default + let desktop = dir.appendingPathComponent("Desktop", isDirectory: true) + let trash = dir.appendingPathComponent("Trash", isDirectory: true) + try fm.createDirectory(at: desktop, withIntermediateDirectories: true) + try fm.createDirectory(at: trash, withIntermediateDirectories: true) + let archive = desktop.appendingPathComponent("FileApex.zip") + try Data("first".utf8).write(to: archive) + + Sandbox.storeBookmark(url: desktop, in: store) + Sandbox.storeBookmark(url: archive, in: store) + #expect(place(Sandbox.securityScopedURL(for: archive, in: store)) == place(archive), + "while the file is there, its own bookmark is the nearest") + + let trashed = trash.appendingPathComponent("FileApex.zip") + try fm.moveItem(at: archive, to: trashed) + #expect(place(Sandbox.securityScopedURL(for: archive, in: store)) == place(desktop), + "in the Trash") + + try fm.removeItem(at: trashed) + #expect(place(Sandbox.securityScopedURL(for: archive, in: store)) == place(desktop), + "Trash emptied") + + // Another file under the old name. A bookmark whose file is gone goes + // by its path, so the old one may find this file: either way what + // comes back is at the path again, or the folder above it. + try Data("second".utf8).write(to: archive) + #expect([place(archive), place(desktop)].contains(place(Sandbox.securityScopedURL(for: archive, in: store))), + "a new file by that name") + } + + /// With no grant above it, such a path has none at all — and the folder + /// is asked for again, rather than passing for covered. + @Test func aBookmarkThatLeftWithItsFileIsNoGrantForItsOldPath() throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let (store, discard) = try makeStore() + defer { discard() } + let archive = dir.appendingPathComponent("FileApex.zip") + try Data("first".utf8).write(to: archive) + Sandbox.storeBookmark(url: archive, in: store) + #expect(Sandbox.securityScopedURL(for: archive, in: store) != nil) + + try FileManager.default.moveItem(at: archive, to: dir.appendingPathComponent("elsewhere.zip")) + #expect(Sandbox.securityScopedURL(for: archive, in: store) == nil) + } + } +} From ce53886f8dee20b83a15252f9b028ab78d88b403 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 11 Oct 2026 08:26:12 +0200 Subject: [PATCH 09/12] fix: quitting stops a running compress instead of leaving half an archive (#278) "Quit Anyway" during Finder's compress ended the app under the writer, and what it had written so far stayed behind under the name the archive was to have. The compress is now cancelled first and the quit waits for it, up to five seconds: the writer removes what it wrote. The warning says so. --- MacPacker/AppDelegate.swift | 13 ++++-- MacPacker/Localizable.xcstrings | 4 +- .../Extraction/ExtractionProgressCenter.swift | 17 ++++++++ Modules/Tests/CoreTests/FolderAddTests.swift | 40 +++++++++++++++++++ 4 files changed, 69 insertions(+), 5 deletions(-) diff --git a/MacPacker/AppDelegate.swift b/MacPacker/AppDelegate.swift index 408eb144..794900b5 100644 --- a/MacPacker/AppDelegate.swift +++ b/MacPacker/AppDelegate.swift @@ -271,8 +271,8 @@ final class AppDelegate: NSObject, NSApplicationDelegate, ObservableObject { } /// Asked for in #119: don't let the app quit silently while an - /// extraction is running. Nor while Finder's compress is: quitting leaves - /// the archive half written under the name it was going to have. + /// extraction is running. Nor while Finder's compress is, which has no + /// window that would say so. public func applicationShouldTerminate(_ sender: NSApplication) -> NSApplication.TerminateReply { #if DEBUG // The extraction demo keeps a job "active" on purpose; don't let the @@ -304,7 +304,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate, ObservableObject { return .terminateCancel } log.notice("Quit forced during \(what)") - return .terminateNow + // What is being compressed is stopped first, and the quit waits for it: + // the writer then removes what it had written. Quit under it, and half + // an archive stays behind under the name the whole one was to have. + Task { @MainActor in + await ExtractionProgressCenter.shared.cancelAll(.compression, waitingUpTo: .seconds(5)) + NSApp.reply(toApplicationShouldTerminate: true) + } + return .terminateLater } public func applicationWillTerminate(_ notification: Notification) { diff --git a/MacPacker/Localizable.xcstrings b/MacPacker/Localizable.xcstrings index 050e903e..520c67c0 100644 --- a/MacPacker/Localizable.xcstrings +++ b/MacPacker/Localizable.xcstrings @@ -230,13 +230,13 @@ } }, "appQuitDuringCompressionWarning": { - "comment": "Body of the alert shown when the user quits while Finder's \"Compress to …\" is still writing an archive.", + "comment": "Body of the alert shown when the user quits while Finder's \"Compress to …\" is still writing an archive. Quitting cancels it and removes what was written so far.", "extractionState": "manual", "localizations": { "en": { "stringUnit": { "state": "translated", - "value": "Quitting now stops the compression and may leave an incomplete archive at the destination." + "value": "Quitting now stops the compression. The archive will not be created." } } } diff --git a/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift b/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift index cf8dcbed..636c147c 100644 --- a/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift +++ b/Modules/Sources/Core/Extraction/ExtractionProgressCenter.swift @@ -235,6 +235,23 @@ public final class ExtractionProgressCenter: ObservableObject { handler() } + /// Asks every running job of `kind` to stop, and comes back once they + /// have — for the app about to quit. After `limit` it comes back whatever + /// they did: a quit is not held up for ever by a job that will not stop. + public func cancelAll(_ kind: ExtractionJob.Kind, waitingUpTo limit: Duration) async { + func running() -> [UUID] { + jobs.filter { !$0.isFinished && $0.kind == kind }.map(\.id) + } + running().forEach(requestCancel) + + // ponytail: looked at every few milliseconds rather than woken by the + // job ending. It runs once, when the app quits. + let deadline = ContinuousClock.now + limit + while !running().isEmpty, ContinuousClock.now < deadline { + try? await Task.sleep(for: .milliseconds(20)) + } + } + /// Byte progress reported by the extraction engine itself. /// `date` must be the time the engine emitted the values — reports may /// arrive on the main queue in bursts, and speed math needs the real diff --git a/Modules/Tests/CoreTests/FolderAddTests.swift b/Modules/Tests/CoreTests/FolderAddTests.swift index 727989db..43194d20 100644 --- a/Modules/Tests/CoreTests/FolderAddTests.swift +++ b/Modules/Tests/CoreTests/FolderAddTests.swift @@ -569,6 +569,46 @@ extension AllCoreTests { #expect(!FileManager.default.fileExists(atPath: dest.path)) } + /// Quitting anyway stops what is being compressed before the app goes: + /// the writer then removes what it had written. Quit under it, and half + /// an archive stays behind under the name the whole one was to have. + @Test func quittingStopsACompressAndWaitsForItToEnd() async throws { + let dir = try makeTempDir() + defer { try? FileManager.default.removeItem(at: dir) } + let project = try makeProject(in: dir, files: 2_000) + let dest = dir.appendingPathComponent("FileApex.zip") + let center = ExtractionProgressCenter() + let state = makeState(reportingTo: center) + // an extraction next to it is not this call's to stop + let extraction = center.begin(archiveName: "other.zip", destination: nil, itemCount: 1, totalBytes: nil) + + let compressing = Task { await state.compress([project], to: dest, showingProgress: true) } + for _ in 0..<1_000 where center.jobs.count < 2 { await Task.yield() } + #expect(center.jobs.count == 2, "the compress never showed up in the progress center") + + await center.cancelAll(.compression, waitingUpTo: .seconds(30)) + + // by the time it comes back, the job has ended and nothing is left + #expect(center.jobs.last?.state == .cancelled, "\(String(describing: center.jobs.last?.state))") + #expect(!FileManager.default.fileExists(atPath: dest.path), "half an archive was left behind") + #expect(center.jobs.first { $0.id == extraction }?.state == .running) + #expect(await !compressing.value) + } + + /// A job that does not stop holds the quit up only so long. + @Test func quittingDoesNotWaitForEverForAJobThatWillNotStop() async { + let center = ExtractionProgressCenter() + let stubborn = center.begin( + kind: .compression, archiveName: "a.zip", destination: nil, itemCount: 1, totalBytes: nil) + var asked = false + center.setOnCancel(stubborn) { asked = true } + + await center.cancelAll(.compression, waitingUpTo: .milliseconds(100)) + + #expect(asked) + #expect(center.jobs.first?.state == .running) + } + /// The quit warning and the window's name follow what is running: /// extraction while there is one, compression when that is all there is. @Test func whatIsRunningDecidesTheWording() { From 2f3c3bc49a5c365f0121b3d64826f1df082adaaf Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 11 Oct 2026 08:33:38 +0200 Subject: [PATCH 10/12] fix: shorter quit warning for a running compress (#278) --- MacPacker/Localizable.xcstrings | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MacPacker/Localizable.xcstrings b/MacPacker/Localizable.xcstrings index 520c67c0..f64b139a 100644 --- a/MacPacker/Localizable.xcstrings +++ b/MacPacker/Localizable.xcstrings @@ -236,7 +236,7 @@ "en": { "stringUnit": { "state": "translated", - "value": "Quitting now stops the compression. The archive will not be created." + "value": "Quitting stops the compression." } } } From 45fc838cf9e56efd854100c9a46811fa5d30079d Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 11 Oct 2026 08:35:37 +0200 Subject: [PATCH 11/12] core: a bookmark passed over is a notice, not an error (#278) Every lookup of a path whose old bookmark is dead logged an error, four times per compress. It is expected now, and says which name it was for. --- Modules/Sources/Core/Sandbox/Sandbox.swift | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/Modules/Sources/Core/Sandbox/Sandbox.swift b/Modules/Sources/Core/Sandbox/Sandbox.swift index 223204c0..b8a80475 100644 --- a/Modules/Sources/Core/Sandbox/Sandbox.swift +++ b/Modules/Sources/Core/Sandbox/Sandbox.swift @@ -148,11 +148,18 @@ public nonisolated final class Sandbox: Sendable { bookmarkDataIsStale: &isStale ) } catch { - log.error("Could not resolve bookmark data: \(error.localizedDescription)") + // Not an error any more: what it was for is gone, and what is asked + // for may well be covered further up. + log.notice("Bookmark cannot be resolved, passing it over", context: [ + "name": path.lastPathComponent, + "error": error.localizedDescription + ]) return nil } guard isSameItem(resolved, path) else { - log.notice("Bookmark has left the path it was stored for", context: ["name": path.lastPathComponent]) + log.notice("Bookmark has left the path it was stored for, passing it over", context: [ + "name": path.lastPathComponent + ]) return nil } return resolved From 046407ed50198e756a2ec62e2ac3aba4a7cf2ff4 Mon Sep 17 00:00:00 2001 From: Stephan Arenswald Date: Sun, 11 Oct 2026 08:52:14 +0200 Subject: [PATCH 12/12] core: highlight the two new changelog entries (#278) The progress window for Finder's compress and the freeze fix show on the welcome window, next to the empty-zip fix. --- Config/products/macpacker.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Config/products/macpacker.json b/Config/products/macpacker.json index 1bdd8c40..85ebf112 100644 --- a/Config/products/macpacker.json +++ b/Config/products/macpacker.json @@ -40,6 +40,7 @@ "items": [ { "type": "feat", + "highlight": true, "title": { "en": "Progress window when compressing from Finder", "zh-Hans": "从 Finder 压缩时显示进度窗口", @@ -89,6 +90,7 @@ }, { "type": "fix", + "highlight": true, "title": { "en": "Compressing a large folder freezes the app", "zh-Hans": "压缩大型文件夹会导致应用程序冻结",