Skip to content
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

Moved diffable data sources to background queue #1033

Merged
merged 9 commits into from
Dec 4, 2024
Merged

Moved diffable data sources to background queue #1033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d06cdf9
Moved lookup controller data source to background
michalrentka Nov 25, 2024
83be9df
Moved pdf annotations diffable data source to background
michalrentka Nov 27, 2024
c8d8fa9
Refactored collection picker to background queue
michalrentka Nov 28, 2024
a450f5d
Refactored collections search to background queue
michalrentka Nov 28, 2024
4789c78
Refactored collections controller to background queue
michalrentka Nov 28, 2024
603b87f
Lookup code review changes
michalrentka Dec 3, 2024
596ab95
Removed unnecessary code
michalrentka Dec 3, 2024
b9ae603
Code review changes
michalrentka Dec 3, 2024
5089eda
Code review fix
michalrentka Dec 3, 2024
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
2 changes: 1 addition & 1 deletion Zotero/Scenes/Detail/ItemDetail/Views/ItemDetailCollectionViewHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,7 @@ final class ItemDetailCollectionViewHandler: NSObject {

let toReload = rowsToReload(from: oldRows, to: newRows, in: section)
if !toReload.isEmpty {
snapshot.reloadItems(toReload)
snapshot.reconfigureItems(toReload)
}

dataSource.apply(snapshot, animatingDifferences: animated)
Expand Down
194 changes: 102 additions & 92 deletions Zotero/Scenes/Detail/Lookup/Views/LookupViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class LookupViewController: UIViewController {
private unowned let remoteFileDownloader: RemoteAttachmentDownloader
private unowned let schemaController: SchemaController
private let disposeBag: DisposeBag
private let updateQueue: DispatchQueue

init(
viewModel: ViewModel<LookupActionHandler>,
Expand All @@ -65,6 +66,7 @@ class LookupViewController: UIViewController {
self.remoteFileDownloader = remoteFileDownloader
self.schemaController = schemaController
self.disposeBag = DisposeBag()
updateQueue = DispatchQueue(label: "org.zotero.LookupViewController.UpdateQueue")
super.init(nibName: "LookupViewController", bundle: nil)
self.setupAttachmentObserving(observer: remoteDownloadObserver)
}
Expand Down Expand Up @@ -100,124 +102,131 @@ class LookupViewController: UIViewController {
private func update(state: LookupState) {
switch state.lookupState {
case .failed(let error):
self.tableView.isHidden = true
self.activityIndicator.stopAnimating()
self.activityIndicator.isHidden = true
self.errorLabel.text = error.localizedDescription
self.errorLabel.isHidden = false
tableView.isHidden = true
activityIndicator.stopAnimating()
activityIndicator.isHidden = true
errorLabel.text = error.localizedDescription
errorLabel.isHidden = false

case .waitingInput:
self.tableView.isHidden = true
self.errorLabel.isHidden = true
self.activityIndicator.stopAnimating()
self.activityIndicator.isHidden = true
tableView.isHidden = true
errorLabel.isHidden = true
activityIndicator.stopAnimating()
activityIndicator.isHidden = true

case .loadingIdentifiers:
self.tableView.isHidden = true
self.errorLabel.isHidden = true
self.activityIndicator.isHidden = false
self.activityIndicator.startAnimating()
tableView.isHidden = true
errorLabel.isHidden = true
activityIndicator.isHidden = false
activityIndicator.startAnimating()

case .lookup(let data):
// It takes a little while for the `contentSize` observer notification to come, so all the content is hidden after the notification arrives, so that there is not an empty screen while
// waiting for it.
self.show(data: data) { [weak self] in
guard let self = self else { return }
self.activityIndicator.stopAnimating()
self.activityIndicator.isHidden = true
self.errorLabel.isHidden = true
self.tableView.isHidden = false

self.dataReloaded?()

self.closeAfterUpdateIfNeeded()
show(data: data) { [weak self] in
guard let self else { return }
activityIndicator.stopAnimating()
activityIndicator.isHidden = true
errorLabel.isHidden = true
tableView.isHidden = false
dataReloaded?()
closeAfterUpdateIfNeeded()
}
}
}

private func show(data: [LookupState.LookupData], completion: @escaping () -> Void) {
var snapshot = NSDiffableDataSourceSnapshot<Int, Row>()
snapshot.appendSections([0])

for lookup in data {
switch lookup.state {
case .translated(let translationData):
let title: String
if let _title = translationData.response.fields[KeyBaseKeyPair(key: FieldKeys.Item.title, baseKey: nil)] {
title = _title
} else {
let _title = translationData.response.fields.first(where: { self.schemaController.baseKey(for: translationData.response.rawType, field: $0.key.key) == FieldKeys.Item.title })?.value
title = _title ?? ""
}
let itemData = Row.Item(type: translationData.response.rawType, title: title)

snapshot.appendItems([.item(itemData)], toSection: 0)
tableView.isHidden = false

let attachments = translationData.attachments.map({ attachment -> Row in
let (progress, error) = self.remoteFileDownloader.data(for: attachment.0.key, parentKey: translationData.response.key, libraryId: attachment.0.libraryId)
let updateKind: RemoteAttachmentDownloader.Update.Kind
if error != nil {
updateKind = .failed
} else if let progress = progress {
updateKind = .progress(progress)
} else {
updateKind = .ready(attachment.0)
}
return .attachment(attachment.0, updateKind)
})
snapshot.appendItems(attachments, toSection: 0)
updateQueue.async { [weak self] in
guard let self else { return }

case .failed:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .failed)])

case .inProgress:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .inProgress)])
let snapshot = createSnapshot(from: data, schemaController: schemaController, remoteFileDownloader: remoteFileDownloader)
dataSource.apply(snapshot, animatingDifferences: false) { [weak self] in
guard let self, contentSizeObserver == nil else {
completion()
return
}

case .enqueued:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .enqueued)])
// For some reason, the observer subscription has to be here, doesn't work if it's in `viewDidLoad`.
contentSizeObserver = tableView.observe(\.contentSize, options: [.new]) { [weak self] _, change in
guard let self, let value = change.newValue, value.height != tableViewHeight.constant else { return }
tableViewHeight.constant = value.height
if value.height >= self.tableView.frame.height, !tableView.isScrollEnabled {
tableView.isScrollEnabled = true
}
completion()
}
}
}

