Skip to content

Fix bugs #1238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mvasilak merged 6 commits into from
Feb 17, 2026
Merged

Fix bugs #1238

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ad4f838
Fix potential TagFilterActionHandler crash during logout
mvasilak Feb 16, 2026
e9555d7
Fix RemoteAttachmentDownloader memory leak
mvasilak Feb 16, 2026
92d907f
Fix IdentifierLookupController memory leak
mvasilak Feb 16, 2026
6ad0caf
Normalize http arxiv URLs when in looking up via identifier
mvasilak Feb 16, 2026
fb656a5
Remove unnecessary comment
mvasilak Feb 17, 2026
1a4f058
Improve IdentifierLookupController URL normalization
mvasilak Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 41 additions & 45 deletions Zotero/Controllers/Attachment Downloader/RemoteAttachmentDownloader.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ final class RemoteAttachmentDownloader {
private let accessQueue: DispatchQueue
private let processingQueue: DispatchQueue
private let operationQueue: OperationQueue
private let disposeBag: DisposeBag
private unowned let apiClient: ApiClient
private unowned let fileStorage: FileStorage

Expand All @@ -44,7 +43,7 @@ final class RemoteAttachmentDownloader {
private var progressObservers: [Download: NSKeyValueObservation]
private var batchProgress: Progress?
private var totalBatchCount: Int = 0

var batchData: (Progress?, Int, Int) {
var progress: Progress?
var totalBatchCount = 0
Expand Down Expand Up @@ -76,7 +75,6 @@ final class RemoteAttachmentDownloader {
self.operations = [:]
self.progressObservers = [:]
self.errors = [:]
self.disposeBag = DisposeBag()
}

func data(for key: String, parentKey: String, libraryId: LibraryIdentifier) -> (progress: CGFloat?, error: Swift.Error?) {
Expand Down Expand Up @@ -113,7 +111,7 @@ final class RemoteAttachmentDownloader {
}
operationQueue.addOperations(operations, waitUntilFinished: false)
}

func createDownload(url: URL, attachment: Attachment, parentKey: String) -> (Download, RemoteAttachmentDownloadOperation)? {
let download = Download(key: attachment.key, parentKey: parentKey, libraryId: attachment.libraryId)

Expand All @@ -122,19 +120,19 @@ final class RemoteAttachmentDownloader {
let progress = Progress(totalUnitCount: 100)
let operation = RemoteAttachmentDownloadOperation(url: url, file: file, progress: progress, apiClient: apiClient, fileStorage: fileStorage, queue: processingQueue)
operation.finishedDownload = { [weak self] result in
self?.accessQueue.async(flags: .barrier) {
finish(download: download, file: file, attachment: attachment, parentKey: parentKey, result: result)
self?.accessQueue.async(flags: .barrier) { [weak self] in
self?.finish(download: download, file: file, attachment: attachment, parentKey: parentKey, result: result)
}
}
operation.progressHandler = { [weak self] progress in
self?.accessQueue.async(flags: .barrier) {
observe(progress: progress, attachment: attachment, download: download)
self?.accessQueue.async(flags: .barrier) { [weak self] in
self?.observe(progress: progress, attachment: attachment, download: download)
}
}

operations[download] = operation
totalBatchCount += 1

if let batchProgress {
batchProgress.addChild(progress, withPendingUnitCount: 100)
batchProgress.totalUnitCount += 100
Expand All @@ -145,45 +143,43 @@ final class RemoteAttachmentDownloader {
}

return (download, operation)

@Sendable func finish(download: Download, file: File, attachment: Attachment, parentKey: String, result: Result<(), Swift.Error>) {
operations[download] = nil
progressObservers[download] = nil
resetBatchDataIfNeeded()

switch result {
case .success:
DDLogInfo("RemoteAttachmentDownloader: finished downloading \(download.key)")
observable.on(.next(Update(download: download, kind: .ready(attachment))))
errors[download] = nil

case .failure(let error):
DDLogError("RemoteAttachmentDownloader: failed to download attachment \(download.key), \(download.libraryId) - \(error)")

let isCancelError = (error as? RemoteAttachmentDownloadOperation.Error) == .cancelled
errors[download] = isCancelError ? nil : error

if isCancelError {
observable.on(.next(Update(download: download, kind: .cancelled)))
} else {
observable.on(.next(Update(download: download, kind: .failed)))
}
}
}

@Sendable func observe(progress: Progress, attachment: Attachment, download: Download) {
let observer = progress.observe(\.fractionCompleted) { [weak self] progress, _ in
self?.observable.on(.next(Update(download: download, kind: .progress(CGFloat(progress.fractionCompleted)))))
}
progressObservers[download] = observer
}
}

private func finish(download: Download, file: File, attachment: Attachment, parentKey: String, result: Result<(), Swift.Error>) {
operations[download] = nil
progressObservers[download] = nil
if operations.isEmpty {
// Reset batch data.
totalBatchCount = 0
batchProgress = nil
}

switch result {
case .success:
DDLogInfo("RemoteAttachmentDownloader: finished downloading \(download.key)")
observable.on(.next(Update(download: download, kind: .ready(attachment))))
errors[download] = nil

case .failure(let error):
DDLogError("RemoteAttachmentDownloader: failed to download attachment \(download.key), \(download.libraryId) - \(error)")

let isCancelError = (error as? RemoteAttachmentDownloadOperation.Error) == .cancelled
errors[download] = isCancelError ? nil : error

if isCancelError {
observable.on(.next(Update(download: download, kind: .cancelled)))
} else {
observable.on(.next(Update(download: download, kind: .failed)))
}
}
}

private func resetBatchDataIfNeeded() {
guard operations.isEmpty else { return }
totalBatchCount = 0
batchProgress = nil

private func observe(progress: Progress, attachment: Attachment, download: Download) {
let observer = progress.observe(\.fractionCompleted) { [weak self] progress, _ in
self?.observable.on(.next(Update(download: download, kind: .progress(CGFloat(progress.fractionCompleted)))))
}
progressObservers[download] = observer
}

func stop() {
Expand Down
Loading