self.tableView.isHidden = false
self.dataSource.apply(snapshot, animatingDifferences: false)

guard self.contentSizeObserver == nil else {
completion()
return
}
// For some reason, the observer subscription has to be here, doesn't work if it's in `viewDidLoad`.
self.contentSizeObserver = self.tableView.observe(\.contentSize, options: [.new]) { [weak self] _, change in
guard let self = self, let value = change.newValue, value.height != self.tableViewHeight.constant else { return }
func createSnapshot(from data: [LookupState.LookupData], schemaController: SchemaController, remoteFileDownloader: RemoteAttachmentDownloader) -> NSDiffableDataSourceSnapshot<Int, Row> {
var snapshot = NSDiffableDataSourceSnapshot<Int, Row>()
snapshot.appendSections([0])

self.tableViewHeight.constant = value.height

if value.height >= self.tableView.frame.height, !self.tableView.isScrollEnabled {
self.tableView.isScrollEnabled = true
for lookup in data {
switch lookup.state {
case .translated(let translationData):
let title: String
if let _title = translationData.response.fields[KeyBaseKeyPair(key: FieldKeys.Item.title, baseKey: nil)] {
title = _title
} else {
let _title = translationData.response.fields.first(where: { schemaController.baseKey(for: translationData.response.rawType, field: $0.key.key) == FieldKeys.Item.title })?.value
title = _title ?? ""
}
let itemData = Row.Item(type: translationData.response.rawType, title: title)

snapshot.appendItems([.item(itemData)], toSection: 0)

let attachments = translationData.attachments.map({ attachment -> Row in
let (progress, error) = remoteFileDownloader.data(for: attachment.0.key, parentKey: translationData.response.key, libraryId: attachment.0.libraryId)
let updateKind: RemoteAttachmentDownloader.Update.Kind
if error != nil {
updateKind = .failed
} else if let progress = progress {
updateKind = .progress(progress)
} else {
updateKind = .ready(attachment.0)
}
return .attachment(attachment.0, updateKind)
})
snapshot.appendItems(attachments, toSection: 0)

case .failed:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .failed)])

case .inProgress:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .inProgress)])

case .enqueued:
snapshot.appendItems([.identifier(identifier: lookup.identifier, state: .enqueued)])
}
}

completion()
return snapshot
}
}

private func process(update: RemoteAttachmentDownloader.Update) {
guard update.download.libraryId == self.viewModel.state.libraryId, var snapshot = self.dataSource?.snapshot(), !snapshot.sectionIdentifiers.isEmpty else { return }

var rows = snapshot.itemIdentifiers(inSection: 0)
private func process(update: RemoteAttachmentDownloader.Update, completion: @escaping () -> Void) {
guard update.download.libraryId == viewModel.state.libraryId, var snapshot = dataSource?.snapshot(), !snapshot.sectionIdentifiers.isEmpty else { return }

guard let index = rows.firstIndex(where: { $0.isAttachment(withKey: update.download.key, libraryId: update.download.libraryId) }) else { return }
updateQueue.async { [weak self] in
guard let self else { return }

snapshot.deleteItems(rows)
var rows = snapshot.itemIdentifiers(inSection: 0)
guard let index = rows.firstIndex(where: { $0.isAttachment(withKey: update.download.key, libraryId: update.download.libraryId) }) else { return }
snapshot.deleteItems(rows)
let row = rows[index]
switch row {
case .attachment(let attachment, _):
rows[index] = .attachment(attachment, update.kind)

let row = rows[index]
switch row {
case .attachment(let attachment, _):
rows[index] = .attachment(attachment, update.kind)
case .item, .identifier: break
case .item, .identifier:
break
}
snapshot.appendItems(rows, toSection: 0)
dataSource.apply(snapshot, animatingDifferences: false, completion: completion)
}

snapshot.appendItems(rows, toSection: 0)

self.dataSource.apply(snapshot, animatingDifferences: false)
}

private func closeAfterUpdateIfNeeded() {
Expand Down Expand Up @@ -300,9 +309,10 @@ class LookupViewController: UIViewController {

private func setupAttachmentObserving(observer: PublishSubject<RemoteAttachmentDownloader.Update>) {
observer.observe(on: MainScheduler.instance)
.subscribe(with: self, onNext: { `self`, update in
self.process(update: update)
self.closeAfterUpdateIfNeeded()
.subscribe(onNext: { [weak self] update in
self?.process(update: update) { [weak self] in
self?.closeAfterUpdateIfNeeded()
}
})
.disposed(by: self.disposeBag)
}
Expand Down
60 changes: 36 additions & 24 deletions Zotero/Scenes/Detail/PDF/Views/PDFAnnotationsViewController.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ protocol AnnotationsDelegate: AnyObject {
final class PDFAnnotationsViewController: UIViewController {
private static let cellId = "AnnotationCell"
private unowned let viewModel: ViewModel<PDFReaderActionHandler>
private let updateQueue: DispatchQueue
private let disposeBag: DisposeBag

private weak var emptyLabel: UILabel!
Expand All @@ -44,7 +45,8 @@ final class PDFAnnotationsViewController: UIViewController {

init(viewModel: ViewModel<PDFReaderActionHandler>) {
self.viewModel = viewModel
self.disposeBag = DisposeBag()
disposeBag = DisposeBag()
updateQueue = DispatchQueue(label: "org.zotero.PDFAnnotationsViewController.UpdateQueue")
super.init(nibName: nil, bundle: nil)
}

Expand All @@ -66,13 +68,16 @@ final class PDFAnnotationsViewController: UIViewController {
override func viewIsAppearing(_ animated: Bool) {
super.viewIsAppearing(animated)

var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(viewModel.state.sortedKeys)
tableView.setEditing(viewModel.state.sidebarEditingEnabled, animated: false)
dataSource.apply(snapshot, animatingDifferences: false)
if let key = viewModel.state.focusSidebarKey, let indexPath = dataSource.indexPath(for: key) {
tableView.selectRow(at: indexPath, animated: false, scrollPosition: .middle)
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(viewModel.state.sortedKeys)
dataSource.apply(snapshot, animatingDifferences: false) { [weak self] in
guard let self, let key = viewModel.state.focusSidebarKey, let indexPath = dataSource.indexPath(for: key) else { return }
tableView.selectRow(at: indexPath, animated: false, scrollPosition: .middle)
}
}

viewModel.stateObservable
Expand Down Expand Up @@ -155,7 +160,7 @@ final class PDFAnnotationsViewController: UIViewController {
emptyLabel.isHidden = !tableView.isHidden
}

self.reloadIfNeeded(for: state) { [weak self] in
reloadIfNeeded(for: state) { [weak self] in
guard let self else { return }

if let keys = state.loadedPreviewImageAnnotationKeys {
Expand Down Expand Up @@ -202,33 +207,40 @@ final class PDFAnnotationsViewController: UIViewController {
let isVisible = parentDelegate?.isSidebarVisible ?? false

if state.changes.contains(.annotations) {
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(state.sortedKeys)
if let keys = state.updatedAnnotationKeys {
snapshot.reloadItems(keys)
}

if state.changes.contains(.sidebarEditing) {
tableView.setEditing(state.sidebarEditingEnabled, animated: isVisible)
tableView.setEditing(state.sidebarEditingEnabled, animated: false)
}
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = NSDiffableDataSourceSnapshot<Int, PDFReaderState.AnnotationKey>()
snapshot.appendSections([0])
snapshot.appendItems(state.sortedKeys)
if let keys = state.updatedAnnotationKeys {
snapshot.reloadItems(keys)
michalrentka marked this conversation as resolved.
Show resolved Hide resolved
}
dataSource.apply(snapshot, animatingDifferences: isVisible, completion: completion)
}
dataSource.apply(snapshot, animatingDifferences: isVisible, completion: completion)
return
}

if state.changes.contains(.interfaceStyle) {
var snapshot = dataSource.snapshot()
guard !snapshot.sectionIdentifiers.isEmpty else { return }
snapshot.reloadSections([0])
dataSource.apply(snapshot, animatingDifferences: false, completion: completion)
updateQueue.async { [weak self] in
guard let self else { return }
var snapshot = dataSource.snapshot()
guard !snapshot.sectionIdentifiers.isEmpty else { return }
snapshot.reloadSections([0])
dataSource.apply(snapshot, animatingDifferences: false, completion: completion)
}
return
}

if state.changes.contains(.selection) || state.changes.contains(.activeComment) {
if let keys = state.updatedAnnotationKeys {
var snapshot = dataSource.snapshot()
snapshot.reloadItems(keys)
dataSource.apply(snapshot, animatingDifferences: false)
updateQueue.sync { [unowned self] in
var snapshot = dataSource.snapshot()
snapshot.reloadItems(keys)
michalrentka marked this conversation as resolved.
Show resolved Hide resolved
dataSource.apply(snapshot, animatingDifferences: false)
}
}

updateCellHeight()
Expand Down
Loading